Bug 1381409 - Part 2 - Let each view keep the state of downloads relevant to it. r=mak draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Mon, 17 Jul 2017 11:20:28 +0100
changeset 609716 22853ac5f25999b94053112d3045bfb9292308f7
parent 609698 4e5537f0367c6584ad6578f93c378dfc98d78bd3
child 637641 bd2a2f016abe83824ae26c3b807345d47c19944a
push id68656
push userpaolo.mozmail@amadzone.org
push dateMon, 17 Jul 2017 10:23:30 +0000
reviewersmak
bugs1381409
milestone56.0a1
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.
    */
@@ -1012,17 +1011,18 @@ const DownloadsViewPrototype = {
    * asynchronous data load or when a new download is started.
    *
    * @param download
    *        Download object that was just added.
    *
    * @note Subclasses should override this.
    */
   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.
@@ -1038,17 +1038,23 @@ const DownloadsViewPrototype = {
    * 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.
    */
   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();
   },
 
@@ -1099,22 +1108,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
@@ -1048,16 +1048,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 +1066,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;