291910 - Provide utility for incremental download

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P1)

Description

[Darin Fisher]

Provide utility for incremental download

This bug is about providing a utility class that can be used to incrementally
download a URL to a specified file location.  The URL is fetched one chunk at a
time using byte range requests periodically until it is entirely downloaded. 
The operation is completely resumable across application sessions.

This is designed to be used for software update to silently fetch security
patches in the background.  The interface I'm about to propose could probably
benefit from some improvements to make it more generic.

Comment 1

[Darin Fisher]

Attachment: v0 patch

Here's a first cut at an interface, implementation, and test executable.

The implementation starts by making a HEAD request to learn the content length
of the requested URL.  It then goes to sleep for the specified interval, waking
up to fetch the next byte range.  If repeats that until the entire file is
downloaded.  The listener object receives OnStartRequest, OnProgress, and
OnStopRequest notifications.

Considering the following use cases:

1) Application wants to silently fetch a large file.  Create an incremental
download object, configure a listener, and go.	Wait for OnStopRequest callback
to know when the file is fully downloaded.

2) Application wants to start actively downloading an existing "silent
download."  Cancel existing incremental download object.  Create a new one, set
the interval to 0, and then go to town.  (Current patch doesn't implement this
yet, but it should be trivial to add.)

3) Application wants to pause the silent download.  Cancel existing incremental
download object.

