Bug 1381409 - Part 2 - Let each view keep the state of downloads relevant to it. r=mak
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Mon, 17 Jul 2017 12:07:17 +0100
changeset 369302 a529720f06eecf154d4784bbe845984c5efcb338
parent 369301 7bf2876b0ffd8fdb5107fcb245f02915805ca3c9
child 369303 4a2fd40a1239e375bc759fdf9b4b860162e38e0a
push id92682
push userpaolo.mozmail@amadzone.org
push dateTue, 18 Jul 2017 13:46:32 +0000
treeherdermozilla-inbound@4a2fd40a1239 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1381409
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1381409 - Part 2 - Let each view keep the state of downloads relevant to it. r=mak The front-end download views now maintain the old download state themselves, instead of relying on the DownloadsData object dispatching the onDownloadStateChanged notification. This allows each view to keep only the state relevant to it, for example the Downloads Panel already keeps the state only for the visible items. This also makes it possible for each view to use a different hash than the one provided by the legacy stateOfDownload method, and allows bypassing the DownloadsData indirection entirely. MozReview-Commit-ID: 2D1ixsZCkCa
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
@@ -765,24 +765,16 @@ DownloadsDataCtor.prototype = {
                           JSON.stringify(downloadMetaData), 0,
                           PlacesUtils.annotations.EXPIRE_WITH_HISTORY);
           } catch (ex) {
             Cu.reportError(ex);
           }
         }
       }
 
