Add methods to remove downloads on history expiration

VERIFIED FIXED in mozilla24

Status

()

Toolkit
Downloads API
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Paolo, Assigned: raymondlee)

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
In the jsdownloads API, the DownloadList object should provide methods to
remove relevant downloads on manual history expiration.
(Reporter)

Comment 1

5 years ago
This should include an nsINavHistoryObserver listener.
(Reporter)

Updated

5 years ago
Blocks: 881058
(Reporter)

Updated

5 years ago
No longer blocks: 825588
(Assignee)

Comment 2

5 years ago
Created attachment 762604 [details] [diff] [review]
WIP

Paolo: I have encountered a problem but couldn't find a way to fix that.  I have added history observer code and tried to add an observer.  

When I run make SOLO_FILE="test_Downloads" -C toolkit/components/jsdownloads/test/ check-one

+var historyService = Cc["@mozilla.org/browser/nav-history-service;1"].
+                       getService(Ci.nsINavHistoryService);
+historyService.addObserver(HistoryObserver, false);

The above code throws the following error
TEST-UNEXPECTED-FAIL | resource://gre/modules/commonjs/sdk/core/promise.js | Unexpected exception [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: resource://gre/modules/Downloads.jsm :: <TOP_LEVEL> :: line 286"  data: no], see following stack:
undefined


Any suggestions?
Flags: needinfo?(paolo.mozmail)
my guess is that you are initing the service too early, what you are doing there is initing the service when importing the module (this is generally a bad thing to do, fwiw) and maybe you are importing the module even before having a profile obtained through do_get_profile()?
Btw, please always use PlacesUtils helpers, and never initialize a service until you really need it.
(Assignee)

Comment 4

5 years ago
Created attachment 763433 [details] [diff] [review]
v1

(In reply to Marco Bonardo [:mak] from comment #3)
> my guess is that you are initing the service too early, what you are doing
> there is initing the service when importing the module (this is generally a
> bad thing to do, fwiw) and maybe you are importing the module even before
> having a profile obtained through do_get_profile()?
> Btw, please always use PlacesUtils helpers, and never initialize a service
> until you really need it.

Thanks Marco for your suggestion :)
Assignee: nobody → raymond
Attachment #762604 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #763433 - Flags: review?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
(Reporter)

Comment 5

5 years ago
Comment on attachment 763433 [details] [diff] [review]
v1

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

This is in the right direction, thanks!

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +41,5 @@
>   */
>  function DownloadList() {
>    this._downloads = [];
>    this._views = new Set();
> +  PlacesUtils.history.addObserver(this, false);

We should do this for the public download list only (we may pass an argument to the constructor).

In fact, currently we remove private downloads when a history entry with the same URI is deleted, but since we don't add history entries for private downloads, this is probably not what we want here.

@@ +190,5 @@
> +    Task.spawn(function() {
> +      let list = yield this.getAll();
> +      for (let download of list) {
> +        if (aURI.equals(download.source.uri)) {
> +          this.remove(download);

We shouldn't remove downloads that are in progress, otherwise the user wouldn't be able to cancel them.

The proper condition is "succeeded || canceled || error". We don't just check "stopped" because we want to remove downloads that have been canceled, even if the cancellation operation hasn't completed yet. Please annotate this in a comment here, and add a new test that calls "cancel", then deletes the history entry immediately.

@@ +193,5 @@
> +        if (aURI.equals(download.source.uri)) {
> +          this.remove(download);
> +        }
> +      }
> +    }.bind(this));

You may use the new "arrow function" construct here, that doesn't require "bind" to be called. You should also always report errors that may happen in tasks (when you don't pass the promise returned by Task.spawn to the caller):

Task.spawn(() => {
  // ...
}).then(null, Cu.reportError);

@@ +196,5 @@
> +      }
> +    }.bind(this));
> +  },
> +
> +  onClearHistory: function () {},

When onClearHistory is called, we should do the same as onDeleteURI, but for all downloads that are not in progress.