4) Application wants to resume the silent download.  Create a new silent
download object for the same URL.  The component will continue where it left
off.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... nsIIncrementalDownloadListener feels sorta redundant, seems like it could
be replaced with nsIRequestObserver combined with nsIProgressEventSink?
(although that'd be two pointers or a QI... hm...)

this sorta duplicates some logic provided via nsIResumableChannel - maybe that
interface should allow specifying a byte count?

base/public/nsIIncrementalDownload.idl
+  readonly attribute long long totalSize;

What if the server does not send a Content-Length header?

And, why not make the sizes unsigned long longs?


Couldn't you use the Content-Range header to determine the total size and avoid
the HEAD request?

Comment 3

[Darin Fisher]

> hm... nsIIncrementalDownloadListener feels sorta redundant, seems like it 
> could be replaced with nsIRequestObserver combined with nsIProgressEventSink?
> (although that'd be two pointers or a QI... hm...)

Yes, there is some similarity, but this interface exists for convenience to the
consumer.  For software update, the consumer only needs progress events when the
user has explicitly requested the download, so the QI approach could work.  It
just uglifies the interface.


> this sorta duplicates some logic provided via nsIResumableChannel - maybe that
> interface should allow specifying a byte count?

Yeah, I thought of that, but nsIIncrementalDownload does far more than resume
the channel.  It fetches chunks over a period of time until the entire document
is downloaded.  nsIResumableChannel doesn't describe that function.


> base/public/nsIIncrementalDownload.idl
> +  readonly attribute long long totalSize;
> 
> What if the server does not send a Content-Length header?

Then this utility fails.  It is designed to only work when a Content-Length
header is provided.


> And, why not make the sizes unsigned long longs?

Because I figured 2^63 was sufficient range, and I wanted to use -1 to mean
unspecified.  I could easily change that.


> Couldn't you use the Content-Range header to determine the total size and 
> avoid the HEAD request?

You mean make a byte range request for 0-0 bytes?  How do I know the size of the
first chunk if I don't know the entire size of the file?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #3)

> Yeah, I thought of that, but nsIIncrementalDownload does far more than resume
> the channel.  It fetches chunks over a period of time until the entire document
> is downloaded.  nsIResumableChannel doesn't describe that function.

Yes, but I meant, couldn't you make this component use such an extended
nsIResumableChannel interface?

> You mean make a byte range request for 0-0 bytes?  How do I know the size of the
> first chunk if I don't know the entire size of the file?

No, you could just start by requesting, say, 4096 bytes or something, no?
(Range: 0-4095 or something). If the file size is less than that, that's fine
too, since the server will limit it to the file size (RFC 2616 14.35.1)

Comment 5

[Darin Fisher]

> Yes, but I meant, couldn't you make this component use such an extended
> nsIResumableChannel interface?

Yes, that is possible.  I may leave that as a followup exercise since right now
the highest priority item is supporting HTTP.  Supporting other protocols is not
a requirement for software update, yet.


> No, you could just start by requesting, say, 4096 bytes or something, no?
> (Range: 0-4095 or something). If the file size is less than that, that's fine
> too, since the server will limit it to the file size (RFC 2616 14.35.1)

Ok, that's interesting.  I'll mess around with that to see how it works with the
Mozilla mirror network.

Comment 6

[Darin Fisher]

Attachment: v1 patch

This patch has the revised observer interface discussed.  The incremental
downloader QIs the observer for nsIProgressEventSink.  This version also
implements the interval=0 mode of operation (in which we just fetch the rest of
the file).

Comment 7

[Darin Fisher]

> > No, you could just start by requesting, say, 4096 bytes or something, no?
> > (Range: 0-4095 or something). If the file size is less than that, that's fine
> > too, since the server will limit it to the file size (RFC 2616 14.35.1)
> 
> Ok, that's interesting.  I'll mess around with that to see how it works with the
> Mozilla mirror network.

Yeah, HTTP/1.1 says this is a MUST, so I'll definitely use it (assuming the
mirror network is happy).  I'm glad to do away with HEAD requests since they
don't get preserved when following redirects, which is probably a bug in itself.

Comment 8

[Darin Fisher]

So, the challenge with not using HEAD is that I have to handle 416 error
responses due to requesting a range beyond the end of the resource.  That's OK I
guess.  At least, avoiding HEAD removes one request.

Comment 9

[Darin Fisher]

Attachment: v1.1 patch

This version no longer issues HEAD requests.

Christian, can you help me out with a code review?  I think you'll find the
code pretty straightforward.

I know you are interested in leveraging nsIResumableChannel, but at this point
there are some obstacles there besides just lacking support for specifying the
maximum length of data to fetch.  We also need a way to retrieve the 'total
entity size' from the channel, for which no generic API currently exists.  So,
I'm going to punt on that for now.  It can be done as a followup.

Comment 10

[u49640]

Do you plan to use this for AutoUpdates only, or also for general downloads? (i
think it wouldnt be such a bad idea to use this for downloading large files)

If so, would be possible to add a "resume from an other Server" option?

Comment 11

[Darin Fisher]

> Do you plan to use this for AutoUpdates only, or also for general downloads? (i
> think it wouldnt be such a bad idea to use this for downloading large files)
> 
> If so, would be possible to add a "resume from an other Server" option?

Right now, this is only planned to be used for software update.  I'm not sure I
understand the capability you are thinking about.  Do you have some user feature
in mind?

Comment 12

[u49640]

> Right now, this is only planned to be used for software update.  I'm not sure I
> understand the capability you are thinking about.  Do you have some user feature
> in mind?

Well, i'd like to have a cross session download manager that can resume the
download from another server (sometimes the original server goes down and i have
to resume from another one). 
Basicly i want a "Getright Light" included in Firefox and after reading what
your patch could do (i havent read the patch itself) i thought it could be a
general Download replacement instead of "just" a Patch downloader. (maybe i have
misunderstood this)

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

>Well, i'd like to have a cross session download manager that can resume the
>download

this interface is probably not the right choice for that. _that_ use case can
use nsIResumableChannel directly. and that would have the advantage that it
works for FTP downloads as well.

Comment 14

[Darin Fisher]

> this interface is probably not the right choice for that.

Agreed.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 181935 [details] [diff] [review]
v1.1 patch

+ * A incremental download object attempts to fetch a file piecemeal over

"An" not "A"


+  readonly attribute nsIURI finalURI;

Does anyone really care?


+  /**
+   * The file where the download is being written.
+   */
+  readonly attribute nsIFile destination;

So usually when downloading, we rename at the end of the successful download. 
In this way, a user doesn't try to execute the file before it has completely
downloaded.  Given that, what does destination mean -- is it a partial file
during the download?  


+  void begin(in nsIRequestObserver observer,
+	      in nsISupports ctxt);


begin isn't a great name.  start or run is better. imo


+// Error code used internally to the incremental downloader
+#define NS_ERROR_DOWNLOAD_COMPLETE \
+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GENERAL, 1)

