899125 - Allow using the JavaScript API instead of nsIDownloadManager when clearing recent history

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The JavaScript API for downloads should be used when clearing history in a
given time range, or when clearing all history, ensuring that in-progress
downloads are removed in addition to stopped ones.

Comment 1

[Neil Deakin]

Attachment: Work in progress: download-sanitize

This patch converts the clear history dialog to use the new download manager. There are also some metro and android sanitizers which are not changed here.

Comment 2

[:Paolo Amadini]

I think we can return true in the canClear getter, I don't think it's relevant
anymore since we've unified the selection checkbox with the one for history in
the user interface.

Let me know when you want a more detailed review.

Comment 3

[:Paolo Amadini]

This should also be subject to the DownloadsCommon.useJSTransfer check.

Comment 4

[Neil Deakin]

Attachment: Part 1 - add a DownloadList.removeAll method

Comment 5

[Neil Deakin]

Attachment: Part 2 - convert sanitizer to new download manager

Comment 6

[Neil Deakin]

Attachment: Part 3 - convert forget about this site to new download manager

Comment 7

[Neil Deakin]

The 'forget about this site' needs to distinguish between active downloads and other downloads. Is download.stopped the best way to check this?

I added a removeAll to DownloadList. So you think there should be a way within DownloadList to remove active downloads as well?

Comment 8

[:Paolo Amadini]

Comment on attachment 788250 [details] [diff] [review]
Part 2 - convert sanitizer to new download manager

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

::: browser/base/content/sanitize.js
@@ +358,5 @@
> +
> +          // Remove any queued up active downloads
> +          dlsToRemove.forEach(function (dl) {
> +            dl.remove();
> +          });

The funny fact is that these "remove" calls do nothing but triggering an assertion:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1860

This means that, differently from what is specified in comment 0, we've never been removing in-progress and paused downloads.

Comment 9

[:Paolo Amadini]