-      for (let view of this._views) {
-        try {
-          view.onDownloadStateChanged(download);
-        } catch (ex) {
-          Cu.reportError(ex);
-        }
-      }
-
       if (download.succeeded ||
           (download.error && download.error.becauseBlocked)) {
         this._notifyDownloadEvent("finish");
       }
     }
 
     if (!download.newDownloadNotified) {
       download.newDownloadNotified = true;
@@ -906,16 +898,23 @@ XPCOMUtils.defineLazyGetter(this, "Downl
 
 // DownloadsViewPrototype
 
 /**
  * A prototype for an object that registers itself with DownloadsData as soon
  * as a view is registered with it.
  */
 const DownloadsViewPrototype = {
+  /**
+   * Contains all the available Download objects and their current state value.
+   *
+   * SUBCLASSES MUST OVERRIDE THIS PROPERTY.
+   */
+  _oldDownloadStates: null,
+
   // Registration of views
 
   /**
    * Array of view objects that should be notified when the available status
    * data changes.
    *
    * SUBCLASSES MUST OVERRIDE THIS PROPERTY.
    */
@@ -1009,20 +1008,21 @@ 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.
    *
-   * @note Subclasses should override this.
+   * @note Subclasses should override this and still call the base method.
    */
   onDownloadAdded(download) {
-    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+    this._oldDownloadStates.set(download,
+                                DownloadsCommon.stateOfDownload(download));
   },
 
   /**
    * 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.
    *
    * The onDownloadChanged notification will always be sent afterwards.
@@ -1035,20 +1035,26 @@ const DownloadsViewPrototype = {
 
   /**
    * Called every time any state property of a Download may have changed,
    * including progress properties.
    *
    * Note that progress notification changes are throttled at the Downloads.jsm
    * API level, and there is no throttling mechanism in the front-end.
    *
-   * @note Subclasses should override this.
+   * @note Subclasses should override this and still call the base method.
    */
   onDownloadChanged(download) {
-    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+    let oldState = this._oldDownloadStates.get(download);
+    let newState = DownloadsCommon.stateOfDownload(download);
+    this._oldDownloadStates.set(download, newState);
+
+    if (oldState != newState) {
+      this.onDownloadStateChanged(download);
+    }
   },
 
   /**
    * Called when a data item is removed, ensures that the widget associated with
    * the view item is removed from the user interface.
    *
    * @param download
    *        Download object that is being removed.
@@ -1087,16 +1093,17 @@ const DownloadsViewPrototype = {
  * the registered download status indicators.
  *
  * Note that using this object does not automatically start the Download Manager
  * service.  Consumers will see an empty list of downloads until the service is
  * actually started.  This is useful to display a neutral progress indicator in
  * the main browser window until the autostart timeout elapses.
  */
 function DownloadsIndicatorDataCtor(aPrivate) {
+  this._oldDownloadStates = new WeakMap();
   this._isPrivate = aPrivate;
   this._views = [];
 }
 DownloadsIndicatorDataCtor.prototype = {
   __proto__: DownloadsViewPrototype,
 
   /**
    * Removes an object previously added using addView.
@@ -1115,16 +1122,17 @@ DownloadsIndicatorDataCtor.prototype = {
   // Callback functions from DownloadsData
 
   onDataLoadCompleted() {
     DownloadsViewPrototype.onDataLoadCompleted.call(this);
     this._updateViews();
   },
 
   onDownloadAdded(download) {
+    DownloadsViewPrototype.onDownloadAdded.call(this, 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
@@ -1152,16 +1160,17 @@ DownloadsIndicatorDataCtor.prototype = {
       // Existing higher level attention indication trumps ATTENTION_WARNING.
       if (this._attention != DownloadsCommon.ATTENTION_SEVERE) {
         this.attention = DownloadsCommon.ATTENTION_WARNING;
       }
     }
   },
 
   onDownloadChanged(download) {
+    DownloadsViewPrototype.onDownloadChanged.call(this, download);
     this._updateViews();
   },
 
   onDownloadRemoved(download) {
     this._itemCount--;
     this._updateViews();
   },
 
@@ -1317,16 +1326,17 @@ function DownloadsSummaryData(aIsPrivate
   // The following properties are updated by _refreshProperties and are then
   // propagated to the views.
   this._showingProgress = false;
   this._details = "";
   this._description = "";
   this._numActive = 0;
   this._percentComplete = -1;
 
+  this._oldDownloadStates = new WeakMap();
   this._isPrivate = aIsPrivate;
   this._views = [];
 }
 
 DownloadsSummaryData.prototype = {
   __proto__: DownloadsViewPrototype,
 
   /**
@@ -1350,27 +1360,29 @@ DownloadsSummaryData.prototype = {
   // are used for.
 
   onDataLoadCompleted() {
     DownloadsViewPrototype.onDataLoadCompleted.call(this);
     this._updateViews();
   },
 
   onDownloadAdded(download) {
+    DownloadsViewPrototype.onDownloadAdded.call(this, 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;
   },
 
-  onDownloadChanged() {
+  onDownloadChanged(download) {
+    DownloadsViewPrototype.onDownloadChanged.call(this, download);
     this._updateViews();
   },
 
   onDownloadRemoved(download) {
     let itemIndex = this._downloads.indexOf(download);
     this._downloads.splice(itemIndex, 1);
     this._updateViews();
   },
--- a/browser/components/downloads/content/allDownloadsViewOverlay.js
+++ b/browser/components/downloads/content/allDownloadsViewOverlay.js
@@ -173,17 +173,17 @@ HistoryDownload.prototype = {
  * both a history and a session download are present, the session download gets
  * priority and its information is displayed.
  *
  * On construction, a new richlistitem is created, and can be accessed through
  * the |element| getter. The shell doesn't insert the item in a richlistbox, the
  * caller must do it and remove the element when it's no longer needed.
  *
  * The caller is also responsible for forwarding status notifications for
- * session downloads, calling the onStateChanged and onChanged methods.
+ * session downloads, calling the onSessionDownloadChanged method.
  *
  * @param [optional] aSessionDownload
  *        The session download, required if aHistoryDownload is not set.
  * @param [optional] aHistoryDownload
  *        The history download, required if aSessionDownload is not set.
  */
 function HistoryDownloadElementShell(aSessionDownload, aHistoryDownload) {
   this.element = document.createElement("richlistitem");
@@ -234,16 +234,19 @@ HistoryDownloadElementShell.prototype = 
   },
   set sessionDownload(aValue) {
     if (this._sessionDownload != aValue) {
       if (!aValue && !this._historyDownload) {
         throw new Error("Should always have either a Download or a HistoryDownload");
       }
 
       this._sessionDownload = aValue;
+      if (aValue) {
+        this.sessionDownloadState = DownloadsCommon.stateOfDownload(aValue);
+      }
 
       this.ensureActive();
       this._updateUI();
     }
     return aValue;
   },
 
   _historyDownload: null,
@@ -286,17 +289,23 @@ HistoryDownloadElementShell.prototype = 
       goUpdateDownloadCommands();
     } else {
       // If a state change occurs in an item that is not currently selected,
       // this is the only command that may be affected.
       goUpdateCommand("downloadsCmd_clearDownloads");
     }
   },
 
-  onChanged() {
+  onSessionDownloadChanged() {
+    let newState = DownloadsCommon.stateOfDownload(this.sessionDownload);
+    if (this.sessionDownloadState != newState) {
+      this.sessionDownloadState = newState;
+      this.onStateChanged();
+    }
+
     // This cannot be placed within onStateChanged because
     // when a download goes from hasBlockedData to !hasBlockedData
     // it will still remain in the same state.
     this.element.classList.toggle("temporary-block",
                                   !!this.download.hasBlockedData);
     this._updateProgress();
   },
 
@@ -1100,22 +1109,18 @@ DownloadsPlacesView.prototype = {
   onDataLoadCompleted() {
     this._ensureInitialSelection();
   },
 
   onDownloadAdded(download) {
     this._addDownloadData(download, null, true);
   },
 
-  onDownloadStateChanged(download) {
-    this._viewItemsForDownloads.get(download).onStateChanged();
-  },
-
   onDownloadChanged(download) {
-    this._viewItemsForDownloads.get(download).onChanged();
+    this._viewItemsForDownloads.get(download).onSessionDownloadChanged();
   },
 
   onDownloadRemoved(download) {
     this._removeSessionDownloadFromView(download);
   },
 
   // nsIController
   supportsCommand(aCommand) {
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -783,23 +783,16 @@ const DownloadsView = {
 
     // 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();
     }
   },
 
-  onDownloadStateChanged(download) {
-    let viewItem = this._visibleViewItems.get(download);
-    if (viewItem) {
-      viewItem.onStateChanged();
-    }
-  },
-
   onDownloadChanged(download) {
     let viewItem = this._visibleViewItems.get(download);
     if (viewItem) {
       viewItem.onChanged();
     }
   },
 
   /**
@@ -1048,16 +1041,17 @@ XPCOMUtils.defineConstant(this, "Downloa
  *
  * @param download
  *        Download object to be associated with the view item.
  * @param aElement
  *        XUL element corresponding to the single download item in the view.
  */
 function DownloadsViewItem(download, aElement) {
   this.download = download;
+  this.downloadState = DownloadsCommon.stateOfDownload(download);
   this.element = aElement;
   this.element._shell = this;
 
   this.element.setAttribute("type", "download");
   this.element.classList.add("download-state");
 
   this._updateState();
 }
@@ -1065,21 +1059,22 @@ function DownloadsViewItem(download, aEl
 DownloadsViewItem.prototype = {
   __proto__: DownloadsViewUI.DownloadElementShell.prototype,
 
   /**
    * The XUL element corresponding to the associated richlistbox item.
    */
   _element: null,
 
-  onStateChanged() {
-    this._updateState();
-  },
-
   onChanged() {
+    let newState = DownloadsCommon.stateOfDownload(this.download);
+    if (this.downloadState != newState) {
+      this.downloadState = newState;
+      this._updateState();
+    }
     this._updateProgress();
   },
 
   isCommandEnabled(aCommand) {
     switch (aCommand) {
       case "downloadsCmd_open": {
         if (!this.download.succeeded) {
           return false;