In the jsdownloads API, the DownloadList object should provide methods to remove relevant downloads on manual history expiration.

This should include an nsINavHistoryObserver listener.

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?

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.

(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 :)

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.

(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.

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();"

(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

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

(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

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

I confirm the fix is verified on Mac OS 10.7.5 and Windows 7 x64 using FF 24. BuildID: 20130910160258