From d01f5d9f73479bb2cdd26638be6cb29e9d0084e3 Mon Sep 17 00:00:00 2001 From: Paolo Amadini Date: Fri, 13 Sep 2013 22:44:54 +0200 Subject: [PATCH] Bug 913110 - Add a combined summary of public and private downloads. r=enn --- .../jsdownloads/src/DownloadList.jsm | 186 ++++++++++++++++++ .../components/jsdownloads/src/Downloads.jsm | 97 ++++++--- .../test/unit/common_test_Download.js | 30 --- .../components/jsdownloads/test/unit/head.js | 48 +++-- .../test/unit/test_DownloadIntegration.js | 2 +- .../test/unit/test_DownloadList.js | 119 ++++++++++- .../jsdownloads/test/unit/test_Downloads.js | 20 ++ 7 files changed, 432 insertions(+), 70 deletions(-) diff --git a/toolkit/components/jsdownloads/src/DownloadList.jsm b/toolkit/components/jsdownloads/src/DownloadList.jsm index 7e63936a54e..4388adb698b 100644 --- a/toolkit/components/jsdownloads/src/DownloadList.jsm +++ b/toolkit/components/jsdownloads/src/DownloadList.jsm @@ -13,6 +13,9 @@ * * DownloadCombinedList * Provides a unified, unordered list combining public and private downloads. + * + * DownloadSummary + * Provides an aggregated view on the contents of a DownloadList. */ "use strict"; @@ -20,6 +23,7 @@ this.EXPORTED_SYMBOLS = [ "DownloadList", "DownloadCombinedList", + "DownloadSummary", ]; //////////////////////////////////////////////////////////////////////////////// @@ -332,3 +336,185 @@ DownloadCombinedList.prototype = { this._notifyAllViews("onDownloadRemoved", aDownload); }, }; + +//////////////////////////////////////////////////////////////////////////////// +//// DownloadSummary + +/** + * Provides an aggregated view on the contents of a DownloadList. + */ +function DownloadSummary() { + this._downloads = []; + this._views = new Set(); +} + +DownloadSummary.prototype = { + /** + * Array of Download objects that are currently part of the summary. + */ + _downloads: null, + + /** + * Underlying DownloadList whose contents should be summarized. + */ + _list: null, + + /** + * This method may be called once to bind this object to a DownloadList. + * + * Views on the summarized data can be registered before this object is bound + * to an actual list. This allows the summary to be used without requiring + * the initialization of the DownloadList first. + * + * @param aList + * Underlying DownloadList whose contents should be summarized. + */ + bindToList: function (aList) + { + if (this._list) { + throw new Error("bindToList may be called only once."); + } + + aList.addView(this); + + // Set the list reference only after addView has returned, so that we don't + // send a notification to our views for each download that is added. + this._list = aList; + this._onListChanged(); + }, + + /** + * Set of currently registered views. + */ + _views: null, + + /** + * Adds a view that will be notified of changes to the summary. The newly + * added view will receive an initial onSummaryChanged notification. + * + * @param aView + * The view object to add. The following methods may be defined: + * { + * onSummaryChanged: function () { + * // Called after any property of the summary has changed. + * }, + * } + */ + addView: function (aView) + { + this._views.add(aView); + + if ("onSummaryChanged" in aView) { + try { + aView.onSummaryChanged(); + } catch (ex) { + Cu.reportError(ex); + } + } + }, + + /** + * Removes a view that was previously added using addView. The removed view + * will not receive any more notifications after this method returns. + * + * @param aView + * The view object to remove. + */ + removeView: function (aView) + { + this._views.delete(aView); + }, + + /** + * Indicates whether all the downloads are currently stopped. + */ + allHaveStopped: true, + + /** + * Indicates the total number of bytes to be transferred before completing all + * the downloads that are currently in progress. + * + * For downloads that do not have a known final size, the number of bytes + * currently transferred is reported as part of this property. + * + * This is zero if no downloads are currently in progress. + */ + progressTotalBytes: 0, + + /** + * Number of bytes currently transferred as part of all the downloads that are + * currently in progress. + * + * This is zero if no downloads are currently in progress. + */ + progressCurrentBytes: 0, + + /** + * This function is called when any change in the list of downloads occurs, + * and will recalculate the summary and notify the views in case the + * aggregated properties are different. + */ + _onListChanged: function () { + let allHaveStopped = true; + let progressTotalBytes = 0; + let progressCurrentBytes = 0; + + // Recalculate the aggregated state. See the description of the individual + // properties for an explanation of the summarization logic. + for (let download of this._downloads) { + if (!download.stopped) { + allHaveStopped = false; + progressTotalBytes += download.hasProgress ? download.totalBytes + : download.currentBytes; + progressCurrentBytes += download.currentBytes; + } + } + + // Exit now if the properties did not change. + if (this.allHaveStopped == allHaveStopped && + this.progressTotalBytes == progressTotalBytes && + this.progressCurrentBytes == progressCurrentBytes) { + return; + } + + this.allHaveStopped = allHaveStopped; + this.progressTotalBytes = progressTotalBytes; + this.progressCurrentBytes = progressCurrentBytes; + + // Notify all the views that our properties changed. + for (let view of this._views) { + try { + if ("onSummaryChanged" in view) { + view.onSummaryChanged(); + } + } catch (ex) { + Cu.reportError(ex); + } + } + }, + + ////////////////////////////////////////////////////////////////////////////// + //// DownloadList view + + onDownloadAdded: function (aDownload) + { + this._downloads.push(aDownload); + if (this._list) { + this._onListChanged(); + } + }, + + onDownloadChanged: function (aDownload) + { + this._onListChanged(); + }, + + onDownloadRemoved: function (aDownload) + { + let index = this._downloads.indexOf(aDownload); + if (index != -1) { + this._downloads.splice(index, 1); + } + this._onListChanged(); + }, +}; diff --git a/toolkit/components/jsdownloads/src/Downloads.jsm b/toolkit/components/jsdownloads/src/Downloads.jsm index 1b370750eb9..12421a0b655 100644 --- a/toolkit/components/jsdownloads/src/Downloads.jsm +++ b/toolkit/components/jsdownloads/src/Downloads.jsm @@ -31,6 +31,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "DownloadIntegration", "resource://gre/modules/DownloadIntegration.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "DownloadList", "resource://gre/modules/DownloadList.jsm"); +XPCOMUtils.defineLazyModuleGetter(this, "DownloadSummary", + "resource://gre/modules/DownloadList.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "DownloadUIHelper", "resource://gre/modules/DownloadUIHelper.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "Promise", @@ -162,44 +164,87 @@ this.Downloads = { * @rejects JavaScript exception. */ getList: function (aType) + { + if (!this._promiseListsInitialized) { + this._promiseListsInitialized = Task.spawn(function () { + let publicList = new DownloadList(); + let privateList = new DownloadList(); + let combinedList = new DownloadCombinedList(publicList, privateList); + + try { + yield DownloadIntegration.addListObservers(publicList, false); + yield DownloadIntegration.addListObservers(privateList, true); + yield DownloadIntegration.initializePublicDownloadList(publicList); + } catch (ex) { + Cu.reportError(ex); + } + + let publicSummary = yield this.getSummary(Downloads.PUBLIC); + let privateSummary = yield this.getSummary(Downloads.PRIVATE); + let combinedSummary = yield this.getSummary(Downloads.ALL); + + publicSummary.bindToList(publicList); + privateSummary.bindToList(privateList); + combinedSummary.bindToList(combinedList); + + this._lists[Downloads.PUBLIC] = publicList; + this._lists[Downloads.PRIVATE] = privateList; + this._lists[Downloads.ALL] = combinedList; + }.bind(this)); + } + + return this._promiseListsInitialized.then(() => this._lists[aType]); + }, + + /** + * Promise resolved when the initialization of the download lists has + * completed, or null if initialization has never been requested. + */ + _promiseListsInitialized: null, + + /** + * After initialization, this object is populated with one key for each type + * of download list that can be returned (Downloads.PUBLIC, Downloads.PRIVATE, + * or Downloads.ALL). The values are the DownloadList objects. + */ + _lists: {}, + + /** + * Retrieves the specified type of DownloadSummary object. There is one + * download summary for each type, and this method always retrieves a + * reference to the same download summary when called with the same argument. + * + * Calling this function does not cause the list of public downloads to be + * reloaded from the previous session. The summary will behave as if no + * downloads are present until the getList method is called. + * + * @param aType + * This can be Downloads.PUBLIC, Downloads.PRIVATE, or Downloads.ALL. + * + * @return {Promise} + * @resolves The requested DownloadList or DownloadCombinedList object. + * @rejects JavaScript exception. + */ + getSummary: function (aType) { if (aType != Downloads.PUBLIC && aType != Downloads.PRIVATE && aType != Downloads.ALL) { throw new Error("Invalid aType argument."); } - if (!(aType in this._listPromises)) { - this._listPromises[aType] = Task.spawn(function () { - let list; - if (aType == Downloads.ALL) { - list = new DownloadCombinedList( - (yield this.getList(Downloads.PUBLIC)), - (yield this.getList(Downloads.PRIVATE))); - } else { - list = new DownloadList(); - try { - yield DownloadIntegration.addListObservers( - list, aType == Downloads.PRIVATE); - if (aType == Downloads.PUBLIC) { - yield DownloadIntegration.initializePublicDownloadList(list); - } - } catch (ex) { - Cu.reportError(ex); - } - } - throw new Task.Result(list); - }.bind(this)); + if (!(aType in this._summaries)) { + this._summaries[aType] = new DownloadSummary(); } - return this._listPromises[aType]; + return Promise.resolve(this._summaries[aType]); }, /** - * This object is populated by the getList method with one key for each type - * of object that can be returned (Downloads.PUBLIC, Downloads.PRIVATE, or - * Downloads.ALL). The values are the promises returned by the method. + * This object is populated by the getSummary method with one key for each + * type of object that can be returned (Downloads.PUBLIC, Downloads.PRIVATE, + * or Downloads.ALL). The values are the DownloadSummary objects. */ - _listPromises: {}, + _summaries: {}, /** * Returns the system downloads directory asynchronously. diff --git a/toolkit/components/jsdownloads/test/unit/common_test_Download.js b/toolkit/components/jsdownloads/test/unit/common_test_Download.js index 29d2d9f260c..a50281ae07e 100644 --- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js +++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ -35,36 +35,6 @@ function promiseStartDownload(aSourceUrl) { }); } -/** - * Waits for a download to reach half of its progress, in case it has not - * reached the expected progress already. - * - * @param aDownload - * The Download object to wait upon. - * - * @return {Promise} - * @resolves When the download has reached half of its progress. - * @rejects Never. - */ -function promiseDownloadMidway(aDownload) { - let deferred = Promise.defer(); - - // Wait for the download to reach half of its progress. - let onchange = function () { - if (!aDownload.stopped && !aDownload.canceled && aDownload.progress == 50) { - aDownload.onchange = null; - deferred.resolve(); - } - }; - - // Register for the notification, but also call the function directly in - // case the download already reached the expected progress. - aDownload.onchange = onchange; - onchange(); - - return deferred.promise; -} - /** * Waits for a download to finish, in case it has not finished already. * diff --git a/toolkit/components/jsdownloads/test/unit/head.js b/toolkit/components/jsdownloads/test/unit/head.js index c45a5f382d8..3510d2e8e66 100644 --- a/toolkit/components/jsdownloads/test/unit/head.js +++ b/toolkit/components/jsdownloads/test/unit/head.js @@ -463,6 +463,36 @@ function promiseStartExternalHelperAppServiceDownload(aSourceUrl) { return deferred.promise; } +/** + * Waits for a download to reach half of its progress, in case it has not + * reached the expected progress already. + * + * @param aDownload + * The Download object to wait upon. + * + * @return {Promise} + * @resolves When the download has reached half of its progress. + * @rejects Never. + */ +function promiseDownloadMidway(aDownload) { + let deferred = Promise.defer(); + + // Wait for the download to reach half of its progress. + let onchange = function () { + if (!aDownload.stopped && !aDownload.canceled && aDownload.progress == 50) { + aDownload.onchange = null; + deferred.resolve(); + } + }; + + // Register for the notification, but also call the function directly in + // case the download already reached the expected progress. + aDownload.onchange = onchange; + onchange(); + + return deferred.promise; +} + /** * Returns a new public or private DownloadList object. * @@ -475,19 +505,13 @@ function promiseStartExternalHelperAppServiceDownload(aSourceUrl) { */ function promiseNewList(aIsPrivate) { - let type = aIsPrivate ? Downloads.PRIVATE : Downloads.PUBLIC; + // We need to clear all the internal state for the list and summary objects, + // since all the objects are interdependent internally. + Downloads._promiseListsInitialized = null; + Downloads._lists = {}; + Downloads._summaries = {}; - // Force the creation of a new list. - if (type in Downloads._listPromises) { - delete Downloads._listPromises[type]; - } - - // Invalidate the combined list, if any. - if (Downloads.ALL in Downloads._listPromises) { - delete Downloads._listPromises[Downloads.ALL]; - } - - return Downloads.getList(type); + return Downloads.getList(aIsPrivate ? Downloads.PRIVATE : Downloads.PUBLIC); } /** diff --git a/toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js b/toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js index fd8cf917984..9dee3bfe5a3 100644 --- a/toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js +++ b/toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js @@ -278,7 +278,7 @@ add_task(function test_mix_notifications() mustInterruptResponses(); let publicList = yield promiseNewList(); - let privateList = yield promiseNewList(true); + let privateList = yield Downloads.getList(Downloads.PRIVATE); let download1 = yield promiseNewDownload(httpUrl("interruptible.txt")); let download2 = yield promiseNewDownload(httpUrl("interruptible.txt")); let promiseAttempt1 = download1.start(); diff --git a/toolkit/components/jsdownloads/test/unit/test_DownloadList.js b/toolkit/components/jsdownloads/test/unit/test_DownloadList.js index 074b4bfaff3..8dd39eca190 100644 --- a/toolkit/components/jsdownloads/test/unit/test_DownloadList.js +++ b/toolkit/components/jsdownloads/test/unit/test_DownloadList.js @@ -137,7 +137,7 @@ add_task(function test_remove() add_task(function test_DownloadCombinedList_add_remove_getAll() { let publicList = yield promiseNewList(); - let privateList = yield promiseNewList(true); + let privateList = yield Downloads.getList(Downloads.PRIVATE); let combinedList = yield Downloads.getList(Downloads.ALL); let publicDownload = yield promiseNewDownload(); @@ -448,3 +448,120 @@ add_task(function test_removeFinished() let downloads = yield list.getAll() do_check_eq(downloads.length, 1); }); + +/** + * Tests the global DownloadSummary objects for the public, private, and + * combined download lists. + */ +add_task(function test_DownloadSummary() +{ + mustInterruptResponses(); + + let publicList = yield promiseNewList(); + let privateList = yield Downloads.getList(Downloads.PRIVATE); + + let publicSummary = yield Downloads.getSummary(Downloads.PUBLIC); + let privateSummary = yield Downloads.getSummary(Downloads.PRIVATE); + let combinedSummary = yield Downloads.getSummary(Downloads.ALL); + + // Add a public download that has succeeded. + let succeededPublicDownload = yield promiseNewDownload(); + yield succeededPublicDownload.start(); + publicList.add(succeededPublicDownload); + + // Add a public download that has been canceled midway. + let canceledPublicDownload = + yield promiseNewDownload(httpUrl("interruptible.txt")); + canceledPublicDownload.start(); + yield promiseDownloadMidway(canceledPublicDownload); + yield canceledPublicDownload.cancel(); + publicList.add(canceledPublicDownload); + + // Add a public download that is in progress. + let inProgressPublicDownload = + yield promiseNewDownload(httpUrl("interruptible.txt")); + inProgressPublicDownload.start(); + yield promiseDownloadMidway(inProgressPublicDownload); + publicList.add(inProgressPublicDownload); + + // Add a private download that is in progress. + let inProgressPrivateDownload = yield Downloads.createDownload({ + source: { url: httpUrl("interruptible.txt"), isPrivate: true }, + target: getTempFile(TEST_TARGET_FILE_NAME).path, + }); + inProgressPrivateDownload.start(); + yield promiseDownloadMidway(inProgressPrivateDownload); + privateList.add(inProgressPrivateDownload); + + // Verify that the summary includes the total number of bytes and the + // currently transferred bytes only for the downloads that are not stopped. + // For simplicity, we assume that after a download is added to the list, its + // current state is immediately propagated to the summary object, which is + // true in the current implementation, though it is not guaranteed as all the + // download operations may happen asynchronously. + do_check_false(publicSummary.allHaveStopped); + do_check_eq(publicSummary.progressTotalBytes, TEST_DATA_SHORT.length * 2); + do_check_eq(publicSummary.progressCurrentBytes, TEST_DATA_SHORT.length); + + do_check_false(privateSummary.allHaveStopped); + do_check_eq(privateSummary.progressTotalBytes, TEST_DATA_SHORT.length * 2); + do_check_eq(privateSummary.progressCurrentBytes, TEST_DATA_SHORT.length); + + do_check_false(combinedSummary.allHaveStopped); + do_check_eq(combinedSummary.progressTotalBytes, TEST_DATA_SHORT.length * 4); + do_check_eq(combinedSummary.progressCurrentBytes, TEST_DATA_SHORT.length * 2); + + yield inProgressPublicDownload.cancel(); + + // Stopping the download should have excluded it from the summary. + do_check_true(publicSummary.allHaveStopped); + do_check_eq(publicSummary.progressTotalBytes, 0); + do_check_eq(publicSummary.progressCurrentBytes, 0); + + do_check_false(privateSummary.allHaveStopped); + do_check_eq(privateSummary.progressTotalBytes, TEST_DATA_SHORT.length * 2); + do_check_eq(privateSummary.progressCurrentBytes, TEST_DATA_SHORT.length); + + do_check_false(combinedSummary.allHaveStopped); + do_check_eq(combinedSummary.progressTotalBytes, TEST_DATA_SHORT.length * 2); + do_check_eq(combinedSummary.progressCurrentBytes, TEST_DATA_SHORT.length); + + yield inProgressPrivateDownload.cancel(); + + // All the downloads should be stopped now. + do_check_true(publicSummary.allHaveStopped); + do_check_eq(publicSummary.progressTotalBytes, 0); + do_check_eq(publicSummary.progressCurrentBytes, 0); + + do_check_true(privateSummary.allHaveStopped); + do_check_eq(privateSummary.progressTotalBytes, 0); + do_check_eq(privateSummary.progressCurrentBytes, 0); + + do_check_true(combinedSummary.allHaveStopped); + do_check_eq(combinedSummary.progressTotalBytes, 0); + do_check_eq(combinedSummary.progressCurrentBytes, 0); +}); + +/** + * Checks that views receive the summary change notification. This is tested on + * the combined summary when adding a public download, as we assume that if we + * pass the test in this case we will also pass it in the others. + */ +add_task(function test_DownloadSummary_notifications() +{ + let list = yield promiseNewList(); + let summary = yield Downloads.getSummary(Downloads.ALL); + + let download = yield promiseNewDownload(); + list.add(download); + + // Check that we receive change notifications. + let receivedOnSummaryChanged = false; + summary.addView({ + onSummaryChanged: function () { + receivedOnSummaryChanged = true; + }, + }); + yield download.start(); + do_check_true(receivedOnSummaryChanged); +}); diff --git a/toolkit/components/jsdownloads/test/unit/test_Downloads.js b/toolkit/components/jsdownloads/test/unit/test_Downloads.js index 678d1a6f0d0..6c7b5ef11f6 100644 --- a/toolkit/components/jsdownloads/test/unit/test_Downloads.js +++ b/toolkit/components/jsdownloads/test/unit/test_Downloads.js @@ -118,6 +118,26 @@ add_task(function test_getList() do_check_neq(publicListOne, privateListOne); }); +/** + * Tests that the getSummary function returns the same summary when called + * multiple times with the same argument, but returns different summaries when + * called with different arguments. More detailed tests are implemented + * separately for the DownloadSummary object in the DownloadList module. + */ +add_task(function test_getSummary() +{ + let publicSummaryOne = yield Downloads.getSummary(Downloads.PUBLIC); + let privateSummaryOne = yield Downloads.getSummary(Downloads.PRIVATE); + + let publicSummaryTwo = yield Downloads.getSummary(Downloads.PUBLIC); + let privateSummaryTwo = yield Downloads.getSummary(Downloads.PRIVATE); + + do_check_eq(publicSummaryOne, publicSummaryTwo); + do_check_eq(privateSummaryOne, privateSummaryTwo); + + do_check_neq(publicSummaryOne, privateSummaryOne); +}); + /** * Tests that the getSystemDownloadsDirectory returns a valid nsFile * download directory object.