unfrozen necko interfaces should use 64-bit integers where appropriate

RESOLVED FIXED in mozilla1.8alpha6

Status

()

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

Trunk
mozilla1.8alpha6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8a6 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 2

14 years ago
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.
Depends on: 261083
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.
Target Milestone: --- → mozilla1.8alpha5
Created attachment 169321 [details] [diff] [review]
patch v1

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....
Attachment #163100 - Attachment is obsolete: true
Attachment #169321 - Flags: review?(darin)
Status: NEW → ASSIGNED
Target Milestone: mozilla1.8alpha5 → mozilla1.8alpha6
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...
Attachment #169321 - Flags: superreview?(bzbarsky)
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
Flags: blocking1.8a6+
brendan: thanks; feel free to do the sr instead of bz (assuming he doesn't mind)

Comment 11

14 years ago
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
Attachment #169321 - Attachment is obsolete: true
Attachment #169321 - Flags: superreview?(bzbarsky)
Attachment #169321 - Flags: review?(darin)
Attachment #169321 - Flags: review-
Created attachment 170196 [details] [diff] [review]
patch v2
Attachment #170196 - Flags: superreview?(bzbarsky)
Attachment #170196 - Flags: review?(darin)

Comment 14

14 years ago
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
Attachment #170196 - Flags: review?(darin) → review+
> 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.

Comment 16

14 years ago
> 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?

Comment 17

14 years ago
> 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.
Attachment #170196 - Flags: superreview?(bzbarsky) → superreview+
*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.
No longer blocks: 277662
Created attachment 170742 [details] [diff] [review]
patch v3

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.
Attachment #170196 - Attachment is obsolete: true
Attachment #170742 - Flags: approval1.8a6?

Comment 24

14 years ago
Comment on attachment 170742 [details] [diff] [review]
patch v3

a=asa for checkin to 1.8a6
Attachment #170742 - Flags: approval1.8a6? → approval1.8a6+
thanks for the reviews and approval!

checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.