Shouldn't this be a better defined error value?  How is a DOWNLOAD_COMPLETE be
an error?


+  rv = lf->OpenNSPRFileDesc(flags, 0644, &fd);

Why 0644?  Isn't this file just for the user?



Is "bytes=123-" a value range request?



+  , mChunkSize(4096 * 16)  // default value

#define and precalculate?


+  , mInterval(2)	    // default value

#define


+			       PRUint64(PRInt64(mCurrentSize)

Yuck.


Why does your onstoprequest have to conditionally call onstartrequest



Only got through about 1/2.  

What i didn't see was:

checking to see if the network was busy before doing the request.
any user preferences

Comment 16

[Darin Fisher]

Thanks for the feedback Doug!


> +  readonly attribute nsIURI finalURI;
> 
> Does anyone really care?

Well, I figured that someone might want to know in case the original URL was
redirected.  That way subsequent calls to this interface could initialize it
with the final URL instead of the original URL.  That would help avoid traffic
through the redirect over and over.


> +  readonly attribute nsIFile destination;
> 
> So usually when downloading, we rename at the end of the successful download. 
> In this way, a user doesn't try to execute the file before it has completely
> downloaded.  Given that, what does destination mean -- is it a partial file
> during the download?  

Right now it is a partial file during the download.  You make a good point, and
I should probably revise the code to add a suffix to this file that ensures that
it is not executable by double-clicking, and so that other code in Mozilla can
easily recognize it as a partial file.  I think the regular Firefox downloader
uses the .part suffix for this purpose, and I should probably do the same.


> +  void begin(in nsIRequestObserver observer,
> +	      in nsISupports ctxt);
> 
> 
> begin isn't a great name.  start or run is better. imo

I'm not partial to the name, so yeah... I think I like start.


> +// Error code used internally to the incremental downloader
> +#define NS_ERROR_DOWNLOAD_COMPLETE \
> +    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GENERAL, 1)
> 
> Shouldn't this be a better defined error value?  How is a DOWNLOAD_COMPLETE be
> an error?

Perhaps I just need a better comment.  This error is used internally to suppress
OnDataAvailable events.  In other words, I return this error code from my
OnStartRequest when I want to tell Necko to skip ODA.  But, I want to recognize
the error code in OnStopRequest, and then suppress it (since a complete download
is not an error).


> +  rv = lf->OpenNSPRFileDesc(flags, 0644, &fd);
> 
> Why 0644?  Isn't this file just for the user?

Yeah, it is... good call.


> Is "bytes=123-" a value range request?

Yup, see RFC 2616 section yadda yadda.  It means give me everything from byte
123 to the end of the file.  The HTTP byte range protocol is designed to
minimize the need for clients to know how big a file is apriori.


> +  , mChunkSize(4096 * 16)  // default value
> 
> #define and precalculate?

The compiler precalculates for me.  #define ... sure :)


> +			       PRUint64(PRInt64(mCurrentSize)
> 
> Yuck.

Yeah, this is a peculiarity of using nsInt64.  Do you have a better suggestion?


> Why does your onstoprequest have to conditionally call onstartrequest

Well, there would appear to be error cases in my OnStartRequest that would lead
to me not calling my observer's OnStartRequest.  So, I made it so that my
OnStopRequest would call my observer's OnStartRequest if that had not been done
already.  The nsIRequestObserver contract is that OnStartRequest must be paired
with and precede each OnStopRequest.


> What i didn't see was:
> 
> checking to see if the network was busy before doing the request.

Right.  This patch makes the simplifying assumption that it is enough to make
small, infrequent fetches.  There may be cases where that is a problem, but we
can code improvements to the scheduling as a followup patch.  This scheduling
algorithm is sufficient to get the ball rolling IMO.


> any user preferences

I decided not to code any preferences at this level.  The consumer will likely
have preferences that pertain to its application of this utility component.  The
fact that there are default values is perhaps not ideal.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

should this use nsISupportsPriority? or maybe it should implement that to allow
the user to set the priority? (or both? :) )


