Comment 34 for bug 235200

Revision history for this message
In , Darin-moz (darin-moz) wrote :

(From update of attachment 150153)
>Index: netwerk/base/src/nsInputStreamPump.cpp

>+ printf("*** offsets: after: %lld, before: %lld\n", offsetAfter, offsetBefore);

you didn't mean to keep this printf in, right?

>Index: netwerk/base/src/nsInputStreamPump.h

>- PRUint32 mStreamOffset;
>- PRUint32 mStreamLength;
>+ nsInt64 mStreamOffset;
>+ nsInt64 mStreamLength;

slightly concerning to me that this also means a change from
unsigned to signed arithmetic. please be sure to double-check
that we aren't making any assumptions anywhere about these being
unsigned.

>Index: netwerk/base/src/nsResumableEntityID.cpp

>+ mSize(LL_INIT(0xffffffff, 0xffffffff)) {

LL_MAXUINT?

>+ if (LL_EQ(mSize, LL_INIT(0xffffffff, 0xffffffff)))

same here,

>+ size = LL_INIT(0xffffffff, 0xffffffff);

and here.

>Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp

>+ mStartPos(LL_INIT(0xffffffff, 0xffffffff))

are you trying to avoid the overhead of calling LL_MaxUint?
i don't like hardcoding magic numbers like this. what if
you were to accidentally type only 7 f's? ;-)

>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp

>+ mFileSize = LL_INIT(0xffffffff, 0xffffffff);

more occurances of this guy.

>+ PR_sscanf(mResponseMsg.get() + 4, "%llu", &mFileSize);
>+ // XXX this so sucks
>+ PRUint32 size32;
>+ LL_L2UI(size32, mFileSize);
>+ if (NS_FAILED(mChannel->SetContentLength(size32))) return FTP_ERROR;

so create a new internal API for FTP... or is it the case that the
nsFTPChannel's mContentLength is only ever used with GetContentLength?

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+ PRUint64 size = LL_INIT(0xffffffff, 0xffffffff);

ding!

>Index: netwerk/protocol/http/src/nsHttpResponseHead.cpp

>+ mHeaders.SetHeader(nsHttp::Content_Length, nsPrintfCString("%lld", len));

hmm... the default buffer size for nsPrintfCString is not large
enough for 64-bit integers. perhaps we should increase the default
buffer size from 15 to 20 (not including trailing null byte).

ok, please come up with a better solution for LL_MAXUINT if you don't
want to simply use it.

it also seems like nsUint64 would be handy in some cases, no?

overall this patch looks really good... thanks for doing this biesi!!