diff --git a/browser/components/downloads/DownloadsCommon.jsm b/browser/components/downloads/DownloadsCommon.jsm index ed62470d3ffc..3c60e0e4e072 100644 --- a/browser/components/downloads/DownloadsCommon.jsm +++ b/browser/components/downloads/DownloadsCommon.jsm @@ -749,7 +749,7 @@ DownloadsDataCtor.prototype = { // This state transition code should actually be located in a Downloads // API module (bug 941009). - DownloadHistory.updateMetaData(download); + DownloadHistory.updateMetaData(download).catch(Cu.reportError); } if (download.succeeded || diff --git a/toolkit/components/downloads/DownloadCore.jsm b/toolkit/components/downloads/DownloadCore.jsm index 62e1acc772d0..35028e473e1e 100644 --- a/toolkit/components/downloads/DownloadCore.jsm +++ b/toolkit/components/downloads/DownloadCore.jsm @@ -23,22 +23,17 @@ var EXPORTED_SYMBOLS = [ ChromeUtils.import("resource://gre/modules/Integration.jsm"); ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); -ChromeUtils.defineModuleGetter(this, "FileUtils", - "resource://gre/modules/FileUtils.jsm"); -ChromeUtils.defineModuleGetter(this, "NetUtil", - "resource://gre/modules/NetUtil.jsm"); -ChromeUtils.defineModuleGetter(this, "OS", - "resource://gre/modules/osfile.jsm"); -ChromeUtils.defineModuleGetter(this, "PromiseUtils", - "resource://gre/modules/PromiseUtils.jsm"); -ChromeUtils.defineModuleGetter(this, "Services", - "resource://gre/modules/Services.jsm"); -ChromeUtils.defineModuleGetter(this, "PrivateBrowsingUtils", - "resource://gre/modules/PrivateBrowsingUtils.jsm"); +XPCOMUtils.defineLazyModuleGetters(this, { + AppConstants: "resource://gre/modules/AppConstants.jsm", + DownloadHistory: "resource://gre/modules/DownloadHistory.jsm", + FileUtils: "resource://gre/modules/FileUtils.jsm", + NetUtil: "resource://gre/modules/NetUtil.jsm", + OS: "resource://gre/modules/osfile.jsm", + PrivateBrowsingUtils: "resource://gre/modules/PrivateBrowsingUtils.jsm", + PromiseUtils: "resource://gre/modules/PromiseUtils.jsm", + Services: "resource://gre/modules/Services.jsm", +}); -XPCOMUtils.defineLazyServiceGetter(this, "gDownloadHistory", - "@mozilla.org/browser/download-history;1", - Ci.nsIDownloadHistory); XPCOMUtils.defineLazyServiceGetter(this, "gExternalAppLauncher", "@mozilla.org/uriloader/external-helper-app-service;1", Ci.nsPIExternalAppLauncher); @@ -1718,34 +1713,8 @@ this.DownloadSaver.prototype = { * the download is private. */ addToHistory() { - if (this.download.source.isPrivate) { - return; - } - - let sourceUri = NetUtil.newURI(this.download.source.url); - let referrer = this.download.source.referrer; - let referrerUri = referrer ? NetUtil.newURI(referrer) : null; - let targetUri = NetUtil.newURI(new FileUtils.File( - this.download.target.path)); - - // The start time is always available when we reach this point. - let startPRTime = this.download.startTime.getTime() * 1000; - - if ("@mozilla.org/browser/download-history;1" in Cc) { - try { - gDownloadHistory.addDownload(sourceUri, referrerUri, startPRTime, - targetUri); - } catch (ex) { - if (!(ex instanceof Components.Exception) || - ex.result != Cr.NS_ERROR_NOT_AVAILABLE) { - throw ex; - } - // - // Under normal operation the download history service may not - // be available. We don't want all downloads that are public to fail - // when this happens so we'll ignore this error and this error only! - // - } + if (AppConstants.MOZ_PLACES) { + DownloadHistory.addDownloadToHistory(this.download).catch(Cu.reportError); } }, diff --git a/toolkit/components/downloads/DownloadHistory.jsm b/toolkit/components/downloads/DownloadHistory.jsm index 590d05a3c9c6..32ae9c1d68ca 100644 --- a/toolkit/components/downloads/DownloadHistory.jsm +++ b/toolkit/components/downloads/DownloadHistory.jsm @@ -18,15 +18,15 @@ var EXPORTED_SYMBOLS = [ ]; ChromeUtils.import("resource://gre/modules/DownloadList.jsm"); -ChromeUtils.import("resource://gre/modules/Services.jsm"); ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); -ChromeUtils.defineModuleGetter(this, "Downloads", - "resource://gre/modules/Downloads.jsm"); -ChromeUtils.defineModuleGetter(this, "OS", - "resource://gre/modules/osfile.jsm"); -ChromeUtils.defineModuleGetter(this, "PlacesUtils", - "resource://gre/modules/PlacesUtils.jsm"); +XPCOMUtils.defineLazyModuleGetters(this, { + Downloads: "resource://gre/modules/Downloads.jsm", + FileUtils: "resource://gre/modules/FileUtils.jsm", + OS: "resource://gre/modules/osfile.jsm", + PlacesUtils: "resource://gre/modules/PlacesUtils.jsm", + Services: "resource://gre/modules/Services.jsm", +}); // Places query used to retrieve all history downloads for the related list. const HISTORY_PLACES_QUERY = @@ -86,6 +86,43 @@ var DownloadHistory = { */ _listPromises: {}, + async addDownloadToHistory(download) { + if (download.source.isPrivate || + !PlacesUtils.history.canAddURI(PlacesUtils.toURI(download.source.url))) { + return; + } + + let targetFile = new FileUtils.File(download.target.path); + let targetUri = Services.io.newFileURI(targetFile); + + let originalPageInfo = await PlacesUtils.history.fetch(download.source.url); + + let pageInfo = await PlacesUtils.history.insert({ + url: download.source.url, + // In case we are downloading a file that does not correspond to a web + // page for which the title is present, we populate the otherwise empty + // history title with the name of the destination file, to allow it to be + // visible and searchable in history results. + title: (originalPageInfo && originalPageInfo.title) || targetFile.leafName, + visits: [{ + // The start time is always available when we reach this point. + date: download.startTime, + transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD, + referrer: download.source.referrer, + }] + }); + + await PlacesUtils.history.update({ + annotations: new Map([["downloads/destinationFileURI", targetUri.spec]]), + // XXX Bug 1479445: We shouldn't have to supply both guid and url here, + // but currently we do. + guid: pageInfo.guid, + url: pageInfo.url, + }); + + await this._updateHistoryListData(download.source.url); + }, + /** * Stores new detailed metadata for the given download in history. This is * normally called after a download finishes, fails, or is canceled. @@ -97,7 +134,7 @@ var DownloadHistory = { * Download object whose metadata should be updated. If the object * represents a private download, the call has no effect. */ - updateMetaData(download) { + async updateMetaData(download) { if (download.source.isPrivate || !download.stopped) { return; } @@ -127,16 +164,24 @@ var DownloadHistory = { } try { - PlacesUtils.annotations.setPageAnnotation( - Services.io.newURI(download.source.url), - METADATA_ANNO, - JSON.stringify(metaData), 0, - PlacesUtils.annotations.EXPIRE_NEVER); + await PlacesUtils.history.update({ + annotations: new Map([[METADATA_ANNO, JSON.stringify(metaData)]]), + url: download.source.url, + }); + + await this._updateHistoryListData(download.source.url); } catch (ex) { Cu.reportError(ex); } }, + async _updateHistoryListData(sourceUrl) { + for (let key of Object.getOwnPropertyNames(this._listPromises)) { + let downloadHistoryList = await this._listPromises[key]; + downloadHistoryList.updateForMetadataChange(sourceUrl); + } + }, + /** * Reads current metadata from Places annotations for the specified URI, and * returns an object with the format: @@ -448,7 +493,6 @@ this.DownloadHistoryList.prototype = { } if (this._result) { - PlacesUtils.annotations.removeObserver(this); this._result.removeObserver(this); this._result.root.containerOpen = false; } @@ -457,11 +501,33 @@ this.DownloadHistoryList.prototype = { if (this._result) { this._result.root.containerOpen = true; - PlacesUtils.annotations.addObserver(this); } }, _result: null, + /** + * Updates the download history item when the meta data or destination file + * changes. + * + * @param {String} sourceUrl The sourceUrl which was updated. + */ + updateForMetadataChange(sourceUrl) { + let slotsForUrl = this._slotsForUrl.get(sourceUrl); + if (!slotsForUrl) { + return; + } + + for (let slot of slotsForUrl) { + if (slot.sessionDownload) { + // The visible data doesn't change, so we don't have to notify views. + return; + } + slot.historyDownload.updateFromMetaData( + DownloadHistory.getPlacesMetaDataFor(sourceUrl)); + this._notifyAllViews("onDownloadChanged", slot.download); + } + }, + /** * Index of the first slot that contains a session download. This is equal to * the length of the list when there are no session downloads. @@ -609,35 +675,6 @@ this.DownloadHistoryList.prototype = { nodeURIChanged() {}, batching() {}, - // nsIAnnotationObserver - onPageAnnotationSet(page, name) { - // Annotations can only be added after a history node has been added, so we - // have to listen for changes to nodes we already added to the list. - if (name != DESTINATIONFILEURI_ANNO && name != METADATA_ANNO) { - return; - } - - let slotsForUrl = this._slotsForUrl.get(page.spec); - if (!slotsForUrl) { - return; - } - - for (let slot of slotsForUrl) { - if (slot.sessionDownload) { - // The visible data doesn't change, so we don't have to notify views. - return; - } - slot.historyDownload.updateFromMetaData( - DownloadHistory.getPlacesMetaDataFor(page.spec)); - this._notifyAllViews("onDownloadChanged", slot.download); - } - }, - - // nsIAnnotationObserver - onItemAnnotationSet() {}, - onPageAnnotationRemoved() {}, - onItemAnnotationRemoved() {}, - // DownloadList callback onDownloadAdded(download) { let url = download.source.url; diff --git a/toolkit/components/downloads/test/unit/common_test_Download.js b/toolkit/components/downloads/test/unit/common_test_Download.js index 0bfdd417259b..0e5438381c26 100644 --- a/toolkit/components/downloads/test/unit/common_test_Download.js +++ b/toolkit/components/downloads/test/unit/common_test_Download.js @@ -2405,13 +2405,16 @@ add_task(async function test_history() { let download = await promiseStartDownload(sourceUrl); // The history and annotation notifications should be received before the download completes. - let [time, transitionType] = await promiseVisit; + let [time, transitionType, lastKnownTitle] = await promiseVisit; await promiseAnnotation; + let expectedFile = new FileUtils.File(download.target.path); + Assert.equal(time, download.startTime.getTime()); Assert.equal(transitionType, Ci.nsINavHistoryService.TRANSITION_DOWNLOAD); + Assert.equal(lastKnownTitle, expectedFile.leafName); - let expectedFileURI = Services.io.newFileURI(new FileUtils.File(download.target.path)); + let expectedFileURI = Services.io.newFileURI(expectedFile); let destFileURI = PlacesUtils.annotations.getPageAnnotation( Services.io.newURI(sourceUrl), "downloads/destinationFileURI"); diff --git a/toolkit/components/downloads/test/unit/head.js b/toolkit/components/downloads/test/unit/head.js index 3685c8b50ca1..f17840c8266d 100644 --- a/toolkit/components/downloads/test/unit/head.js +++ b/toolkit/components/downloads/test/unit/head.js @@ -36,6 +36,8 @@ ChromeUtils.defineModuleGetter(this, "FileTestUtils", "resource://testing-common/FileTestUtils.jsm"); ChromeUtils.defineModuleGetter(this, "MockRegistrar", "resource://testing-common/MockRegistrar.jsm"); +ChromeUtils.defineModuleGetter(this, "TestUtils", + "resource://testing-common/TestUtils.jsm"); XPCOMUtils.defineLazyServiceGetter(this, "gExternalHelperAppService", "@mozilla.org/uriloader/external-helper-app-service;1", @@ -152,7 +154,7 @@ function promiseWaitForVisit(aUrl) { Assert.equal(event.type, "page-visited"); if (event.url == aUrl) { PlacesObservers.removeListener(["page-visited"], listener); - resolve([event.visitTime, event.transitionType]); + resolve([event.visitTime, event.transitionType, event.lastKnownTitle]); } } PlacesObservers.addListener(["page-visited"], listener); @@ -585,21 +587,10 @@ function isValidDate(aDate) { * because the addDownload method will add these to the database asynchronously. */ function waitForAnnotation(sourceUriSpec, annotationName) { - let sourceUri = Services.io.newURI(sourceUriSpec); - return new Promise(resolve => { - PlacesUtils.annotations.addObserver({ - onPageAnnotationSet(page, name) { - if (!page.equals(sourceUri) || name != annotationName) { - return; - } - PlacesUtils.annotations.removeObserver(this); - resolve(); - }, - onItemAnnotationSet() {}, - onPageAnnotationRemoved() {}, - onItemAnnotationRemoved() {}, - }); - }); + return TestUtils.waitForCondition(async () => { + let pageInfo = await PlacesUtils.history.fetch(sourceUriSpec, {includeAnnotations: true}); + return pageInfo.annotations.has(annotationName); + }, `Should have found annotation ${annotationName} for ${sourceUriSpec}`); } /**