From 931ed2e441190908d7f8df1cc6a699d09efe9cf6 Mon Sep 17 00:00:00 2001 From: Ursula Sarracini Date: Tue, 2 Oct 2018 11:23:05 -0400 Subject: [PATCH] Backport Bug 1493969 - Refactor the user interface update code of download element r=ursula (#4453) --- lib/DownloadsManager.jsm | 110 +++++++++++-------------- test/unit/lib/DownloadsManager.test.js | 42 ++++------ test/unit/unit-entry.js | 18 ++-- 3 files changed, 74 insertions(+), 96 deletions(-) diff --git a/lib/DownloadsManager.jsm b/lib/DownloadsManager.jsm index ae01b31f2..90443698f 100644 --- a/lib/DownloadsManager.jsm +++ b/lib/DownloadsManager.jsm @@ -4,36 +4,19 @@ XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]); const {actionTypes: at} = ChromeUtils.import("resource://activity-stream/common/Actions.jsm", {}); -ChromeUtils.defineModuleGetter(this, "DownloadsViewUI", - "resource:///modules/DownloadsViewUI.jsm"); -ChromeUtils.defineModuleGetter(this, "DownloadsCommon", - "resource:///modules/DownloadsCommon.jsm"); +XPCOMUtils.defineLazyModuleGetters(this, { + DownloadsCommon: "resource:///modules/DownloadsCommon.jsm", + DownloadsViewUI: "resource:///modules/DownloadsViewUI.jsm", + FileUtils: "resource://gre/modules/FileUtils.jsm", +}); const DOWNLOAD_CHANGED_DELAY_TIME = 1000; // time in ms to delay timer for downloads changed events -class DownloadElement extends DownloadsViewUI.DownloadElementShell { - constructor(download, browser) { - super(); - this._download = download; - this.element = browser; - this.element._shell = this; - } - - get download() { - return this._download; - } - - downloadsCmd_copyLocation() { - let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].getService(Ci.nsIClipboardHelper); - clipboard.copyString(this.download.source.url); - } -} - this.DownloadsManager = class DownloadsManager { constructor(store) { this._downloadData = null; this._store = null; - this._viewableDownloadItems = new Map(); + this._downloadItems = new Map(); this._downloadTimer = null; } @@ -43,17 +26,16 @@ this.DownloadsManager = class DownloadsManager { return timer; } - formatDownload(element) { - const downloadedItem = element.download; - const description = element.sizeStrings.stateLabel; + formatDownload(download) { return { - hostname: new URL(downloadedItem.source.url).hostname, - url: downloadedItem.source.url, - path: downloadedItem.target.path, - title: element.displayName, - description, - referrer: downloadedItem.source.referrer, - date_added: downloadedItem.endTime, + hostname: new URL(download.source.url).hostname, + url: download.source.url, + path: download.target.path, + title: DownloadsViewUI.getDisplayName(download), + description: DownloadsViewUI.getSizeWithUnits(download) || + DownloadsCommon.strings.sizeUnknown, + referrer: download.source.referrer, + date_added: download.endTime, }; } @@ -65,10 +47,8 @@ this.DownloadsManager = class DownloadsManager { } onDownloadAdded(download) { - const elem = new DownloadElement(download, this._browser); - const downloadedItem = elem.download; - if (!this._viewableDownloadItems.has(downloadedItem.source.url)) { - this._viewableDownloadItems.set(downloadedItem.source.url, elem); + if (!this._downloadItems.has(download.source.url)) { + this._downloadItems.set(download.source.url, download); // On startup, all existing downloads fire this notification, so debounce them if (this._downloadTimer) { @@ -83,13 +63,13 @@ this.DownloadsManager = class DownloadsManager { } onDownloadRemoved(download) { - if (this._viewableDownloadItems.has(download.source.url)) { - this._viewableDownloadItems.delete(download.source.url); + if (this._downloadItems.has(download.source.url)) { + this._downloadItems.delete(download.source.url); this._store.dispatch({type: at.DOWNLOAD_CHANGED}); } } - async getDownloads(threshold, {numItems = this._viewableDownloadItems.size, onlySucceeded = false, onlyExists = false}) { + async getDownloads(threshold, {numItems = this._downloadItems.size, onlySucceeded = false, onlyExists = false}) { if (!threshold) { return []; } @@ -97,22 +77,22 @@ this.DownloadsManager = class DownloadsManager { // Only get downloads within the time threshold specified and sort by recency const downloadThreshold = Date.now() - threshold; - let downloads = [...this._viewableDownloadItems.values()] - .filter(elem => elem.download.endTime > downloadThreshold) - .sort((elem1, elem2) => elem1.download.endTime < elem2.download.endTime); + let downloads = [...this._downloadItems.values()] + .filter(download => download.endTime > downloadThreshold) + .sort((download1, download2) => download1.endTime < download2.endTime); - for (const elem of downloads) { + for (const download of downloads) { // Only include downloads where the file still exists if (onlyExists) { // Refresh download to ensure the 'exists' attribute is up to date - await elem.download.refresh(); - if (!elem.download.target.exists) { continue; } + await download.refresh(); + if (!download.target.exists) { continue; } } // Only include downloads that were completed successfully if (onlySucceeded) { - if (!elem.download.succeeded) { continue; } + if (!download.succeeded) { continue; } } - const formattedDownloadForHighlights = this.formatDownload(elem); + const formattedDownloadForHighlights = this.formatDownload(download); results.push(formattedDownloadForHighlights); if (results.length === numItems) { break; @@ -133,31 +113,41 @@ this.DownloadsManager = class DownloadsManager { } onAction(action) { - let downloadsCmd; + let doDownloadAction = callback => { + let download = this._downloadItems.get(action.data.url); + if (download) { + callback(download); + } + }; + switch (action.type) { case at.COPY_DOWNLOAD_LINK: - downloadsCmd = "downloadsCmd_copyLocation"; + doDownloadAction(download => { + DownloadsCommon.copyDownloadLink(download); + }); break; case at.REMOVE_DOWNLOAD_FILE: - downloadsCmd = "downloadsCmd_delete"; + doDownloadAction(download => { + DownloadsCommon.deleteDownload(download).catch(Cu.reportError); + }); break; case at.SHOW_DOWNLOAD_FILE: - downloadsCmd = "downloadsCmd_show"; + doDownloadAction(download => { + DownloadsCommon.showDownloadedFile( + new FileUtils.File(download.target.path)); + }); break; case at.OPEN_DOWNLOAD_FILE: - downloadsCmd = "downloadsCmd_open"; + doDownloadAction(download => { + DownloadsCommon.openDownloadedFile( + new FileUtils.File(download.target.path), null, + this._browser.ownerGlobal); + }); break; case at.UNINIT: this.uninit(); break; } - // Call the appropriate downloads command function based on the event we received - if (downloadsCmd) { - let elem = this._viewableDownloadItems.get(action.data.url); - if (elem) { - elem[downloadsCmd](); - } - } } }; this.EXPORTED_SYMBOLS = ["DownloadsManager"]; diff --git a/test/unit/lib/DownloadsManager.test.js b/test/unit/lib/DownloadsManager.test.js index 06c396888..b8607ebf3 100644 --- a/test/unit/lib/DownloadsManager.test.js +++ b/test/unit/lib/DownloadsManager.test.js @@ -5,18 +5,10 @@ import {GlobalOverrider} from "test/unit/utils"; describe("Downloads Manager", () => { let downloadsManager; let globals; - let download; - let sandbox; const DOWNLOAD_URL = "https://site.com/download.mov"; beforeEach(() => { globals = new GlobalOverrider(); - sandbox = globals.sandbox; - global.Cc["@mozilla.org/widget/clipboardhelper;1"] = { - getService() { - return {copyString: sinon.stub()}; - }, - }; global.Cc["@mozilla.org/timer;1"] = { createInstance() { return { @@ -31,10 +23,11 @@ describe("Downloads Manager", () => { addView: sinon.stub(), removeView: sinon.stub(), }), + copyDownloadLink: sinon.stub(), + deleteDownload: sinon.stub().returns(Promise.resolve()), + openDownloadedFile: sinon.stub(), + showDownloadedFile: sinon.stub(), }); - sandbox.stub(global.DownloadsViewUI.DownloadElementShell.prototype, "downloadsCmd_open"); - sandbox.stub(global.DownloadsViewUI.DownloadElementShell.prototype, "downloadsCmd_show"); - sandbox.stub(global.DownloadsViewUI.DownloadElementShell.prototype, "downloadsCmd_delete"); downloadsManager = new DownloadsManager(); downloadsManager.init({dispatch() {}}); @@ -45,10 +38,10 @@ describe("Downloads Manager", () => { succeeded: true, refresh: async () => {}, }); - download = downloadsManager._viewableDownloadItems.get(DOWNLOAD_URL); + assert.ok(downloadsManager._downloadItems.has(DOWNLOAD_URL)); }); afterEach(() => { - downloadsManager._viewableDownloadItems.clear(); + downloadsManager._downloadItems.clear(); globals.restore(); }); describe("#init", () => { @@ -59,21 +52,20 @@ describe("Downloads Manager", () => { }); describe("#onAction", () => { it("should copy the file on COPY_DOWNLOAD_LINK", () => { - sinon.spy(download, "downloadsCmd_copyLocation"); downloadsManager.onAction({type: at.COPY_DOWNLOAD_LINK, data: {url: DOWNLOAD_URL}}); - assert.calledOnce(download.downloadsCmd_copyLocation); + assert.calledOnce(global.DownloadsCommon.copyDownloadLink); }); it("should remove the file on REMOVE_DOWNLOAD_FILE", () => { downloadsManager.onAction({type: at.REMOVE_DOWNLOAD_FILE, data: {url: DOWNLOAD_URL}}); - assert.calledOnce(download.downloadsCmd_delete); + assert.calledOnce(global.DownloadsCommon.deleteDownload); }); it("should show the file on SHOW_DOWNLOAD_FILE", () => { downloadsManager.onAction({type: at.SHOW_DOWNLOAD_FILE, data: {url: DOWNLOAD_URL}}); - assert.calledOnce(download.downloadsCmd_show); + assert.calledOnce(global.DownloadsCommon.showDownloadedFile); }); it("should open the file on OPEN_DOWNLOAD_FILE if the type is download", () => { downloadsManager.onAction({type: at.OPEN_DOWNLOAD_FILE, data: {url: DOWNLOAD_URL, type: "download"}}); - assert.calledOnce(download.downloadsCmd_open); + assert.calledOnce(global.DownloadsCommon.openDownloadedFile); }); it("should copy the file on UNINIT", () => { // DownloadsManager._downloadData needs to exist first @@ -82,13 +74,13 @@ describe("Downloads Manager", () => { }); it("should not execute a download command if we do not have the correct url", () => { downloadsManager.onAction({type: at.SHOW_DOWNLOAD_FILE, data: {url: "unknown_url"}}); - assert.notCalled(download.downloadsCmd_show); + assert.notCalled(global.DownloadsCommon.showDownloadedFile); }); }); describe("#onDownloadAdded", () => { let newDownload; beforeEach(() => { - downloadsManager._viewableDownloadItems.clear(); + downloadsManager._downloadItems.clear(); newDownload = { source: {url: "https://site.com/newDownload.mov"}, endTime: Date.now(), @@ -98,20 +90,18 @@ describe("Downloads Manager", () => { }; }); afterEach(() => { - downloadsManager._viewableDownloadItems.clear(); + downloadsManager._downloadItems.clear(); }); it("should add a download on onDownloadAdded", () => { downloadsManager.onDownloadAdded(newDownload); - const result = downloadsManager._viewableDownloadItems.get("https://site.com/newDownload.mov"); - assert.ok(result); - assert.instanceOf(result, global.DownloadsViewUI.DownloadElementShell); + assert.ok(downloadsManager._downloadItems.has("https://site.com/newDownload.mov")); }); it("should not add a download if it already exists", () => { downloadsManager.onDownloadAdded(newDownload); downloadsManager.onDownloadAdded(newDownload); downloadsManager.onDownloadAdded(newDownload); downloadsManager.onDownloadAdded(newDownload); - const results = downloadsManager._viewableDownloadItems; + const results = downloadsManager._downloadItems; assert.equal(results.size, 1); }); it("should not return any downloads if no threshold is provided", async () => { @@ -275,7 +265,7 @@ describe("Downloads Manager", () => { describe("#onDownloadRemoved", () => { let newDownload; beforeEach(() => { - downloadsManager._viewableDownloadItems.clear(); + downloadsManager._downloadItems.clear(); newDownload = { source: {url: "https://site.com/removeMe.mov"}, endTime: Date.now(), diff --git a/test/unit/unit-entry.js b/test/unit/unit-entry.js index 8d94aee45..fc9dd01ba 100644 --- a/test/unit/unit-entry.js +++ b/test/unit/unit-entry.js @@ -5,15 +5,6 @@ import chaiJsonSchema from "chai-json-schema"; import enzyme from "enzyme"; enzyme.configure({adapter: new Adapter()}); -class DownloadElementShell { - downloadsCmd_open() {} - downloadsCmd_show() {} - downloadsCmd_openReferrer() {} - downloadsCmd_delete() {} - get sizeStrings() { return {stateLabel: "1.5 MB"}; } - displayName() {} -} - // Cause React warnings to make tests that trigger them fail const origConsoleError = console.error; // eslint-disable-line no-console console.error = function(msg, ...args) { // eslint-disable-line no-console @@ -104,7 +95,14 @@ const TEST_GLOBAL = { PluralForm: {get() {}}, Preferences: FakePrefs, PrivateBrowsingUtils: {isWindowPrivate: () => false}, - DownloadsViewUI: {DownloadElementShell}, + DownloadsViewUI: { + getDisplayName: () => "filename.ext", + getSizeWithUnits: () => "1.5 MB", + }, + FileUtils: { + // eslint-disable-next-line object-shorthand + File: function() {}, // NB: This is a function/constructor + }, Services: { locale: { get appLocaleAsLangTag() { return "en-US"; },