In addition, we should add a removeByTimeframe method that does the same, but checks the startTime of downloads that are not in progress (see RemoveDownloadsByTimeframe).

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +223,5 @@
> + *
> + * @return {Promise}
> + * @rejects JavaScript exception.
> + */
> +function promiseAddHistory(aSourceURI) {

promiseAddDownloadToHistory

@@ +231,5 @@
> +    {
> +      uri: aSourceURI || TEST_SOURCE_URI,
> +      visits: [{
> +        transitionType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
> +        visitDate:  now++

I think "now" is meant to be a global variable?

::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ +219,5 @@
> +  Services.prefs.setIntPref("places.history.expiration.max_pages", 0);
> +
> +  // Force a history expiration.
> +  let expire = Cc["@mozilla.org/places/expiration;1"].
> +                  getService(Ci.nsIObserver);

nit: per style guide, the dot should be at the beginning of the second line. Indentation:

let expire = Cc["@mozilla.org/places/expiration;1"]
               .getService(Ci.nsIObserver);

@@ +222,5 @@
> +  let expire = Cc["@mozilla.org/places/expiration;1"].
> +                  getService(Ci.nsIObserver);
> +  expire.observe(null, "places-debug-start-expiration", -1);
> +
> +  do_test_pending();

When you need to wait in test tasks, you should use Promise.defer() and wait on its promise, like we do in other tests here.
Attachment #763433 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 6

5 years ago
Created attachment 765214 [details] [diff] [review]
v2

(In reply to Paolo Amadini [:paolo] from comment #5)
> Comment on attachment 763433 [details] [diff] [review]
> v1
> 
> Review of attachment 763433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is in the right direction, thanks!
> 
> ::: toolkit/components/jsdownloads/src/DownloadList.jsm
> @@ +41,5 @@
> >   */
> >  function DownloadList() {
> >    this._downloads = [];
> >    this._views = new Set();
> > +  PlacesUtils.history.addObserver(this, false);
> 
> We should do this for the public download list only (we may pass an argument
> to the constructor).
> 
> In fact, currently we remove private downloads when a history entry with the
> same URI is deleted, but since we don't add history entries for private
> downloads, this is probably not what we want here.
> 

Updated.

> @@ +190,5 @@
> > +    Task.spawn(function() {
> > +      let list = yield this.getAll();
> > +      for (let download of list) {
> > +        if (aURI.equals(download.source.uri)) {
> > +          this.remove(download);
> 
> We shouldn't remove downloads that are in progress, otherwise the user
> wouldn't be able to cancel them.
> 
> The proper condition is "succeeded || canceled || error". We don't just
> check "stopped" because we want to remove downloads that have been canceled,
> even if the cancellation operation hasn't completed yet. Please annotate
> this in a comment here, and add a new test that calls "cancel", then deletes
> the history entry immediately.

Updated.

> 
> @@ +193,5 @@
> > +        if (aURI.equals(download.source.uri)) {
> > +          this.remove(download);
> > +        }
> > +      }
> > +    }.bind(this));
> 
> You may use the new "arrow function" construct here, that doesn't require
> "bind" to be called. You should also always report errors that may happen in
> tasks (when you don't pass the promise returned by Task.spawn to the caller):
> 
> Task.spawn(() => {
>   // ...
> }).then(null, Cu.reportError);
> 

arrow function can't contain yield.

> @@ +196,5 @@
> > +      }
> > +    }.bind(this));
> > +  },
> > +
> > +  onClearHistory: function () {},
> 
> When onClearHistory is called, we should do the same as onDeleteURI, but for
> all downloads that are not in progress.

Added.

> 
> In addition, we should add a removeByTimeframe method that does the same,
> but checks the startTime of downloads that are not in progress (see
> RemoveDownloadsByTimeframe).

Added

> 
> ::: toolkit/components/jsdownloads/test/unit/head.js
> @@ +223,5 @@
> > + *
> > + * @return {Promise}
> > + * @rejects JavaScript exception.
> > + */
> > +function promiseAddHistory(aSourceURI) {
> 
> promiseAddDownloadToHistory

Updated.

> 
> @@ +231,5 @@
> > +    {
> > +      uri: aSourceURI || TEST_SOURCE_URI,
> > +      visits: [{
> > +        transitionType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
> > +        visitDate:  now++
> 
> I think "now" is meant to be a global variable?

Updated.

> 
> ::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
> @@ +219,5 @@
> > +  Services.prefs.setIntPref("places.history.expiration.max_pages", 0);
> > +
> > +  // Force a history expiration.
> > +  let expire = Cc["@mozilla.org/places/expiration;1"].
> > +                  getService(Ci.nsIObserver);
> 
> nit: per style guide, the dot should be at the beginning of the second line.
> Indentation:
> 
> let expire = Cc["@mozilla.org/places/expiration;1"]
>                .getService(Ci.nsIObserver);
> 

Fixed

> @@ +222,5 @@
> > +  let expire = Cc["@mozilla.org/places/expiration;1"].
> > +                  getService(Ci.nsIObserver);
> > +  expire.observe(null, "places-debug-start-expiration", -1);
> > +
> > +  do_test_pending();
> 
> When you need to wait in test tasks, you should use Promise.defer() and wait
> on its promise, like we do in other tests here.

Updated.
Attachment #763433 - Attachment is obsolete: true
Attachment #765214 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 7

5 years ago
Comment on attachment 765214 [details] [diff] [review]
v2

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

Looks good, we may just simplify a few things and improve the test case.

::: toolkit/components/jsdownloads/src/DownloadList.jsm
@@ +46,4 @@
>    this._downloads = [];
>    this._views = new Set();
> +  if (aIsPublic) {
> +    PlacesUtils.history.addObserver(this, false);

Please add a comment specifying why we do this for the public list only.

@@ +197,5 @@
> +   *        The start time date object.
> +   * @param aEndTime
> +   *        The end time date object.
> +   */
> +  removeByTimeFrame: function DL_removeByTimeFrame(aStartTime, aEndTime) {

We should spell "Timeframe" with the lowercase "f" for consistency:

http://mxr.mozilla.org/mozilla-central/search?string=Timeframe

@@ +205,5 @@
> +        // Remove downloads that have been canceled, even if the cancellation
> +        // operation hasn't completed yet so we don't check "stopped" here.
> +        if (download.startTime >= aStartTime &&
> +            download.startTime <= aEndTime &&
> +            (download.succeeded || download.canceled || download.error)) {

For correctness, since startTime can be null if the download hasn't started yet, I'd move the test after the state check.

In fact, since we define almost the same task in three different places, I also think we should create an internal helper function _removeWhere(aTestFn), where we call aTestFn(download) after checking (download.succeeded || download.canceled || download.error).

So, our three functions might just become one-liners:

this._removeWhere(() => true);
this._removeWhere(download => aURI.equals(download.source.uri));
this._removeWhere(download => download.startTime >= aStartTime &&
                              download.startTime <= aEndTime);

@@ +212,5 @@
> +      }
> +    }.bind(this)).then(null, Cu.reportError);
> +  },
> +
> +  // nsINavHistoryObserver

Please use this module's conventions for interface implementation comments (including a separate nsISupports section before nsINavHistoryObserver):

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadLegacy.js#70

::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ +214,5 @@
> +    onDownloadRemoved: function (aDownload) {
> +      if (++removeNotifications == 2) {
> +        deferred.resolve(function() {
> +          do_test_finished(cleanup);
> +        });

You just need to call deferred.resolve() here, then cleanup() at the end of the test function (after "yield deferred.promise;").

@@ +245,5 @@
> +
> +  // Force a history expiration.
> +  let expire = Cc["@mozilla.org/places/expiration;1"]
> +                 .getService(Ci.nsIObserver);
> +  expire.observe(null, "places-debug-start-expiration", -1);

We may be fine with something simpler for downloadTwo:

- Call "downloadTwo.start()", without waiting with yield
- Do "let promiseCanceled = downloadTwo.cancel()"
- Immediately call "expire.observe(...)"
  (so that downloadTwo may still be in the cancellation phase)
- After "yield deferred.promise;", also "yield promiseCanceled;"
  (to ensure we complete the request before the next text)

@@ +270,5 @@
> +  let removeNotifications = 0;
> +  let downloadView = {
> +    onDownloadRemoved: function (aDownload) {
> +      if (++removeNotifications == 2) {
> +        deferred.resolve(do_test_finished);

Just "deferred.resolve();"
Attachment #765214 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 8

5 years ago
Created attachment 765537 [details] [diff] [review]
v3

(In reply to Paolo Amadini [:paolo] from comment #7)
> Comment on attachment 765214 [details] [diff] [review]
> v2
> 
> Review of attachment 765214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, we may just simplify a few things and improve the test case.
> 
> ::: toolkit/components/jsdownloads/src/DownloadList.jsm
> @@ +46,4 @@
> >    this._downloads = [];
> >    this._views = new Set();
> > +  if (aIsPublic) {
> > +    PlacesUtils.history.addObserver(this, false);
> 
> Please add a comment specifying why we do this for the public list only.
> 

Added

> @@ +197,5 @@
> > +   *        The start time date object.
> > +   * @param aEndTime
> > +   *        The end time date object.
> > +   */
> > +  removeByTimeFrame: function DL_removeByTimeFrame(aStartTime, aEndTime) {
> 
> We should spell "Timeframe" with the lowercase "f" for consistency:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=Timeframe

Updated

> 
> @@ +205,5 @@
> > +        // Remove downloads that have been canceled, even if the cancellation
> > +        // operation hasn't completed yet so we don't check "stopped" here.
> > +        if (download.startTime >= aStartTime &&
> > +            download.startTime <= aEndTime &&
> > +            (download.succeeded || download.canceled || download.error)) {
> 
> For correctness, since startTime can be null if the download hasn't started
> yet, I'd move the test after the state check.
> 
> In fact, since we define almost the same task in three different places, I
> also think we should create an internal helper function
> _removeWhere(aTestFn), where we call aTestFn(download) after checking
> (download.succeeded || download.canceled || download.error).
> 
> So, our three functions might just become one-liners:
> 
> this._removeWhere(() => true);
> this._removeWhere(download => aURI.equals(download.source.uri));
> this._removeWhere(download => download.startTime >= aStartTime &&
>                               download.startTime <= aEndTime);
Done

> 
> @@ +212,5 @@
> > +      }
> > +    }.bind(this)).then(null, Cu.reportError);
> > +  },
> > +
> > +  // nsINavHistoryObserver
> 
> Please use this module's conventions for interface implementation comments
> (including a separate nsISupports section before nsINavHistoryObserver):
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/
> src/DownloadLegacy.js#70
> 

Updated

> ::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
> @@ +214,5 @@
> > +    onDownloadRemoved: function (aDownload) {
> > +      if (++removeNotifications == 2) {
> > +        deferred.resolve(function() {
> > +          do_test_finished(cleanup);
> > +        });
> 
> You just need to call deferred.resolve() here, then cleanup() at the end of
> the test function (after "yield deferred.promise;").

Done

> 
> @@ +245,5 @@
> > +
> > +  // Force a history expiration.
> > +  let expire = Cc["@mozilla.org/places/expiration;1"]
> > +                 .getService(Ci.nsIObserver);
> > +  expire.observe(null, "places-debug-start-expiration", -1);
> 
> We may be fine with something simpler for downloadTwo:
> 
> - Call "downloadTwo.start()", without waiting with yield
> - Do "let promiseCanceled = downloadTwo.cancel()"
> - Immediately call "expire.observe(...)"
>   (so that downloadTwo may still be in the cancellation phase)
> - After "yield deferred.promise;", also "yield promiseCanceled;"
>   (to ensure we complete the request before the next text)
> 

Done

> @@ +270,5 @@
> > +  let removeNotifications = 0;
> > +  let downloadView = {
> > +    onDownloadRemoved: function (aDownload) {
> > +      if (++removeNotifications == 2) {
> > +        deferred.resolve(do_test_finished);
> 
> Just "deferred.resolve();"

Done
Attachment #765214 - Attachment is obsolete: true
Attachment #765537 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 9

5 years ago
Comment on attachment 765537 [details] [diff] [review]
v3

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

Looks good!

::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ +226,5 @@
> +  yield downloadOne.start();
> +
> +  // Start download two and then cancel it.
> +  downloadTwo.start()
> +  let promiseCanceled = downloadTwo.cancel()

note: missing semicolon
Attachment #765537 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 766005 [details] [diff] [review]
Patch for check-in

(In reply to Paolo Amadini [:paolo] from comment #9)
> Comment on attachment 765537 [details] [diff] [review]
> v3
> 
> Review of attachment 765537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: toolkit/components/jsdownloads/test/unit/test_DownloadList.js
> @@ +226,5 @@
> > +  yield downloadOne.start();
> > +
> > +  // Start download two and then cancel it.
> > +  downloadTwo.start()
> > +  let promiseCanceled = downloadTwo.cancel()
> 
> note: missing semicolon

Fixed
Attachment #765537 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=5b7b4791e768
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e0f29d574e4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
I confirm the fix is verified on Mac OS 10.7.5 and Windows 7 x64 using FF 24.
BuildID: 20130910160258
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.