901262 - Add a DownloadList method to remove stopped downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the JavaScript API for downloads, the DownloadList object should have a
method like removeStoppedDownloads, that is similar to the cleanUp method of
nsIDownloadManager.

Comment 1

[Neil Deakin]

Attachment: download-removeall

Add removeFinishedDownloads methods. I'm unclear which changes are desired for the conditional check in removeWhere: Currently it looks like:

         if ((download.succeeded || download.canceled || download.error) &&
             aTestFn(download)) {

Changing it to download.stopped && !download.hasPartialData causes the test_history_expiration test to fail because it expects to have a cancelled test removed from the list.

Comment 2

[:Paolo Amadini]

Comment on attachment 788924 [details] [diff] [review]
download-removeall

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

(In reply to Neil Deakin from comment #1)
> Changing it to download.stopped && !download.hasPartialData causes the
> test_history_expiration test to fail because it expects to have a cancelled
> test removed from the list.

I think we can change the test so that "let promiseCanceled = downloadTwo.cancel();" becomes "yield downloadTwo.cancel();".

One case I've been thinking about is the one of downloads that have partial data, but are failed rather than canceled. We show them as inactive, failed downloads in the user interface, even though they can be restarted from where they stopped.

History expiration and manual cleanup should remove those downloads (together with their partial data).

Serialization to disk, instead, should include those downloads so that we don't lose the reference to the ".part" file across sessions.

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +224,5 @@
>            // don't need to wait for the procedure to be complete before
>            // processing the other downloads in the list.
> +          if (aDontFinalize) {
> +            download.finalize(true);
> +          }

It's safer and simpler if we always call finalize - this does nothing unless it is required, but ensures that we don't leave unreferenced ".part" files (or other metadata in the future).

::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ +305,5 @@
> +  let removeNotifications = 0;
> +  let downloadView = {
> +    onDownloadRemoved: function (aDownload) {
> +      do_check_true(removeNotifications < 2);
> +      if (++removeNotifications == 2) {

I'd also check that aDownload is one of those that should be removed, for additional clarity.

Comment 3

[:Paolo Amadini]

Comment on attachment 788924 [details] [diff] [review]
download-removeall

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

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ -219,5 @@
>            // Ensure that the download is stopped and no partial data is kept.
>            // This works even if the download state has changed meanwhile.  We
>            // don't need to wait for the procedure to be complete before
>            // processing the other downloads in the list.
> -          download.finalize(true);

I just noticed a pre-existing missing error reporting:

download.finalize(true).then(null, Cu.reportError);

Comment 4

[Neil Deakin]

Attachment: download-removeall, v2

Comment 5

[:Paolo Amadini]

Comment on attachment 790180 [details] [diff] [review]
download-removeall, v2

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

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +200,5 @@
>    },
>  
>    /**
> +   * Removes downloads that have finished and have not been canceled from
> +   * the list, based on the given test function.

Add paragraph: "This method finalizes each removed download, ensuring that any partially downloaded data associated with it is also removed."

(or something to that effect)

@@ +205,3 @@
>     *
> +   * @param aFilterFn
> +   *        The filter function is called for each download and should return

"for each download" => "with each download as its only argument,"

(or something to that effect)

@@ +205,5 @@
>     *
> +   * @param aFilterFn
> +   *        The filter function is called for each download and should return
> +   *        true to remove the download and false to keep it. aFilterFn may be
> +   *        null to have no filter, in which can all finished downloads are

null => omitted?

@@ +211,2 @@
>     */
> +  removeWhere: function DL_removeWhere(aFilterFn) {

I still have a hard time figuring out a good name for this API, that also finalizes the downloads and has an inner filter.

clearUnneededWhere, removeStoppedWhere, removeFinishedWhere, removeAndFinalizeInactiveWhere, ...

If the aFilterFn argument can be undefined, maybe we can omit "Where":

clearUnneeded, removeStopped, removeFinished, removeAndFinalizeInactive, ...

@@ +215,5 @@
>        for (let download of list) {
>          // Remove downloads that have been canceled, even if the cancellation
>          // operation hasn't completed yet so we don't check "stopped" here.
> +        if (download.stopped && !download.hasPartialData &&
> +            (!aFilterFn || aFilterFn(download))) {

I think this should also remove failed downloads for which we have
partial data, something like:

"download.stopped && (!download.hasPartialData || download.error) && ..."

@@ +243,2 @@
>                                    download.startTime <= aEndTime);
>    },

I think removeByTimeframe can be removed and easily implemented as a filter function in the history expiration code.

@@ +256,5 @@
>                                                        download.source.url)));
>    },
>  
>    onClearHistory: function DL_onClearHistory() {
> +    this.removeWhere(null);

Just call without the argument.

::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ +226,5 @@
>    let deferred = Promise.defer();
>    let removeNotifications = 0;
>    let downloadView = {
>      onDownloadRemoved: function (aDownload) {
> +      if (++removeNotifications == 1) {

Shouldn't we still receive two notifications here?

@@ +304,5 @@
> +  let deferred = Promise.defer();
> +  let removeNotifications = 0;
> +  let downloadView = {
> +    onDownloadRemoved: function (aDownload) {
> +      do_check_true(aDownload == downloadOne  || aDownload == downloadTwo || aDownload == downloadThree);

indentation nit:

do_check_true(aDownload == downloadOne || aDownload == downloadTwo ||
              aDownload == downloadThree);

@@ +315,5 @@
> +  list.addView(downloadView);
> +
> +  // Start three of the downloads, but don't start downloadTwo, then set
> +  // downloadFour to have partial data. Only downloadOne and downloadThree
> +  // should be removed.

Update comment

Comment 6

[Neil Deakin]

Attachment: download-removeall, v3

Comment 7

[Neil Deakin]

I called the method removeFinished. Do you think it should be changed to return/yield the task rather than return nothing?

Comment 8

[:Paolo Amadini]

Comment on attachment 791296 [details] [diff] [review]
download-removeall, v3

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

(In reply to Neil Deakin from comment #7)
> I called the method removeFinished. Do you think it should be changed to
> return/yield the task rather than return nothing?

We run the deletions in parallel, and the task finishes before they are completed. So, I don't think it's useful to have a reference to the task's promise for the caller.

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +200,5 @@
>    },
>  
>    /**
> +   * Removes downloads from the list that have finished, have not been canceled,
> +   * or have failed with partial data, based on the given test function. This

"Removes downloads from the list that have finished, have failed, or have been canceled without keeping partial data.  A filter function may be specified to remove only a subset of those downloads."

@@ +208,5 @@
> +   * @param aFilterFn
> +   *        The filter function is called with each download as its only
> +   *        argument, and should return true to remove the download and false
> +   *        to keep it. aFilterFn may be null or omitted to have no filter, in
> +   *        which can all finished downloads are removed.

I'd just say "This parameter may be null or omitted to have no additional filter."

::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ +328,5 @@
> +
> +  let downloads = yield list.getAll()
> +  do_check_eq(downloads.length, 1);
> +
> +  yield deferred.promise;

Should wait on this promise before getting the list of downloads.

Comment 9

[:Paolo Amadini]

Landed with the suggested documentation and test changes:

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

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd7da708551f

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

I used the latest 26.0a1 builds across main platforms (Ubuntu 13.04, Win 7 and Mac 10.7.5) to verify that there is a "Clear List" option in the Downloads panel and "Clear Downloads" Library and these options remove all the finished&stopped downloads from the list.
Markin the bug Verified.