Closed Bug 899125 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla26

People

(Reporter: Paolo, Assigned: enndeakin)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
Assignee: nobody → enndeakin
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.
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.
This should also be subject to the DownloadsCommon.useJSTransfer check.
Attachment #786391 - Attachment is obsolete: true
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?
Flags: needinfo?(paolo.mozmail)
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 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.
(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.
Flags: needinfo?(paolo.mozmail)
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?
(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.
Depends on: 901262
Attachment #788248 - Attachment is obsolete: true
Attachment #788250 - Attachment is obsolete: true
Attachment #788252 - Attachment is obsolete: true
Attachment #790214 - Flags: review?(paolo.mozmail)
Attachment #790218 - Flags: review?(paolo.mozmail)
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.
Attachment #790214 - Flags: review?(paolo.mozmail) → feedback+
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));
Attachment #790218 - Flags: review?(paolo.mozmail)
Attachment #791302 - Flags: review?(paolo.mozmail)
Attachment #790214 - Attachment is obsolete: true
Attachment #790218 - Attachment is obsolete: true
Attachment #791303 - Flags: review?(paolo.mozmail)
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.
Attachment #791302 - Flags: review?(paolo.mozmail) → review+
Attachment #791303 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/16038b948077
https://hg.mozilla.org/mozilla-central/rev/47ce79f94564
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 908048
No longer depends on: 908048
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.