(I'll try to get to the review later today or tomorrow)

Comment 18

[Darin Fisher]

I thought about doing that.  However, we don't exactly have a socket level
queue.  HTTP only supports queuing on individual hosts.  So, I'm not sure that
it would be very useful without additional support at the socket level.  To
simply avoid consuming bandwidth we could leverage some information from the
socket transport service directly.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 181935 [details] [diff] [review]
v1.1 patch

base/public/nsIIncrementalDownload.idl
+   * The URI being fetched after any redirects have been followed.  This
+   * attribute is only set after OnStartRequest has been called on the
observer
+   * passed to the begin method.

hm, so it's not set during onstartrequest?

base/src/nsIncrementalDownload.cpp
+  nsCOMPtr<nsILocalFile> lf = do_QueryInterface(f, &rv);

Maybe the interface should take an nsILocalFile as the target, instead of an
nsIFile?

or, should Init fail if the object does not support nsILocalFile? We clearly
can't write to it in that case.

+  rangeSpec.AppendInt(PRInt64(size));

maybe we should add an AppendInt variant that takes a PRUint64...

+  NS_IF_ADDREF(*result = mURI);

Your error checking is inconsistent... GetName does not nullcheck the URI.

In Begin:
+  rv = StartTimer(0);

hm... why not call ProcessTimeout() directly?


You should set mIsPending to false at some point :)


+  NS_ASSERTION(!mChunk, "leaking memory chunk");

could use nsAutoArrayPtr, maybe...

+  if (strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
+    Cancel(NS_BINDING_ABORTED);

Can events still be posted at this point? What about timers? :)
(to ensure that OnStopRequest will be called at some point)

Comment 20

[Darin Fisher]

> +   * The URI being fetched after any redirects have been followed.  This
> +   * attribute is only set after OnStartRequest has been called on the
> observer
> +   * passed to the begin method.
> 
> hm, so it's not set during onstartrequest?

ok, bad documentation.  it is set before onstartrequest, but not "until"
onstartrequest.  make sense?  from a consumer point of view, they learn
about the final URI once they get onstartrequest.


> Maybe the interface should take an nsILocalFile as the target, instead of an
> nsIFile?

sounds reasonable.


> or, should Init fail if the object does not support nsILocalFile? We clearly
> can't write to it in that case.

maybe.


> +  rangeSpec.AppendInt(PRInt64(size));
> 
> maybe we should add an AppendInt variant that takes a PRUint64...

The parameter is a |const nsInt64&|, and so I needed to explicitly
case to PRInt64 in order to disambiguate things for the compiler.



> +  NS_IF_ADDREF(*result = mURI);
> 
> Your error checking is inconsistent... GetName does not nullcheck the URI.

Yeah, GetName probably should since we wouldn't want GetName to crash just
because Init failed or wasn't called.


> In Begin:
> +  rv = StartTimer(0);
> 
> hm... why not call ProcessTimeout() directly?

I don't want to call the observer's methods while inside Begin().  Doing so
places extra burden on the consumer.  Most of the necko asynchronous APIs try to
make this promise.  They always try to fire callbacks from PLEvents to ensure
that the stack is not too deep.


> You should set mIsPending to false at some point :)

Good catch.


> +  NS_ASSERTION(!mChunk, "leaking memory chunk");
> 
> could use nsAutoArrayPtr, maybe...

yeah... though we really shouldn't need to.



> +  if (strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
> +    Cancel(NS_BINDING_ABORTED);
> 
> Can events still be posted at this point? What about timers? :)
> (to ensure that OnStopRequest will be called at some point)

Yes, I believe events can still be posted at this point.  Of course, those event
handlers may have trouble posting events.  Can you think of a better solution?

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #20)
> > hm... why not call ProcessTimeout() directly?
> 
> I don't want to call the observer's methods while inside Begin().

Yes, that makes sense, but ProcessTimeout wouldn't do that anyway, would it?
It'd just open the channel...

