Bug 1381409 - Part 1 - Always add Download objects in their original order. r=mak draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Sun, 16 Jul 2017 17:36:00 +0100
changeset 609698 4e5537f0367c6584ad6578f93c378dfc98d78bd3
parent 609596 aff336ac161daa3ea350e59a288963edbd58ed39
child 609716 22853ac5f25999b94053112d3045bfb9292308f7
child 609737 3c66d1d7d8a75b414d317294adb18c21d565a90b
push id68643
push userpaolo.mozmail@amadzone.org
push dateMon, 17 Jul 2017 08:50:07 +0000
reviewersmak
bugs1381409
milestone56.0a1
Bug 1381409 - Part 1 - Always add Download objects in their original order. r=mak When a new front-end view is added to the DownloadsData object, the Download objects it contains are loaded into the view starting with the newest, which is the opposite of the order used by the underlying DownloadList. This was originally implemented as an optimization for the Downloads Panel, at a time when the full list of completed downloads was persisted in the same SQLite database as session downloads. This difference is now unnecessary given the low number of session downloads, and can be removed in preparation for bypassing the DownloadsData indirection entirely. MozReview-Commit-ID: 5EBkqvyXFq4
browser/components/downloads/DownloadsCommon.jsm
browser/components/downloads/content/allDownloadsViewOverlay.js
browser/components/downloads/content/downloads.js
--- a/browser/components/downloads/DownloadsCommon.jsm
+++ b/browser/components/downloads/DownloadsCommon.jsm
@@ -720,17 +720,17 @@ DownloadsDataCtor.prototype = {
     // download terminates on a Desktop browser, it becomes a history download,
     // for which the end time is stored differently, as a Places annotation.
     download.endTime = Date.now();
 
     this.oldDownloadStates.set(download,
                                DownloadsCommon.stateOfDownload(download));
 
     for (let view of this._views) {
-      view.onDownloadAdded(download, true);
+      view.onDownloadAdded(download);
     }
   },
 
   onDownloadChanged(download) {
     let oldState = this.oldDownloadStates.get(download);
     let newState = DownloadsCommon.stateOfDownload(download);
     this.oldDownloadStates.set(download, newState);
 
@@ -835,21 +835,20 @@ DownloadsDataCtor.prototype = {
    *
    * @param aView
    *        DownloadsView object to be initialized.
    */
   _updateView(aView) {
     // Indicate to the view that a batch loading operation is in progress.
     aView.onDataLoadStarting();
 
-    // Sort backwards by start time, ensuring that the most recent
-    // downloads are added first regardless of their state.
-    let downloadsArray = [...this.downloads];
-    downloadsArray.sort((a, b) => b.startTime - a.startTime);
-    downloadsArray.forEach(download => aView.onDownloadAdded(download, false));
+    // This iterates downloads in the same order as they were originally added.
+    for (let download of this.downloads) {
+      aView.onDownloadAdded(download);
+    }
 
     // Notify the view that all data is available.
     aView.onDataLoadCompleted();
   },
 
   // Notifications sent to the most recent browser window only
 
   /**
@@ -1009,26 +1008,20 @@ const DownloadsViewPrototype = {
   },
 
   /**
    * Called when a new download data item is available, either during the
    * asynchronous data load or when a new download is started.
    *
    * @param download
    *        Download object that was just added.
-   * @param newest
-   *        When true, indicates that this item is the most recent and should be
-   *        added in the topmost position.  This happens when a new download is
-   *        started.  When false, indicates that the item is the least recent
-   *        with regard to the items that have been already added. The latter
-   *        generally happens during the asynchronous data load.
    *
    * @note Subclasses should override this.
    */
-  onDownloadAdded(download, newest) {
+  onDownloadAdded(download) {
     throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
   },
 
   /**
    * Called when the overall state of a Download has changed. In particular,
    * this is called only once when the download succeeds or is blocked
    * permanently, and is never called if only the current progress changed.
    *
@@ -1121,17 +1114,17 @@ DownloadsIndicatorDataCtor.prototype = {
 
   // Callback functions from DownloadsData
 
   onDataLoadCompleted() {
     DownloadsViewPrototype.onDataLoadCompleted.call(this);
     this._updateViews();
   },
 
-  onDownloadAdded(download, newest) {
+  onDownloadAdded(download) {
     this._itemCount++;
     this._updateViews();
   },
 
   onDownloadStateChanged(download) {
     if (!download.succeeded && download.error && download.error.reputationCheckVerdict) {
       switch (download.error.reputationCheckVerdict) {
         case Downloads.Error.BLOCK_VERDICT_UNCOMMON: // fall-through
@@ -1356,23 +1349,18 @@ DownloadsSummaryData.prototype = {
   // DownloadsViewPrototype for more information on what these functions
   // are used for.
 
   onDataLoadCompleted() {
     DownloadsViewPrototype.onDataLoadCompleted.call(this);
     this._updateViews();
   },
 
-  onDownloadAdded(download, newest) {
-    if (newest) {
-      this._downloads.unshift(download);
-    } else {
-      this._downloads.push(download);
-    }
-
+  onDownloadAdded(download) {
+    this._downloads.unshift(download);
     this._updateViews();
   },
 
   onDownloadStateChanged() {
     // Since the state of a download changed, reset the estimated time left.
     this._lastRawTimeLeft = -1;
     this._lastTimeLeft = -1;
   },
--- a/browser/components/downloads/content/allDownloadsViewOverlay.js
+++ b/browser/components/downloads/content/allDownloadsViewOverlay.js
@@ -605,17 +605,17 @@ DownloadsPlacesView.prototype = {
    *     alongside the other session downloads. If we don't, then we go ahead
    *     and create a new element for the download.
    *
    * @param [optional] sessionDownload
    *        A Download object, or null for history downloads.
    * @param [optional] aPlacesNode
    *        The Places node for a history download, or null for session downloads.
    * @param [optional] aNewest
-   *        @see onDownloadAdded. Ignored for history downloads.
+   *        Whether the download should be added at the top of the list.
    * @param [optional] aDocumentFragment
    *        To speed up the appending of multiple elements to the end of the
    *        list which are coming in a single batch (i.e. invalidateContainer),
    *        a document fragment may be passed to which the new elements would
    *        be appended. It's the caller's job to ensure the fragment is merged
    *        to the richlistbox at the end.
    */
   _addDownloadData(sessionDownload, aPlacesNode, aNewest = false,
@@ -1095,18 +1095,18 @@ DownloadsPlacesView.prototype = {
     }
   },
 
   onDataLoadStarting() {},
   onDataLoadCompleted() {
     this._ensureInitialSelection();
   },
 
-  onDownloadAdded(download, newest) {
-    this._addDownloadData(download, null, newest);
+  onDownloadAdded(download) {
+    this._addDownloadData(download, null, true);
   },
 
   onDownloadStateChanged(download) {
     this._viewItemsForDownloads.get(download).onStateChanged();
   },
 
   onDownloadChanged(download) {
     this._viewItemsForDownloads.get(download).onChanged();
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -762,43 +762,27 @@ const DownloadsView = {
   },
 
   /**
    * Called when a new download data item is available, either during the
    * asynchronous data load or when a new download is started.
    *
    * @param aDownload
    *        Download object that was just added.
-   * @param aNewest
-   *        When true, indicates that this item is the most recent and should be
-   *        added in the topmost position.  This happens when a new download is
-   *        started.  When false, indicates that the item is the least recent
-   *        and should be appended.  The latter generally happens during the
-   *        asynchronous data load.
    */
-  onDownloadAdded(download, aNewest) {
-    DownloadsCommon.log("A new download data item was added - aNewest =",
-                        aNewest);
+  onDownloadAdded(download) {
+    DownloadsCommon.log("A new download data item was added");
+
+    this._downloads.unshift(download);
 
-    if (aNewest) {
-      this._downloads.unshift(download);
-    } else {
-      this._downloads.push(download);
-    }
-
-    let itemsNowOverflow = this._downloads.length > this.kItemCountLimit;
-    if (aNewest || !itemsNowOverflow) {
-      // The newly added item is visible in the panel and we must add the
-      // corresponding element.  This is either because it is the first item, or
-      // because it was added at the bottom but the list still doesn't overflow.
-      this._addViewItem(download, aNewest);
-    }
-    if (aNewest && itemsNowOverflow) {
-      // If the list overflows, remove the last item from the panel to make room
-      // for the new one that we just added at the top.
+    // The newly added item is visible in the panel and we must add the
+    // corresponding element. If the list overflows, remove the last item from
+    // the panel to make room for the new one that we just added at the top.
+    this._addViewItem(download, true);
+    if (this._downloads.length > this.kItemCountLimit) {
       this._removeViewItem(this._downloads[this.kItemCountLimit]);
     }
 
     // For better performance during batch loads, don't update the count for
     // every item, because the interface won't be visible until load finishes.
     if (!this.loading) {
       this._itemCountChanged();
     }