836437 - Add the ability to resume a download from where it stopped

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The "cancel" method accepts an aOptions argument, where the keepPartFile option
can be used to determine if the download will restart from the point where it
currently stopped, if supported by the saver object. The "target.partFile"
property keeps a reference to the partially downloaded file.

If the partially downloaded file exists, it will be considered when the "start"
method is called, if the source supports resuming.

Comment 1

[:Paolo Amadini]

If the source does not support resuming from partially downloaded files, then the
file should not be kept, even if requested.

Comment 2

[Marcos Aruj]

Hi guys.

Can I get more info on what part of the code is involved and how to implement this?

Thanks for your help.

Marcos.

Comment 3

[:Paolo Amadini]

(In reply to Marcos Aruj from comment #2)
> Can I get more info on what part of the code is involved and how to
> implement this?

Hello Marcos! This bug is about adding off-main-thread resuming support to the
JavaScript API for downloads. It can be divided in two main parts.

The first involves adding resuming support to the BackgroundFileSaver component.
The component is written in C++, you can see its interface defined in
"BackgroundFileSaver.idl" and the implementation in "BackgroundFileSaver.cpp".

The second part, to be done after the first one, involves changes to the code in
the "toolkit/components/jsdownloads" folder, which is not built by default now,
but will be available in Nightly in a few days from now. There is still some time
to be spent to better understand how resuming would work in the new API, before
getting to coding it.

The C++ part, however, can already be compiled and if you can run tests from the
"test_backgroundfilesaver.js" file, you have what you need to move forward on
implementing that part. If you're interested, let me know and I can file a new
dependent bug to let you get started there.

Alternatively, there are some other JavaScript bugs that may be worked on in
the new API, and require less time to complete. I can point you to a few of
them if you're interested.

Comment 4

[Marcos Aruj]

Thanks Paolo for the info. I think I would then prefer the alternative bugs you mention if that's OK. :) Please point me to them. 

Kind regards.

Comment 5

[:Paolo Amadini]

Attachment: Interface definition

Supporting partial download data in a fully asynchronous environment where we
may have different views controlling the same download is not easy, but I
think this interface is simple enough and prevents race conditions. To do
this, I had to write an interface that is slightly different from comment 0.

Please take a look at this interface, and let me know if you see any possible
race conditions or a use case that is not covered.

This interface supports the current pause/resume logic, but is more clear
about what "pausing" means (in fact, it does not suspend the request, it just
cancels the request and keeps any partial data). So, if hasPartial is true,
it means the download can be "paused": you can call cancel() and start() to
actually pause and resume it.

To completely stop a download, you can call cancel() and then removePartial()
immediately. But, probably, we don't want to do that, because the stop button
should still preserve partial data where possible, until the download is
removed from the list. (Once stopping does the right thing automatically,
we'll unify the concepts in the user interface also.)

On shutdown, we will just cancel() all the downloads, and this will do the
right thing, keeping the partial data if possible, to use when the download
is restarted when the browser is reopened.

Comment 6

[Neil Deakin]

Comment on attachment 774667 [details] [diff] [review]
Interface definition

+   *
+   * To have any effect, this property must be set before starting the download.
+   * Setting this property to false after the download already started will not
+   * remove any partial data.
+   *

after the download *has* already started


>+   * @return {Promise}
>+   * @resolves When the partial data has been successfully removed.
>+   * @rejects JavaScript exception if the operation could not be completed.
>+   */
>+  removePartial: function ()
>+  {
>+    return Promise.resolve();
>+  },

So if someone wants to restart a download fresh, do they need to call this method first?

>+  /**
>+   * Prevents the download from starting again after having been stopped.  This
>+   * method does not cancel the download in case it has already started.
>+   *
>+   * This method is used while shutting down or disposing of the download
>+   * object, to prevent other callers to interfere with the operation.  This is
>+   * required because cancellation and other operations are asynchronous.
>+   */
>+  preventStart: function ()
>+  {

This name is a bit confusing. If it is intended for cleanup, it is private to the download manager implementation or would other callers use it?


>           this.remove(download);
>+          // Since the download state may have changed meanwhile, we fully
>+          // execute the procedure that ensures that the download is stopped and
>+          // no partial data is kept.  We don't need to wait for the procedure
>+          // to be complete before processing the other downloads in the list.
>+          download.preventStart();
>+          download.cancel();
>+          download.removePartial();

Could we combine these into a single call that just stops and cleans up?

Comment 7

[:Paolo Amadini]

(In reply to Neil Deakin from comment #6)
> >+  removePartial: function ()
> >+  {
> >+    return Promise.resolve();
> >+  },
> 
> So if someone wants to restart a download fresh, do they need to call this
> method first?

Yes, if they want to restart the same download with the same target file,
but without using any previously stored partial data.

> >+  preventStart: function ()
> >+  {
> 
> This name is a bit confusing. If it is intended for cleanup, it is private
> to the download manager implementation or would other callers use it?
> 
> >+          download.preventStart();
> >+          download.cancel();
> >+          download.removePartial();
> 
> Could we combine these into a single call that just stops and cleans up?

Since the two cases when preventStart is currently used are before throwing
away a download and before shutting down the browser, maybe we might define
a download.shutdown(bool removePartial) method that calls preventStart plus
"cancel", and optionally calls removePartial, returning a promise resolved
when the operation is completed. This would replace the need for a public
preventStart method. What do you think?

Comment 8

[Neil Deakin]

> Since the two cases when preventStart is currently used are before throwing
> away a download and before shutting down the browser, maybe we might define
> a download.shutdown(bool removePartial) method that calls preventStart plus
> "cancel", and optionally calls removePartial, returning a promise resolved
> when the operation is completed. This would replace the need for a public
> preventStart method. What do you think?

preventStart isn't a clear api name (nor is removePartial) so I think that would be better.

I probably wouldn't call it 'shutdown' though as it doesn't have to be called at shutdown. Maybe 'reset' or 'cleanup'.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

what about just download.finalize()?

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 774667 [details] [diff] [review]
Interface definition

Review of attachment 774667 [details] [diff] [review]:
-----------------------------------------------------------------

I think covered pretty well the request, just a couple notes (and my suggested API name is in previous commment)

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +417,5 @@
> +   * download failed or has been canceled, using the removePartial method.  If
> +   * this property is still true, partial data will be retained during the next
> +   * download attempt.
> +   */
> +  tryKeepPartial: true,

I see you used this in SimpleDownload, but do we have a real use case for it?
What's the downside if SimpleDownload would just not care and we'd not expose this option?

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +221,5 @@
> +          // no partial data is kept.  We don't need to wait for the procedure
> +          // to be complete before processing the other downloads in the list.
> +          download.preventStart();
> +          download.cancel();
> +          download.removePartial();

do we already handle PB somewhere? cause there we should remove the partial.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

that was "I think Neil covered pretty well the request..."

Comment 12

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #10)
> > +  tryKeepPartial: true,
> 
> I see you used this in SimpleDownload, but do we have a real use case for it?
> What's the downside if SimpleDownload would just not care and we'd not
> expose this option?

Consumers of simpleDownload would leak partial files on disk on failure.

Setting tryKeepPartial to false is also useful for those cases where we want to
handle a full download with progress report (that is, not a simpleDownload) but
we just discard the result on failure (for example, this would allow us to
move add-on XPI downloads off the main thread).

Actually, since tryKeepPartial is only safe to use if the download is going to
be persisted across sessions or the caller invokes removePartial before
shutdown, I wonder if we should just have the default to false, and set it
to true only where we add the download to the list (DownloadLegacySaver and
panel front-end code).

> ::: toolkit/components/jsdownloads/src/DownloadList.jsm
> do we already handle PB somewhere? cause there we should remove the partial.

Yes, we should remove partial data when downloads are canceled when exiting
private browsing mode. That's covered by bug 836443.

Comment 13

[:Paolo Amadini]

(In reply to Neil Deakin from comment #8)
> preventStart isn't a clear api name (nor is removePartial)

Would removePartialData be clearer than removePartial?

(In reply to Marco Bonardo [:mak] from comment #9)
> what about just download.finalize()?

finalize looks good to me, with an aRemovePartialData argument.

Comment 14

[:Paolo Amadini]

Attachment: Part 1 of 2 - Unify DownloadLegacy tests in preparation for resuming support

This bug will make it possible to restart downloads previously started through
a DownloadLegacySaver. This makes the behavior of this saver interchangeable
with DownloadCopySaver.

I've thus refactored the tests so that they are the same for both components.
For the moment, I've disabled the restart tests, that will be enabled in a
separate patch on this bug.

Unfortunately, since with the legacy saver we cannot get a reference to the
Download object before starting the download, some tests have a slightly
different control flow between the two versions.

This patch contains a rename of test_DownloadCore.js to the new name
common_test_Download.js, with the modifications to include all the former
test_DownloadLegacy.js tests.

Comment 15

[:Paolo Amadini]

Attachment: Work in progress - Add the ability to resume to DownloadCopySaver.

This patch contains the updated interface definitions, as well as the
first implementation of resuming for DownloadCopySaver.  This will need
more tests for all the new methods in the interface, but there should be
enough material for a preliminary review.

Marco, I think I'll do the DownloadError constructor refactoring we talked
about, since it seems we have more cases where we need to set the properties
manually, and not based on the current constructor parameters.

Given the complexity of this patch alone, I think I'll add resuming support
for DownloadLegacySaver in a third, separate patch.

Comment 16

[:Paolo Amadini]

Attachment: Fix tests - Add the ability to resume to DownloadCopySaver.

With the refactoring, some tests now failed more consistently for the same
causes as the intermittent tests in bug 865364. This patch makes each request
independent from the others. This fixed the tests and will probably also have
the effect of resolving the intermittent failures.

Comment 17

[:Paolo Amadini]

Attachment: Work in progress - Add the ability to resume to DownloadLegacySaver.

This is how the third and last patch in the series would look like.

Comment 18

[Neil Deakin]

Comment on attachment 778498 [details] [diff] [review]
Part 1 of 2 - Unify DownloadLegacy tests in preparation for resuming support

>+  let persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>+                  .createInstance(Ci.nsIWebBrowserPersist);
>+
>+  // Apply decoding if required by the "Content-Encoding" header.
>+  persist.persistFlags &= ~Ci.nsIWebBrowserPersist.PERSIST_FLAGS_NO_CONVERSION;
>+
>+  // We must create the nsITransfer implementation using its class ID because
>+  // the "@mozilla.org/transfer;1" contract is currently implemented in
>+  // "toolkit/components/downloads".  When the other folder is not included in
>+  // builds anymore (bug 851471), we'll be able to use the contract ID.
>+  let transfer =
>+      Components.classesByID["{1b4c85df-cbdd-4bb6-b04e-613caece083c}"]
>+                .createInstance(Ci.nsITransfer);
>+
>+  if (aOptions) {
>+    aOptions.outPersist = persist;
>+  }

Move these outPersist lines above to where persist is set up.


>- * Executes a download, started by constructing the simplest Download object.
>+ * Executes a download and checks its basic properties after constructions.

'construction'


>-add_task(function test_download_construction()
>+add_task(function test_basic()
> {
>-  let targetPath = getTempFile(TEST_TARGET_FILE_NAME).path;
>+  let tempDirectory = FileUtils.getDir("TmpD", []);

tempDirectory is unused.


>+    // The download is already started, wait for completion and report errors.
>+    if (!download.stopped) {
>+      yield download.start();
>+    }

What is this condition checking for? If you want the test to verify that download.stopped must be false, the test should check that. Are there cases where download.stopped could be true here? Same with a number of other places where this condition appears.


>-add_task(function test_download_final_state_notified()
>+add_task(function test_final_state_notified()
> {
>-  let download = yield promiseSimpleDownload();
>+  let deferResponse = deferNextResponse();
>+
>+  let download = yield promiseStartDownload(httpUrl("interruptible.txt"));
> 
...
>-  // Starts the download and waits for completion.
>-  yield download.start();

I'm a bit confused here. Doesn't promiseStartDownload end up calling start() already?


It's a bit confusing that some of the tests use promiseStartDownload, while others check gUseLegacySaver and set things up separately. Some comments as to why this is needed is those cases would be helpful.

Comment 19

[Neil Deakin]

Comment on attachment 779715 [details] [diff] [review]
Fix tests - Add the ability to resume to DownloadCopySaver.

>+   * Indicates whether any partially downloaded data should be retained, to use
>+   * it when restarting a failed or canceled download.  The default is false.

remove 'it'


>+   * If this property is set to true, care should be taken that partial data is
>+   * removed before the reference to the download is discarded.  This can be
>+   * done using the removePartialData or the "finalize" methods.
>+   */
>+  tryKeepPartialData: false,

Maybe better as 'tryToKeepPartialData'


>   /**
>+   * String containing the path of the ".part" file containing the data
>+   * downloaded so far, or null to disable the use of a ".part" file to keep
>+   * partially downloaded data.
>+   */
>+  partFilePath: null,
>+
>+  /**

So to retain partial files, does one need to set both target.partFilePath and download.tryKeepPartialData? Seems like only one field should be needed to be set.


>+   *
>+   * This method never called until the promise returned by "execute" is either
>+   * resolved or rejected, and the "execute" method is not called again until
>+   * the promise returned by this method is resolved or rejected.

This method *is* never called...


>-      // Set the target file, that will be deleted if the download fails.
>-      backgroundFileSaver.setTarget(new FileUtils.File(download.target.path),
>-                                    false);

The old code sets the target here before everything starts, but the new code doesn't set it until onStartRequest is called. Is that ok?

>+      // Create a placeholder for the final target file immediately.  If the
>+      // file already exist, don't delete its contents yet.

file already *exists*, ...

Maybe explain why we need to create the file right away.


>+        if (this._canceled) {
>+          // Don't create the BackgroundFileSaver object if we have been
>+          // canceled meanwhile.
>+          throw new DownloadError(Cr.NS_ERROR_FAILURE, "Saver canceled.");
>+        }

Is canceling a download an error, or does this error not propagate?


>+  let deferCancel = Promise.defer();
>+  download.onchange = function () {
>+    if (!download.stopped && !download.canceled && download.progress == 50) {
>+      // After we receive the progress notification, we should wait some time
>+      // for the worker thread of BackgroundFileSaver to receive the data to
>+      // be written to disk.  We don't have control over when this happens.
>+      do_timeout(50, () => {
>+        deferCancel.resolve(download.cancel());
>+      });

This will fail if the test happens to take longer that 50ms, so it should probably iterate.


Is the promiseNextRequestReceived method still needed?


Not sure I like the names blockResponses and unblockResponses. These are used just to delay a response part way and start it again right? Perhaps 'mustPauseResponse' and 'continueResponse' would be clearer.

Comment 20

[Neil Deakin]

Comment on attachment 780284 [details] [diff] [review]
Work in progress - Add the ability to resume to DownloadLegacySaver.

Both DownloadCopySaver.removePartialData and DownloadLegacySaver.removePartialData are the same code. Could they be shared or just use an inherited implementation or will they differ?

Comment 21

[:Paolo Amadini]

Attachment: Part 1 of 3 - Unify DownloadLegacy tests in preparation for resuming support

(In reply to Neil Deakin from comment #18)
> >+    // The download is already started, wait for completion and report errors.
> >+    if (!download.stopped) {
> >+      yield download.start();
> >+    }
> 
> What is this condition checking for?
> 
> It's a bit confusing that some of the tests use promiseStartDownload, while
> others check gUseLegacySaver and set things up separately. Some comments as
> to why this is needed is those cases would be helpful.

To clarify the tests, I've added a promiseDownloadStopped function, renamed
promiseSimpleDownload to promiseNewDownload, added comments when we do things
differently for DownloadLegacySaver, and refactored some tests to include the
DownloadLegacySaver case in error state checks.

It should be much more readable overall, let me know if there is something else
than needs clarification.

Comment 22

[:Paolo Amadini]

Attachment: Part 2 of 3 - Add the ability to resume a download from where it stopped to DownloadCopySaver.

(In reply to Neil Deakin from comment #19)
> So to retain partial files, does one need to set both target.partFilePath
> and download.tryKeepPartialData? Seems like only one field should be needed
> to be set.

Currently, partFilePath may be set even if tryKeepPartialData is false, to
use a ".part" file instead of the final target while downloading. In case
tryKeepPartialData is false, the ".part" file is always deleted.

tryKeepPartialData instead controls whether partial data should be kept, for
all saver types. A DownloadDOMDocumentSaver might for example use this flag to
determine if the responses should be kept using the browser cache, without
actually needing the partFilePath to be set.

Currently, both DownloadCopySaver and DownloadLegacySaver also need partFilePath
to be set, to be able to keep partial data. We may make it so that if
tryKeepPartialData is true and partFilePath is null, a default value is used,
but then, since tryKeepPartialData may be set to false later, the
removePartialData function would always need to check if the file with the
default name exists and delete it, even if we didn't want to use a part file
in the first place. What do you think?

> The old code sets the target here before everything starts, but the new code
> doesn't set it until onStartRequest is called. Is that ok?

Yes, this should work correctly.

> >+        if (this._canceled) {
> >+          // Don't create the BackgroundFileSaver object if we have been
> >+          // canceled meanwhile.
> >+          throw new DownloadError(Cr.NS_ERROR_FAILURE, "Saver canceled.");
> >+        }
> 
> Is canceling a download an error, or does this error not propagate?

The Download object handles cancellation by requesting the DownloadSaver object
to terminate the "execute" method with an exception that is then ignored, so it
does not propagate.

> >+  let deferCancel = Promise.defer();
> >+  download.onchange = function () {
> >+    if (!download.stopped && !download.canceled && download.progress == 50) {
> >+      // After we receive the progress notification, we should wait some time
> >+      // for the worker thread of BackgroundFileSaver to receive the data to
> >+      // be written to disk.  We don't have control over when this happens.
> >+      do_timeout(50, () => {
> >+        deferCancel.resolve(download.cancel());
> >+      });
> 
> This will fail if the test happens to take longer that 50ms, so it should
> probably iterate.

I've implemented an iterative approach that should be more robust. I wasn't
able to test it as the actual code flow depends on timing, and it seems that
the simple call to OS.File.exists makes the timeout unnecessary, probably
because it posts events between threads.

> Not sure I like the names blockResponses and unblockResponses. These are
> used just to delay a response part way and start it again right? Perhaps
> 'mustPauseResponse' and 'continueResponse' would be clearer.

I've gone for mustInterruptResponses and continueResponses (plural since they
apply to all the requests at the same time).

Comment 23

[:Paolo Amadini]

(In reply to Neil Deakin from comment #20)
> Both DownloadCopySaver.removePartialData and
> DownloadLegacySaver.removePartialData are the same code. Could they be
> shared or just use an inherited implementation or will they differ?

They'll be the same, but the function won't apply to other saver types, so I've
just done "return DownloadCopySaver.prototype.removePartialData.call(this);".

Comment 24

[:Paolo Amadini]

Attachment: Part 3 of 3 - Add the ability to resume a download from where it stopped to DownloadLegacySaver.

Comment 25

[:Paolo Amadini]

Marco, let me know if you have any feedback on any of the three patches.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

sure, sorry I had some issues keeping up with requests this week.

Comment 27

[:Paolo Amadini]

Attachment: Part 2 of 3 - Add the ability to resume a download from where it stopped to DownloadCopySaver.

This patch adds the missing tests and should be ready for review.

Comment 28

[:Paolo Amadini]

Attachment: Part 3 of 3 - Add the ability to resume a download from where it stopped to DownloadLegacySaver.

This completes the implementation of resuming support in DownloadLegacySaver,
and adds tests for downloads started through nsIExternalHelperAppService.

Comment 29

[Neil Deakin]

Comment on attachment 780859 [details] [diff] [review]
Part 1 of 3 - Unify DownloadLegacy tests in preparation for resuming support

The comments here and the promiseDownloadStopped method make what's happening here much clearer. Thanks!

The only change is that the comment below, the last comma should be removed, or the sentence reworded:

+ // When testing DownloadCopySaver, we have control over the download, thus
+ // we can hook its onchange callback, that will be notified when the
+ // download starts.

And in the next comment:

+ // is created, and it may already have notified all its property changes
+ // until now, thus there 

I can't quite parse this line. Do you mean 'may have already made all needed property change notifications'?

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 781083 [details] [diff] [review]
Part 2 of 3 - Add the ability to resume a download from where it stopped to DownloadCopySaver.

Review of attachment 781083 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +417,5 @@
>      return this._promiseCanceled;
>    },
>  
>    /**
> +   * Indicates whether any partially downloaded data should be retained, to use

maybe "to be used"?

@@ +425,5 @@
> +   * download source, and may not be known before the download is started.
> +   *
> +   * To have any effect, this property must be set before starting the download.
> +   * Resetting this property to false after the download has already started
> +   * will not remove any partial data.

is that an expected use-case? otherwise you may replace the value with a getter once the download has started, to prevent setting it.

@@ +429,5 @@
> +   * will not remove any partial data.
> +   *
> +   * If this property is set to true, care should be taken that partial data is
> +   * removed before the reference to the download is discarded.  This can be
> +   * done using the removePartialData or the "finalize" methods.

either both should have apices, or none, there's nothing special about finalize.

@@ +444,5 @@
> +  /**
> +   * Removes any partial data kept as part of a canceled or failed download.
> +   *
> +   * If the download is not canceled or failed, this method has no effect, and
> +   * it returns a resolved promise.  If the "cancel" method was called but the

I wonder if that should be considered an error and rejected, like removing partial data of an in-progress download. Couldn't just resolving hide mistakes like that?

@@ +727,5 @@
>    } else if (aSerializable instanceof Ci.nsIURI) {
>      source.url = aSerializable.spec;
>    } else {
> +    // Convert String objects to primitive strings at this point.
> +    source.url = "" + aSerializable.url;

nit: it may be more polite to explicitly use .toString()

@@ +801,5 @@
>      target.path = aSerializable.path;
>    } else {
>      // Read the "path" property of the serializable DownloadTarget
> +    // representation, converting String objects to primitive strings.
> +    target.path = "" + aSerializable.path;

ditto

@@ +1008,5 @@
>     * Implements "DownloadSaver.execute".
>     */
>    execute: function DCS_execute(aSetProgressBytesFn)
>    {
> +    let self = this;

s/self/copySaver/ ?

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +96,5 @@
> + * saved to disk.  You must call "continueResponses" to allow the interruptible
> + * request to continue.
> + *
> + * TODO: This function uses either DownloadCopySaver or DownloadLegacySaver
> + * based on the current test run.

I'm not sure what's the todo part in that phrase.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I quickly skimmed through the patches and the approach and naming looks good to me, didn't go for a deep line-by-line review though.

Comment 32

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #30)
> > +   * Indicates whether any partially downloaded data should be retained, to use
> 
> maybe "to be used"?

I think the grammar is correct per comment 19, right?

> @@ +425,5 @@
> > +   * download source, and may not be known before the download is started.
> > +   *
> > +   * To have any effect, this property must be set before starting the download.
> > +   * Resetting this property to false after the download has already started
> > +   * will not remove any partial data.
> 
> is that an expected use-case? otherwise you may replace the value with a
> getter once the download has started, to prevent setting it.

This gives control on whether to keep partial data when restarting downloads
originally started from DownloadLegacySaver, we have a test case for that.

> @@ +429,5 @@
> > +   * will not remove any partial data.
> > +   *
> > +   * If this property is set to true, care should be taken that partial data is
> > +   * removed before the reference to the download is discarded.  This can be
> > +   * done using the removePartialData or the "finalize" methods.
> 
> either both should have apices, or none, there's nothing special about
> finalize.

Well, finalizeHasNoInterCaps, this is why I've added quotes to mark it clearly
as an identifier. When translating to formatted text, identifiers become
monospace, but in the text-only version I preferred not to use "quotes" or
|pipes| where avoidable without ambiguity.

> @@ +444,5 @@
> > +  /**
> > +   * Removes any partial data kept as part of a canceled or failed download.
> > +   *
> > +   * If the download is not canceled or failed, this method has no effect, and
> > +   * it returns a resolved promise.  If the "cancel" method was called but the
> 
> I wonder if that should be considered an error and rejected, like removing
> partial data of an in-progress download. Couldn't just resolving hide
> mistakes like that?

The idea is that the download might have been restarted by another listener
while we are processing a completion notification, and this shouldn't result
in a possibly intermittent error condition that is visible to the user. This
is the same logic used by other methods on the download, it isn't an error if
calls arrive in a different order.

As long as we keep a consistent internal state, and properly notify changes in
hasPartialData and other properties to the views, whether the download still
has or hasn't performed the requested action because of a conflicting command
coming from another listener should be secondary.

> @@ +727,5 @@
> >    } else if (aSerializable instanceof Ci.nsIURI) {
> >      source.url = aSerializable.spec;
> >    } else {
> > +    // Convert String objects to primitive strings at this point.
> > +    source.url = "" + aSerializable.url;
> 
> nit: it may be more polite to explicitly use .toString()

I'll update the code.

> ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
> @@ +96,5 @@
> > + * saved to disk.  You must call "continueResponses" to allow the interruptible
> > + * request to continue.
> > + *
> > + * TODO: This function uses either DownloadCopySaver or DownloadLegacySaver
> > + * based on the current test run.
> 
> I'm not sure what's the todo part in that phrase.

In fact, it's implemented in the following patch.

(In reply to Marco Bonardo [:mak] from comment #31)
> I quickly skimmed through the patches and the approach and naming looks good
> to me, didn't go for a deep line-by-line review though.

Thanks a lot for the feedback!

Comment 33

[:Paolo Amadini]

Made changes from comment 29 and landed part 1 of 3 to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/8c3ee4235ec1

Comment 34

[Dão Gottwald [:dao]]

(In reply to Paolo Amadini [:paolo] from comment #33)
> Made changes from comment 29 and landed part 1 of 3 to fx-team:
> 
> https://hg.mozilla.org/integration/fx-team/rev/8c3ee4235ec1

https://hg.mozilla.org/mozilla-central/rev/8c3ee4235ec1

Comment 35

[:Paolo Amadini]

While working on this, I found that bug 899102 is affecting the reliability of
these tests. I think we should try to land this patch despite the issue, so that
we can build upon it, without blocking on bug 899102.

So, I've added the following workaround after every cancellation in the legacy
nsExternalHelperAppService tests:

  // This time-based solution is a workaround to avoid intermittent failures,
  // and will be removed when bug 899102 is resolved.
  if (gUseLegacySaver) {
    yield promiseTimeout(250);
  }

Comment 36

[Neil Deakin]

Comment on attachment 781228 [details] [diff] [review]
Part 3 of 3 - Add the ability to resume a download from where it stopped to DownloadLegacySaver.

>+          throw new Components.Exception("Asynchronous version implemented.",
>+                                         Cr.NS_ERROR_NOT_AVAILABLE);

This is a confusing error message. It would make more sense to say 'Synchronous promptForSaveToFile not implemented.'

>+  let legacyDownload = yield promiseStartLegacyDownload();
>+  yield legacyDownload.cancel();
>+  listForSave.add(legacyDownload);

Why do you cancel this download first, but not the others?

Comment 37

[:Paolo Amadini]

(In reply to Neil Deakin from comment #36)
> >+  let legacyDownload = yield promiseStartLegacyDownload();
> >+  yield legacyDownload.cancel();
> >+  listForSave.add(legacyDownload);
> 
> Why do you cancel this download first, but not the others?

We cannot create legacy downloads without starting them, while when we use the
other functions that create normal downloads (Downloads.createDownload and
promiseNewDownload) we can just avoid to start them.

Comment 38

[:Paolo Amadini]

Attachment: Part 2 of 3 - Add the ability to resume a download from where it stopped to DownloadCopySaver.

This is the rebased version of the same patch.

Comment 39

[:Paolo Amadini]

Attachment: Part 3 of 3 - Add the ability to resume a download from where it stopped to DownloadLegacySaver.

I'm working on resolving some intermittent failures that appeared on Windows, I
hope this version of the patch will address them.

Comment 40

[:Paolo Amadini]

Intermittent failures disappeared with this new version of the patch:

https://tbpl.mozilla.org/?tree=Try&rev=a6f34758d89a

If any other intermittent failure appears, it should be filed as a separate bug.

Comment 41

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/5913eecc1447
https://hg.mozilla.org/integration/fx-team/rev/9b8995108081

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5913eecc1447
https://hg.mozilla.org/mozilla-central/rev/9b8995108081

Comment 43

[Mihai Morar, (:MihaiMorar)]

Marking as [qa-] since there are unit tests for both pushlogs.