(hm... unless people cancel the request before they call Begin(), I guess. is
that likely?)

> > could use nsAutoArrayPtr, maybe...
> 
> yeah... though we really shouldn't need to.

Yeah, feel free not to :)

> Yes, I believe events can still be posted at this point.  Of course, those event
> handlers may have trouble posting events.  Can you think of a better solution?

Maybe Observe() could call CallOnStopRequest directly? I think that should be safe.

Comment 22

[Darin Fisher]

(In reply to comment #21)
> (In reply to comment #20)
> > > hm... why not call ProcessTimeout() directly?
> > 
> > I don't want to call the observer's methods while inside Begin().
> 
> Yes, that makes sense, but ProcessTimeout wouldn't do that anyway, would it?
> It'd just open the channel...

Well, I didn't want to code ProcessTimeout with such concerns.  i wanted there
to be one code path leading to ProcessTimeout.  NOTE: the caller of
ProcessTimeout catches any errors and issues a Cancel accordingly.  I would have
to duplicate that code in Begin if I wanted to call ProcessTimeout directly.  I
guess I could move that code to a subroutine, but I just didn't much value in
doing so.  StartTimer(0) was an easy way to put things together.


> (hm... unless people cancel the request before they call Begin(), I guess. is
> that likely?)

We should make sure that case is handled sanely.


> Maybe Observe() could call CallOnStopRequest directly? I think that should be
safe.

Yeah, that sounds like a good idea.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

> wanted there
> to be one code path leading to ProcessTimeout.  NOTE: the caller of
> ProcessTimeout catches any errors and issues a Cancel accordingly.

ok, that makes sense, better to leave this as-is then.

Comment 24

[Darin Fisher]

Attachment: v1.2 patch

OK, here's a final patch with review comments from dougt and biesi applied. 
Boris, would you please give this a once over as well.	Thanks.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'll try to review this Sunday...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182228 [details] [diff] [review]
v1.2 patch

>Index: base/public/nsIIncrementalDownload.idl

>+interface nsIIncrementalDownload : nsIRequest

>+   * Initialize the incremental download object.  If the destination file
>+   * already exists, then only the remaining portion of the file will be
>+   * fetched.

There's no attempt to verify that what's already there matches what's on the
server at this point, right?  So in fact, what's on disk could already be
longer than what's on the server, in which case what will happen?

>+ It will create the file with the permissions 0600 if needed.

I know you did this in response to a review comment, but I think that'll break
on at least some Windows and Unix systems -- no one but the user will be able
to read the resulting downloaded file, even if the file is moved to a shared
directory or whatnot.  That's ok for software update, but not so great in
general....

Is there a reason not to just take the file creation permissions as an
argument?  Or default to 0666 (0644 if we're being paranoid) and let the umask
apply as needed?

What's the behavior here, in general, if the file changes while we're
downloading it?   Either on the server or on the client (the latter can be
deleted, say, or modified by the user, or whatever)?  Worth documenting.

>+  void init(in nsIURI uri, in nsIFile destination, in long chunkSize,
>+            in long intervalInSeconds);

What happens if init() is called on an in-progress downloader?	I assume we
throw ALREADY_INITIALIZED; is it worth documenting that?

>+   * The current number of bytes downloaded so far.

s/current//;

>  This attribute is set just
>+   * prior to calling OnStartRequest on the observer passed to the start
>+   * method.

Do we set it before making onProgress calls too?  If not, should we?  In any
case, document whether we do.

I see nothing in the interface indicating that it's HTTP-specific... Should
there be something about that here?  And in fact that it only works with HTTP
1.1 servers and only for entities that support byte-range requests (so eg for a
CGI it may fail with some servers)?  Might be nice to also document the exact
errors we'll throw, and where, for non-HTTP channels, HTTP not supporting
byte-range, etc.

>Index: base/src/nsIncrementalDownload.cpp

>+#define NS_ERROR_DOWNLOAD_COMPLETE \
>+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GENERAL, 1)

I guess this is safe because necko doesn't use NS_ERROR_MODULE_GENERAL and
GENERAL codes shouldn't be returned across module boundaries?

>+MakeRangeSpec(const nsInt64 &size, const nsInt64 &maxSize, PRInt32 chunkSize,

Document what |size| and |maxSize| are supposed to be here?

>+  nsInt64 end = size + nsInt64(chunkSize);
>+  if (maxSize != nsInt64(-1) && end > maxSize)
>+    end = maxSize;
>+  end -= 1;
>+
>+  rangeSpec.AppendInt(PRInt64(end));

So if maxSize is, say, 0, we end up with -1 for end.  Is that OK?

>+  PRInt32                        mChunkLen;
>+  PRInt32                        mChunkSize;
>+  PRInt32                        mInterval;
>+  nsInt64                        mTotalSize;
>+  nsInt64                        mCurrentSize;
>+  PRUint32                       mLoadFlags;
>+  nsresult                       mStatus;
>+  PRPackedBool                   mIsPending;
>+  PRPackedBool                   mDidOnStartRequest;

This might actually pack better if you move one of those 32-bit things to
before the 64-bit things, depending on whether nsInt64 gets aligned on 64-bit
boundaries...

Document what mChunkLen is?  And the units on mInterval?  And perhaps  that the
various data lengths are in bytes?

>+nsIncrementalDownload::ReadCurrentSize()
>+  nsInt64 size;
>+  nsresult rv = mDest->GetFileSize((PRInt64 *) &size);

Why not just use PRInt64 for |size|?  We can assign that to mCurrentSize
easily, and don't need to make assumptions about the internal structure of
nsInt64...  Please change this.

>+nsIncrementalDownload::Init(nsIURI *uri, nsIFile *dest,

>+  mDest = do_QueryInterface(dest);
>+  NS_ENSURE_ARG(mDest);

This wasn't documented in the api either.  It should be.

>+nsIncrementalDownload::OnStartRequest(nsIRequest *request,
>+    if (PR_sscanf(buf.get() + slash + 1, "%lld", (PRInt64 *) &mTotalSize) != 1)

Again, I'd really rather not make assumptions about internal structure of the
nsU?Int64 types... Use a PRInt64 temporary, please?

sr=bzbarsky with that addressed.

Comment 27

[Darin Fisher]

> There's no attempt to verify that what's already there matches what's on the
> server at this point, right?  So in fact, what's on disk could already be
> longer than what's on the server, in which case what will happen?

That's correct.  No use of ETags for that (mainly because the Mozilla mirror
network does not support ETags -- each server generates a different ETag for the
same file).  I could optionally support ETags in the implementation and expose a
way for the consumer to specify an ETag that should be validated.  That is
probably a good thing to do.


> >+ It will create the file with the permissions 0600 if needed.
> 
> I know you did this in response to a review comment, but I think that'll break
> on at least some Windows and Unix systems -- no one but the user will be able
> to read the resulting downloaded file, even if the file is moved to a shared
> directory or whatnot.  That's ok for software update, but not so great in
> general....
> 
> Is there a reason not to just take the file creation permissions as an
> argument?  Or default to 0666 (0644 if we're being paranoid) and let the umask
> apply as needed?
> 
> What's the behavior here, in general, if the file changes while we're
> downloading it?   Either on the server or on the client (the latter can be
> deleted, say, or modified by the user, or whatever)?  Worth documenting.

Did you see the comments in the IDL file?  Consumers of this interface may
create the nsIFile before passing it to this interface.  Otherwise, we default
to something that ensures privacy.  Better to error on the side of caution in
case the consumer does not think about file permissions.  BTW, if I included the
permission bits as a parameter, then I would have to decide whether or not to
try to apply those bits when the file already exists.  I'd rather leave the
management of that up to the consumer of this interface.


> >+  void init(in nsIURI uri, in nsIFile destination, in long chunkSize,
> >+            in long intervalInSeconds);
> 
> What happens if init() is called on an in-progress downloader?	I assume we
> throw ALREADY_INITIALIZED; is it worth documenting that?

Sure, I will document that.


> Do we set it before making onProgress calls too?  If not, should we?  In any
> case, document whether we do.

OnProgress calls occur between OnStartRequest and OnStopRequest.  Perhaps that
information was lost in the IDL when I made the switch from
nsIIncrementalDownloadObserver to nsIProgressEventSink.


> I see nothing in the interface indicating that it's HTTP-specific... Should
> there be something about that here?  And in fact that it only works with HTTP
> 1.1 servers and only for entities that support byte-range requests (so eg for a
> CGI it may fail with some servers)?  Might be nice to also document the exact
> errors we'll throw, and where, for non-HTTP channels, HTTP not supporting
> byte-range, etc.

Those would be implementation details related to the implementation of the
interface included in this patch.  I don't think those points should be
documented in the IDL.


> >Index: base/src/nsIncrementalDownload.cpp
> 
> >+#define NS_ERROR_DOWNLOAD_COMPLETE \
> >+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_GENERAL, 1)
> 
> I guess this is safe because necko doesn't use NS_ERROR_MODULE_GENERAL and
> GENERAL codes shouldn't be returned across module boundaries?

Correct.  That is my assumption.


> >+MakeRangeSpec(const nsInt64 &size, const nsInt64 &maxSize, PRInt32 chunkSize,
> 
> Document what |size| and |maxSize| are supposed to be here?
> 
> >+  nsInt64 end = size + nsInt64(chunkSize);
> >+  if (maxSize != nsInt64(-1) && end > maxSize)
> >+    end = maxSize;
> >+  end -= 1;
> >+
> >+  rangeSpec.AppendInt(PRInt64(end));
> 
> So if maxSize is, say, 0, we end up with -1 for end.  Is that OK?

If maxSize is 0, then we are it deep doodoo.  I'll add an assertion and/or
runtime check in the right place to deal with that.


> >+  PRInt32                        mChunkLen;
> >+  PRInt32                        mChunkSize;
> >+  PRInt32                        mInterval;
> >+  nsInt64                        mTotalSize;
> >+  nsInt64                        mCurrentSize;
> >+  PRUint32                       mLoadFlags;
> >+  nsresult                       mStatus;
> >+  PRPackedBool                   mIsPending;
> >+  PRPackedBool                   mDidOnStartRequest;
> 
> This might actually pack better if you move one of those 32-bit things to
> before the 64-bit things, depending on whether nsInt64 gets aligned on 64-bit
> boundaries...

Yes, probably...


> Document what mChunkLen is?  And the units on mInterval?  And perhaps that 
> the various data lengths are in bytes?

Sure


> >+nsIncrementalDownload::ReadCurrentSize()
> >+  nsInt64 size;
> >+  nsresult rv = mDest->GetFileSize((PRInt64 *) &size);
> 
> Why not just use PRInt64 for |size|?  We can assign that to mCurrentSize
> easily, and don't need to make assumptions about the internal structure of
> nsInt64...  Please change this.

This is probably based on some code that I used to have in this function that
made use of size being a nsInt64 to do some work.


> >+nsIncrementalDownload::Init(nsIURI *uri, nsIFile *dest,
> 
> >+  mDest = do_QueryInterface(dest);
> >+  NS_ENSURE_ARG(mDest);
> 
> This wasn't documented in the api either.  It should be.

Agreed.


> >+nsIncrementalDownload::OnStartRequest(nsIRequest *request,
> >+    if (PR_sscanf(buf.get() + slash + 1, "%lld", (PRInt64 *) &mTotalSize) != 1)
> 
> Again, I'd really rather not make assumptions about internal structure of the
> nsU?Int64 types... Use a PRInt64 temporary, please?
> 
> sr=bzbarsky with that addressed.

nsInt64 should be made more useful.  Perhaps I should just use the mValue
_public_ member variable directly.

Comment 28

[Darin Fisher]

Comment on attachment 182228 [details] [diff] [review]
v1.2 patch

This is needed for the new software update system.  It doesn't need to land for
1.1a

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 182228 [details] [diff] [review]
v1.2 patch

2xa=shaver.

Comment 30

[Darin Fisher]

fixed-on-trunk