Bug 1115369 - Use notifications instead of getViewItem for DownloadsView. r=mak
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Wed, 14 Jan 2015 17:30:27 +0000
changeset 223841 c10c7d4d04a2e69dac93085d325590164151b145
parent 223840 d5f3005f78c69ea0921cd883a985b4c86070f332
child 223842 85a8c3b8ad3837822bbe729f89888cebdfefe265
push id54039
push userryanvm@gmail.com
push dateWed, 14 Jan 2015 21:32:59 +0000
treeherdermozilla-inbound@af7c46628581 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1115369
milestone38.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 1115369 - Use notifications instead of getViewItem for DownloadsView. r=mak
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
@@ -713,17 +713,17 @@ DownloadsDataCtor.prototype = {
 
     if (wasInProgress && !aDataItem.inProgress) {
       aDataItem.endTime = Date.now();
     }
 
     if (oldState != aDataItem.state) {
       for (let view of this._views) {
         try {
-          view.getViewItem(aDataItem).onStateChange(oldState);
+          view.onDataItemStateChanged(aDataItem, oldState);
         } catch (ex) {
           Cu.reportError(ex);
         }
       }
 
       // This state transition code should actually be located in a Downloads
       // API module (bug 941009).  Moreover, the fact that state is stored as
       // annotations should be ideally hidden behind methods of
@@ -751,17 +751,17 @@ DownloadsDataCtor.prototype = {
       this._notifyDownloadEvent("start");
     }
 
     if (!wasDone && aDataItem.done) {
       this._notifyDownloadEvent("finish");
     }
 
     for (let view of this._views) {
-      view.getViewItem(aDataItem).onProgressChange();
+      view.onDataItemChanged(aDataItem);
     }
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Registration of views
 
   /**
    * Adds an object to be notified when the available download data changes.
@@ -1245,26 +1245,36 @@ const DownloadsViewPrototype = {
    *
    * @note Subclasses should override this.
    */
   onDataItemRemoved(aDataItem) {
     throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
   },
 
   /**
-   * Returns the view item associated with the provided data item for this view.
+   * Called when the "state" property of a DownloadsDataItem has changed.
    *
-   * @param aDataItem
-   *        DownloadsDataItem object for which the view item is requested.
-   *
-   * @return Object that can be used to notify item status events.
+   * The onDataItemChanged notification will be sent afterwards.
    *
    * @note Subclasses should override this.
    */
-  getViewItem(aDataItem) {
+  onDataItemStateChanged(aDataItem) {
+    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+  },
+
+  /**
+   * Called every time any state property of a DownloadsDataItem may have
+   * changed, including progress properties and the "state" property.
+   *
+   * 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.
+   */
+  onDataItemChanged(aDataItem) {
     throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
   },
 
   /**
    * Private function used to refresh the internal properties being sent to
    * each registered view.
    *
    * @note Subclasses should override this.
@@ -1353,44 +1363,31 @@ DownloadsIndicatorDataCtor.prototype = {
    * @param aDataItem
    *        DownloadsDataItem object that is being removed.
    */
   onDataItemRemoved(aDataItem) {
     this._itemCount--;
     this._updateViews();
   },
 
-  /**
-   * Returns the view item associated with the provided data item for this view.
-   *
-   * @param aDataItem
-   *        DownloadsDataItem object for which the view item is requested.
-   *
-   * @return Object that can be used to notify item status events.
-   */
-  getViewItem(aDataItem) {
-    let data = this._isPrivate ? PrivateDownloadsIndicatorData
-                               : DownloadsIndicatorData;
-    return Object.freeze({
-      onStateChange(aOldState) {
-        if (aDataItem.state == nsIDM.DOWNLOAD_FINISHED ||
-            aDataItem.state == nsIDM.DOWNLOAD_FAILED) {
-          data.attention = true;
-        }
+  // DownloadsView
+  onDataItemStateChanged(aDataItem, aOldState) {
+    if (aDataItem.state == nsIDM.DOWNLOAD_FINISHED ||
+        aDataItem.state == nsIDM.DOWNLOAD_FAILED) {
+      this.attention = true;
+    }
 
-        // Since the state of a download changed, reset the estimated time left.
-        data._lastRawTimeLeft = -1;
-        data._lastTimeLeft = -1;
+    // Since the state of a download changed, reset the estimated time left.
+    this._lastRawTimeLeft = -1;
+    this._lastTimeLeft = -1;
+  },
 
-        data._updateViews();
-      },
-      onProgressChange() {
-        data._updateViews();
-      }
-    });
+  // DownloadsView
+  onDataItemChanged() {
+    this._updateViews();
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Propagation of properties to our views
 
   // The following properties are updated by _refreshProperties and are then
   // propagated to the views.  See _refreshProperties for details.
   _hasDownloads: false,
@@ -1619,29 +1616,26 @@ DownloadsSummaryData.prototype = {
   },
 
   onDataItemRemoved(aDataItem) {
     let itemIndex = this._dataItems.indexOf(aDataItem);
     this._dataItems.splice(itemIndex, 1);
     this._updateViews();
   },
 
-  getViewItem(aDataItem) {
-    let self = this;
-    return Object.freeze({
-      onStateChange(aOldState) {
-        // Since the state of a download changed, reset the estimated time left.
-        self._lastRawTimeLeft = -1;
-        self._lastTimeLeft = -1;
-        self._updateViews();
-      },
-      onProgressChange() {
-        self._updateViews();
-      }
-    });
+  // DownloadsView
+  onDataItemStateChanged(aOldState) {
+    // Since the state of a download changed, reset the estimated time left.
+    this._lastRawTimeLeft = -1;
+    this._lastTimeLeft = -1;
+  },
+
+  // DownloadsView
+  onDataItemChanged() {
+    this._updateViews();
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Propagation of properties to our views
 
   /**
    * Computes aggregate values and propagates the changes to our views.
    */
--- a/browser/components/downloads/content/allDownloadsViewOverlay.js
+++ b/browser/components/downloads/content/allDownloadsViewOverlay.js
@@ -50,19 +50,18 @@ const NOT_AVAILABLE = Number.MAX_VALUE;
  * can be accessed through the |element| getter, and can then be inserted/removed from
  * a richlistbox.
  *
  * The shell doesn't take care of inserting the item, or removing it when it's no longer
  * valid. That's the caller (a DownloadsPlacesView object) responsibility.
  *
  * The caller is also responsible for "passing over" notification from both the
  * download-view and the places-result-observer, in the following manner:
- *  - The DownloadsPlacesView object implements getViewItem of the download-view
- *    pseudo interface.  It returns this object (therefore we implement
- *    onStateChangea and onProgressChange here).
+ *  - The DownloadsPlacesView object implements onDataItemStateChanged and
+ *    onDataItemChanged of the DownloadsView pseudo interface.
  *  - The DownloadsPlacesView object adds itself as a places result observer and
  *    calls this object's placesNodeIconChanged, placesNodeTitleChanged and
  *    placeNodeAnnotationChanged from its callbacks.
  *
  * @param [optional] aDataItem
  *        The data item of a the session download. Required if aPlacesNode is not set
  * @param [optional] aPlacesNode
  *        The places node for a past download. Required if aDataItem is not set.
@@ -563,40 +562,39 @@ DownloadElementShell.prototype = {
           // This will also update the download commands if necessary.
           this._targetFileInfoFetched = false;
           this._fetchTargetFileInfo();
         }
       }
     }
   },
 
-  /* DownloadView */
-  onStateChange(aOldState) {
+  onStateChanged(aOldState) {
     let metaData = this.getDownloadMetaData();
     metaData.state = this.dataItem.state;
     if (aOldState != nsIDM.DOWNLOAD_FINISHED && aOldState != metaData.state) {
       // See comment in DVI_onStateChange in downloads.js (the panel-view)
       this._element.setAttribute("image", this._getIcon() + "&state=normal");
       metaData.fileSize = this._dataItem.maxBytes;
       if (this._targetFileInfoFetched) {
         this._targetFileInfoFetched = false;
         this._fetchTargetFileInfo();
       }
     }
 
     this._updateDownloadStatusUI();
+
     if (this._element.selected) {
       goUpdateDownloadCommands();
     } else {
       goUpdateCommand("downloadsCmd_clearDownloads");
     }
   },
 
-  /* DownloadView */
-  onProgressChange() {
+  onChanged() {
     this._updateDownloadStatusUI();
   },
 
   /* nsIController */
   isCommandEnabled(aCommand) {
     // The only valid command for inactive elements is cmd_delete.
     if (!this.active && aCommand != "cmd_delete") {
       return false;
@@ -929,17 +927,17 @@ DownloadsPlacesView.prototype = {
     //    another shell for the download (so we have one shell for each data
     //    item).
     //
     // Note: If a cancelled session download is already in the list, and the
     // download is retired, onDataItemAdded is called again for the same
     // data item. Thus, we also check that we make sure we don't have a view item
     // already.
     if (!shouldCreateShell &&
-        aDataItem && this.getViewItem(aDataItem) == null) {
+        aDataItem && !this._viewItemsForDataItems.has(aDataItem)) {
       // If there's a past-download-only shell for this download-uri with no
       // associated data item, use it for the new data item. Otherwise, go ahead
       // and create another shell.
       shouldCreateShell = true;
       for (let shell of shellsForURI) {
         if (!shell.dataItem) {
           shouldCreateShell = false;
           shell.dataItem = aDataItem;
@@ -1046,17 +1044,17 @@ DownloadsPlacesView.prototype = {
   },
 
   _removeSessionDownloadFromView(aDataItem) {
     let shells = this._downloadElementsShellsForURI.get(aDataItem.uri);
     if (shells.size == 0) {
       throw new Error("Should have had at leaat one shell for this uri");
     }
 
-    let shell = this.getViewItem(aDataItem);
+    let shell = this._viewItemsForDataItems.get(aDataItem);
     if (!shells.has(shell)) {
       throw new Error("Missing download element shell in shells list for url");
     }
 
     // If there's more than one item for this download uri, we can let the
     // view item for this this particular data item go away.
     // If there's only one item for this download uri, we should only
     // keep it if it is associated with a history download.
@@ -1357,18 +1355,24 @@ DownloadsPlacesView.prototype = {
   onDataItemAdded(aDataItem, aNewest) {
     this._addDownloadData(aDataItem, null, aNewest);
   },
 
   onDataItemRemoved(aDataItem) {
     this._removeSessionDownloadFromView(aDataItem);
   },
 
-  getViewItem(aDataItem) {
-    return this._viewItemsForDataItems.get(aDataItem, null);
+  // DownloadsView
+  onDataItemStateChanged(aDataItem, aOldState) {
+    this._viewItemsForDataItems.get(aDataItem).onStateChanged(aOldState);
+  },
+
+  // DownloadsView
+  onDataItemChanged(aDataItem) {
+    this._viewItemsForDataItems.get(aDataItem).onChanged();
   },
 
   supportsCommand(aCommand) {
     if (DOWNLOAD_VIEW_SUPPORTED_COMMANDS.indexOf(aCommand) != -1) {
       // The clear-downloads command may be performed by the toolbar-button,
       // which can be focused on OS X.  Thus enable this command even if the
       // richlistbox is not focused.
       // For other commands, be prudent and disable them unless the richlistview
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -549,17 +549,17 @@ const DownloadsPanel = {
         DownloadsCommon.error("Downloads button cannot be found.");
         return;
       }
 
       // When the panel is opened, we check if the target files of visible items
       // still exist, and update the allowed items interactions accordingly.  We
       // do these checks on a background thread, and don't prevent the panel to
       // be displayed while these checks are being performed.
-      for each (let viewItem in DownloadsView._viewItems) {
+      for (let viewItem of DownloadsView._visibleViewItems.values()) {
         viewItem.verifyTargetExists();
       }
 
       DownloadsCommon.log("Opening downloads panel popup.");
       this.panel.openPopup(anchor, "bottomcenter topright", 0, 0, false, null);
     });
   },
 };
@@ -668,21 +668,21 @@ const DownloadsView = {
    * Ordered array of all DownloadsDataItem objects.  We need to keep this array
    * because only a limited number of items are shown at once, and if an item
    * that is currently visible is removed from the list, we might need to take
    * another item from the array and make it appear at the bottom.
    */
   _dataItems: [],
 
   /**
-   * Object containing the available DownloadsViewItem objects, indexed by their
-   * numeric download identifier.  There is a limited number of view items in
-   * the panel at any given time.
+   * Associates the visible DownloadsDataItem objects with their corresponding
+   * DownloadsViewItem object.  There is a limited number of view items in the
+   * panel at any given time.
    */
-  _viewItems: {},
+  _visibleViewItems: new Map(),
 
   /**
    * Called when the number of items in the list changes.
    */
   _itemCountChanged() {
     DownloadsCommon.log("The downloads item count has changed - we are tracking",
                         this._dataItems.length, "downloads in total.");
     let count = this._dataItems.length;
@@ -809,73 +809,64 @@ const DownloadsView = {
         // Reinsert the next item into the panel.
         this._addViewItem(this._dataItems[this.kItemCountLimit - 1], false);
       }
     }
 
     this._itemCountChanged();
   },
 
-  /**
-   * Returns the view item associated with the provided data item for this view.
-   *
-   * @param aDataItem
-   *        DownloadsDataItem object for which the view item is requested.
-   *
-   * @return Object that can be used to notify item status events.
-   */
-  getViewItem(aDataItem) {
-    // If the item is visible, just return it, otherwise return a mock object
-    // that doesn't react to notifications.
-    if (aDataItem.downloadGuid in this._viewItems) {
-      return this._viewItems[aDataItem.downloadGuid];
+  // DownloadsView
+  onDataItemStateChanged(aDataItem, aOldState) {
+    let viewItem = this._visibleViewItems.get(aDataItem);
+    if (viewItem) {
+      viewItem.onStateChanged(aOldState);
     }
-    return this._invisibleViewItem;
   },
 
-  /**
-   * Mock DownloadsDataItem object that doesn't react to notifications.
-   */
-  _invisibleViewItem: Object.freeze({
-    onStateChange() {},
-    onProgressChange() {},
-  }),
+  // DownloadsView
+  onDataItemChanged(aDataItem) {
+    let viewItem = this._visibleViewItems.get(aDataItem);
+    if (viewItem) {
+      viewItem.onChanged();
+    }
+  },
 
   /**
    * Creates a new view item associated with the specified data item, and adds
    * it to the top or the bottom of the list.
    */
   _addViewItem(aDataItem, aNewest)
   {
     DownloadsCommon.log("Adding a new DownloadsViewItem to the downloads list.",
                         "aNewest =", aNewest);
 
     let element = document.createElement("richlistitem");
     let viewItem = new DownloadsViewItem(aDataItem, element);
-    this._viewItems[aDataItem.downloadGuid] = viewItem;
+    this._visibleViewItems.set(aDataItem, viewItem);
     if (aNewest) {
       this.richListBox.insertBefore(element, this.richListBox.firstChild);
     } else {
       this.richListBox.appendChild(element);
     }
   },
 
   /**
    * Removes the view item associated with the specified data item.
    */
   _removeViewItem(aDataItem) {
     DownloadsCommon.log("Removing a DownloadsViewItem from the downloads list.");
-    let element = this.getViewItem(aDataItem)._element;
+    let element = this._visibleViewItems.get(aDataItem)._element;
     let previousSelectedIndex = this.richListBox.selectedIndex;
     this.richListBox.removeChild(element);
     if (previousSelectedIndex != -1) {
       this.richListBox.selectedIndex = Math.min(previousSelectedIndex,
                                                 this.richListBox.itemCount - 1);
     }
-    delete this._viewItems[aDataItem.downloadGuid];
+    this._visibleViewItems.delete(aDataItem);
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// User interface event functions
 
   /**
    * Helper function to do commands on a specific download item.
    *
@@ -1047,17 +1038,17 @@ DownloadsViewItem.prototype = {
   //////////////////////////////////////////////////////////////////////////////
   //// Callback functions from DownloadsData
 
   /**
    * Called when the download state might have changed.  Sometimes the state of
    * the download might be the same as before, if the data layer received
    * multiple events for the same download.
    */
-  onStateChange(aOldState) {
+  onStateChanged(aOldState) {
     // If a download just finished successfully, it means that the target file
     // now exists and we can extract its specific icon.  To ensure that the icon
     // is reloaded, we must change the URI used by the XUL image element, for
     // example by adding a query parameter.  Since this URI has a "moz-icon"
     // scheme, this only works if we add one of the parameters explicitly
     // supported by the nsIMozIconURI interface.
     if (aOldState != Ci.nsIDownloadManager.DOWNLOAD_FINISHED &&
         aOldState != this.dataItem.state) {
@@ -1067,24 +1058,22 @@ DownloadsViewItem.prototype = {
       // successfully, without checking the condition in the background.  If the
       // panel is already open, this will take effect immediately.  If the panel
       // is opened later, a new background existence check will be performed.
       this._element.setAttribute("exists", "true");
     }
 
     // Update the user interface after switching states.
     this._element.setAttribute("state", this.dataItem.state);
-    this._updateProgress();
-    this._updateStatusLine();
   },
 
   /**
    * Called when the download progress has changed.
    */
-  onProgressChange() {
+  onChanged() {
     this._updateProgress();
     this._updateStatusLine();
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Functions for updating the user interface
 
   /**