content lengths, offsets, etc on nonfrozen interfaces should use 64bit integers, to support files > 2 gb / > 4 gb.

so, i'm a little bit worried about changing these interfaces. maybe it would be a good idea to post a question to n.p.m.embedding (cross post on .netlib, .xpcom, and maybe .seamonkey) to see if anyone is going to be broken by these changes. on one hand, i agree that better now than later... but, we could also just invent a whole new 64-bit API tree.

so the problem with making nsIProgressEventSink2 is that then each channel would have to have compat code w/ nsIProgressEventSink... some helper function could improve that, maybe a wrapper for the notificationcallbacks that autowraps, but still... on the other hand, I need a solution for nsIAuthPrompt anyway... ok... Maybe best would be to do the backwards compat stuff in the helper functions from bug 261083.

news://news.mozilla.org/<cldj2d$cti1@ripley.netscape.com>

ok, for the next version of the patch, I'll keep nsIInputStreamPump as-is and add nsIInputStreamPump2. since it is basically just an Init function, this is easy to do, and doesn't break current users of it.

updated to trunk. using nsIInputStreamPump2. sorry for the delay, it was caused by me not realizing that the problems my build had with gmail were due to a --disable-extensions configure argument instead of this patch :( I still hope to get this into 1.8a6....

Comment on attachment 169321 [details] [diff] [review] patch v1 still not giving up hope on a alpha6 checkin; requesting sr now in hopes of of getting it in time...

Oy. I'll try for 1.8a6, but no guarantees.... my review backlog is nasty. :(

biesi: don't sweat it, I can help get this in after the freeze next week; also, if I can free some time, maybe I can relieve bz of sr duty here? /be

brendan: thanks; feel free to do the sr instead of bz (assuming he doesn't mind)

I spoke with biesi about this patch over IRC. Given that no one responded to his query to the newsgroups, I think it should be fine to change nsIInputStreamPump instead of introducing nsIInputStreamPump2. After this patch lands, I think we should consider freezing nsIInputStreamPump and the corresponding necko component that implements the interface.

Comment on attachment 169321 [details] [diff] [review] patch v1 patch with that change coming up

Comment on attachment 170196 [details] [diff] [review] patch v2 >Index: netwerk/base/public/nsIStreamLoader.idl > /** > * Asynchronously loads a channel into a memory buffer. >+ * >+ * XXX define behaviour for sizes >4 GB > */ > [scriptable, uuid(31d37360-8e5a-11d3-93ad-00104ba0fd40)] > interface nsIStreamLoader : nsISupports This interface is to be avoided when the data could be large :-) >Index: netwerk/base/src/nsStreamTransportService.cpp >+ if (!!mOffset) { ... >+ if (mOffset != LL_MAXUINT) { Hmm... it almost seems like it'd be nice if nsInt64 had methods to test for zero, max, and min. >Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp >+ if (!mSuppliedEntityID.IsEmpty() || (mStartPos != LL_MAXUINT && mStartPos != nsUint64(0))) { hmm... what's the best way to compare a 64 bit value against zero? LL_IS_ZERO? nsUint64::operator!()? >+ restString.AppendInt(PRInt64(PRUint64(mStartPos)), 10); so much casting /sigh/ r=darin

> hmm... what's the best way to compare a 64 bit value against zero? I'd have added an |operator bool()| to nsInt64/nsUint64; but I am unsure if using bool is allowed in mozilla code these days... |operator PRBool()| does not work - I get told that such an operator already exists. and just testing if (some_nsint64) is ambiguous as-is.

> I'd have added an |operator bool()| to nsInt64/nsUint64; but I am unsure if > using bool is allowed in mozilla code these days... |operator PRBool()| does not > work - I get told that such an operator already exists. and just testing if > (some_nsint64) is ambiguous as-is. What about adding isZero, isMin, and isMax methods to nsTInt64?

> What about adding isZero, isMin, and isMax methods to nsTInt64? doesn't have to hold up this patch of course ;)

Comment on attachment 170196 [details] [diff] [review] patch v2 >Index: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp >+ // XXX this truncates to 32bit It'd be good to have a standard searchable string for comments like this one... perhaps "64-bit" as in the wycywig channel comment? Or something more distinctive? This applies to all comments along these lines in this patch. If we have something we can search on, we can probably add this to the "good first bug" list.... >+ if (mTotalCurrentProgress == nsInt64(0) && mTotalMaxProgress == nsInt64(0)) It'd really make sense to be able to compare nsU?Int64 with native 32-bit ints. File a followup bug on that, maybe? It should be possible to implement the !=, ==, >, <, <=, and >= operators for 32-bit ints on the nsInt64 classes.... In any case, wouldn't comparing to LL_ZERO be cleaner here? >Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp Changing this interface per your comments should be OK from the POV of JS callers. File a bug on this, please? Make sure darin, jst, and doron are cced? >Index: modules/libjar/nsJARChannel.cpp >+ mProgressSink->OnProgress(this, nsnull, nsUint64(offset + count), >+ nsUint64(mContentLength)); Weird indent. >Index: netwerk/base/src/nsInputStreamPump.cpp >@@ -268,13 +268,13 @@ nsInputStreamPump::AsyncRead(nsIStreamLi >- rv = sts->CreateInputTransport(mStream, mStreamOffset, mStreamLength, >+ rv = sts->CreateInputTransport(mStream, PRUint64(mStreamOffset), PRUint64(mStreamLength), > mCloseWhenDone, This looks very weird as a cast to PRInt64... ;) At least comment here that that's what you're doing? >Index: netwerk/base/src/nsStreamTransportService.cpp > nsInputStreamTransport::Read(char *buf, PRUint32 count, PRUint32 >+ if (!!mOffset) { This would be covered by implementing the == operator for ints that I mention above. >+ seekable->Seek(nsISeekableStream::NS_SEEK_SET, PRUint64(mOffset)); Again, please document that this is just a weird way to cast to a PRInt64.... > nsOutputStreamTransport::Write(const char *buf, PRUint32 count, PRUint32 *result) Same comments apply. >Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp >- NS_ASSERTION(mStartPos == PRUint32(-1) || mStartPos == 0, >+ NS_ASSERTION(mStartPos == LL_MAXUINT || mStartPos == nsUint64(0), > "Non-initial start position given to directory request"); >- if (!mSuppliedEntityID.IsEmpty() || (mStartPos != PRUint32(-1) && mStartPos != 0)) { >+ if (!mSuppliedEntityID.IsEmpty() || (mStartPos != LL_MAXUINT && mStartPos != nsUint64(0))) { This could use the == and != operators, again. Or LL_ZERO? >+ restString.AppendInt(PRInt64(PRUint64(mStartPos)), 10); Why do you need the second cast (to PRInt64)? Because otherwise the call is ambiguous with native 64-bit types? If so, comment on that here? >Index: uriloader/base/nsDocLoader.cpp >+ if ((oldMax < nsInt64(0)) && (mMaxSelfProgress < nsInt64(0))) { Again, maybe LL_ZERO, or just leave it for the operator followup... >+ if ((oldMax == nsInt64(0)) && (info->mCurrentProgress == nsInt64(0))) { And here? >+ if (individualProgress < nsInt64(0)) // if one of the elements doesn't know it's size And here. >+ if (mMaxSelfProgress >= nsInt64(0) && newMaxTotal >= nsInt64(0)) And here. >+ if (!info->mUploading && (nsInt64(0) == info->mCurrentProgress) && (nsInt64(0) == info->mMaxProgress)) { Same here. >Index: webshell/tests/viewer/nsBrowserWindow.cpp >+ if (nsUint64(0) != aProgressMax) { Same here. sr=bzbarsky with the nits picked and followup bugs filed.

*sigh*, ns*Int64 sucks. it is ambiguous to create an nsUint64 from an PRInt64. LL_ZERO is an PRInt64. hence, much of what you suggested would not work.

hm, actually, maybe that's not the problem. instead, the compiler considers converting the nsUint64 to various types (PRFloat64, PRInt32, etc) and use the build-in operator==.

(and I used some of the nsUint64(0) to have an nsUint64 object, for compilers where PRInt64 is a struct... although maybe we need not worry about those)

I'll make it so that a regex search for XXX.*64-bit will find all these comments. Do note that many of those are not good first bugs, since they are forced by the current nsIStreamListener API (people passing aOffset + aCount as progress). filed Bug 277662 about XMLHTTPRequest (nsIDOMLSProgressEvent), and Bug 277663 about the operators. I am not sure whether isMax() is actually needed - == LL_MAXUINT does work on an nsUint64, and matches == PR_UINT32_MAX for 32-bit integers. but if you want I can file such a bug too. isZero() can be covered in Bug 277663, I think. attaching updated patch in a second.

I would like to land this in 1.8alpha6 - it changes necko APIs in an incompatible way. the risk should be rather low, since this mainly changes some data types to 64-bit integers.

Comment on attachment 170742 [details] [diff] [review] patch v3 a=asa for checkin to 1.8a6

thanks for the reviews and approval! checked in.