Comment on attachment 788252 [details] [diff] [review]
Part 3 - convert forget about this site to new download manager

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

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +131,5 @@
> +          while (enumerator.hasMoreElements()) {
> +            let dl = enumerator.getNext().QueryInterface(Ci.nsIDownload);
> +            if (hasRootDomain(dl.source.host, aDomain)) {
> +              dl.cancel();
> +              dl.remove();

"Forget about this site" is removing active downloads in the correct way.

However, I think that we should be consistent between the two methods of clearing history, and don't touch active downloads. This is better because we don't cancel the downloads without alerting the user.

We can leave the old behavior as is, so that the change is linked to the activation of the new API, in other products also.

Comment 10

[:Paolo Amadini]

(In reply to Neil Deakin from comment #7)
> The 'forget about this site' needs to distinguish between active downloads
> and other downloads. Is download.stopped the best way to check this?

In _removeWhere, we're currently checking (download.succeeded ||
download.canceled || download.error), so that calling "cancel" and then one of
the remove methods would also remove the download that is pending cancellation.

However, also in the light of the work I'm doing in bug 836443 for determining
which downloads to save, I think we should just check "stopped", and update
_removeWhere consequently.

Since we currently consider that stopped downloads with partial data are "paused",
we should also avoid removing a download in case hasPartialData is set.

> I added a removeAll to DownloadList. So you think there should be a way
> within DownloadList to remove active downloads as well?

Given all the above, the removeAll method should not touch in-progress and paused
downloads. Part 1 thus becomes bug 901262.

With regard to naming, there are two differences between "remove" and
"removeAll", and I'm not sure how to surface those in the method name:
 - One finalizes the download, the other doesn't touch it.
 - One works only on downloads that are stopped and don't have partial data.

Comment 11

[Neil Deakin]

Paolo, with these patches running with the useJSTransfer preference enabled, some of the existing download manager and ui tests fail. See https://tbpl.mozilla.org/?tree=Try&rev=3eb2d4d22b99

Should I disable these or update these tests to work with the new download manager api?

Comment 12

[:Paolo Amadini]

(In reply to Neil Deakin from comment #11)
> Paolo, with these patches running with the useJSTransfer preference enabled,
> some of the existing download manager and ui tests fail. See
> https://tbpl.mozilla.org/?tree=Try&rev=3eb2d4d22b99
> 
> Should I disable these or update these tests to work with the new download
> manager api?

I'd like to handle all the test changes in bug 847863 when we switch the
preference, and don't touch the test suite at all for now. Can you separate
the tests and attach the test-only patch to the other bug?

The other history sanitize changes can be easily tested from the UI for now.

Comment 13

[Neil Deakin]

Attachment: Part 1.2 - convert sanitizer to new download manager

Comment 14

[Neil Deakin]

Attachment: Part 2.2 - convert forget about this site to new download manager

Comment 15

[:Paolo Amadini]

Comment on attachment 790214 [details] [diff] [review]
Part 1.2 - convert sanitizer to new download manager

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

::: browser/base/content/sanitize.js
@@ +322,5 @@
> +            else {
> +              publicList = yield publicList.removeWhere();
> +              privateList = yield privateList.removeWhere();
> +            }
> +          }.bind(this));

Task error reporting, unneeded assignments.

@@ +352,5 @@
> +            for (let dlsEnum of [dlMgr.activeDownloads, dlMgr.activePrivateDownloads]) {
> +              while (dlsEnum.hasMoreElements()) {
> +                dlsToRemove.push(dlsEnum.next());
> +              }
> +            }

We may just remove all this old code related to active downloads.

Comment 16

[:Paolo Amadini]

Comment on attachment 790218 [details] [diff] [review]
Part 2.2 - convert forget about this site to new download manager

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

Can you test the next version from the browser's user interface?

::: modules/libpref/src/init/all.js
@@ +76,5 @@
>  // cache compression turned off for now - see bug #715198
>  pref("browser.cache.compression_level", 0);
>  
> +// Enables the asynchronous Downloads API in the Downloads Panel.
> +pref("browser.download.useJSTransfer", false);

I don't think we want to have this about:config preference visible for products other than Firefox for Desktop at present, should just catch and assume false in case it's missing (may implement a lazy getter).

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +41,5 @@
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  this.ForgetAboutSite = {
> +  removeDataFromDomainAsync: function CRH_removeDataFromDomainAsync(aDomain)

I don't think this new function is needed, removeDataFromDomain is already expected to complete asynchronously in production code.

We'll update the tests to listen for download remove notification asynchronously.

@@ +49,5 @@
> +    // Downloads
> +    if (Services.prefs.getBoolPref("browser.download.useJSTransfer")) {
> +      for (let list of [Downloads.getPublicDownloadList(),
> +                        Downloads.getPrivateDownloadList()]) {
> +        list = yield list;

let promiseList of, list = yield promiseList

In any case, we should spawn our own task.

@@ +51,5 @@
> +      for (let list of [Downloads.getPublicDownloadList(),
> +                        Downloads.getPrivateDownloadList()]) {
> +        list = yield list;
> +        list.removeWhere(download =>
> +                         hasRootDomain(NetUtil.newURI(download.source.url).host, aDomain));

indentation nit: try to stay within 80 characters, for example:

list.removeWhere(download =>
                 hasRootDomain(NetUtil.newURI(download.source.url).host,
                               aDomain));

Comment 17

[Neil Deakin]

Attachment: Part 1.3 - convert sanitizer to new download manager

Comment 18

[Neil Deakin]

Attachment: Part 2.3 - convert forget about this site to new download manager

Comment 19

[:Paolo Amadini]

Comment on attachment 791302 [details] [diff] [review]
Part 1.3 - convert sanitizer to new download manager

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

::: browser/base/content/sanitize.js
@@ +326,5 @@
> +        else {
> +          var dlMgr = Components.classes["@mozilla.org/download-manager;1"]
> +                                .getService(Components.interfaces.nsIDownloadManager);
> +
> +          var dlsToRemove = [];

Unused variable.

Comment 20

[:Paolo Amadini]

Landed after removing the unused variable:

https://hg.mozilla.org/integration/fx-team/rev/16038b948077
https://hg.mozilla.org/integration/fx-team/rev/47ce79f94564

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/16038b948077
https://hg.mozilla.org/mozilla-central/rev/47ce79f94564

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130911030258

I verified that the completed and stopped downloads are removed from the Library/Downloads panel when history is cleared for the time they were performed, but in progress and paused downloads are not removed.
Marking the bug as VERIFIED.