Bug 1115379 - Streamline DownloadsViewItemController construction and remove now unneeded identifiers. r=mak
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Wed, 14 Jan 2015 17:30:27 +0000
changeset 223842 85a8c3b8ad3837822bbe729f89888cebdfefe265
parent 223841 c10c7d4d04a2e69dac93085d325590164151b145
child 223843 76c43f71b2d7166786c7296c45db567684a85f8c
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
bugs1115379
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 1115379 - Streamline DownloadsViewItemController construction and remove now unneeded identifiers. r=mak
browser/components/downloads/DownloadsCommon.jsm
browser/components/downloads/content/downloads.js
browser/components/downloads/test/browser/browser_basic_functionality.js
--- a/browser/components/downloads/DownloadsCommon.jsm
+++ b/browser/components/downloads/DownloadsCommon.jsm
@@ -606,22 +606,18 @@ XPCOMUtils.defineLazyGetter(DownloadsCom
  *
  * Note that DownloadsData and PrivateDownloadsData are two equivalent singleton
  * objects, one accessing non-private downloads, and the other accessing private
  * ones.
  */
 function DownloadsDataCtor(aPrivate) {
   this._isPrivate = aPrivate;
 
-  // This Object contains all the available DownloadsDataItem objects, indexed by
-  // their globally unique identifier.  The identifiers of downloads that have
-  // been removed from the Download Manager data are still present, however the
-  // associated objects are replaced with the value "null".  This is required to
-  // prevent race conditions when populating the list asynchronously.
-  this.dataItems = {};
+  // Contains all the available DownloadsDataItem objects.
+  this.dataItems = new Set();
 
   // Array of view objects that should be notified when the available download
   // data changes.
   this._views = [];
 
   // Maps Download objects to DownloadDataItem objects.
   this._downloadToDataItemMap = new Map();
 }
@@ -639,18 +635,18 @@ DownloadsDataCtor.prototype = {
     }
   },
   _dataLinkInitialized: false,
 
   /**
    * True if there are finished downloads that can be removed from the list.
    */
   get canRemoveFinished() {
-    for (let [, dataItem] of Iterator(this.dataItems)) {
-      if (dataItem && !dataItem.inProgress) {
+    for (let dataItem of this.dataItems) {
+      if (!dataItem.inProgress) {
         return true;
       }
     }
     return false;
   },
 
   /**
    * Asks the back-end to remove finished downloads from the list.
@@ -663,17 +659,17 @@ DownloadsDataCtor.prototype = {
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Integration with the asynchronous Downloads back-end
 
   onDownloadAdded(aDownload) {
     let dataItem = new DownloadsDataItem(aDownload);
     this._downloadToDataItemMap.set(aDownload, dataItem);
-    this.dataItems[dataItem.downloadGuid] = dataItem;
+    this.dataItems.add(dataItem);
 
     for (let view of this._views) {
       view.onDataItemAdded(dataItem, true);
     }
 
     this._updateDataItemState(dataItem);
   },
 
@@ -690,17 +686,17 @@ DownloadsDataCtor.prototype = {
   onDownloadRemoved(aDownload) {
     let dataItem = this._downloadToDataItemMap.get(aDownload);
     if (!dataItem) {
       Cu.reportError("Download doesn't exist.");
       return;
     }
 
     this._downloadToDataItemMap.delete(aDownload);
-    this.dataItems[dataItem.downloadGuid] = null;
+    this.dataItems.delete(dataItem);
     for (let view of this._views) {
       view.onDataItemRemoved(dataItem);
     }
   },
 
   /**
    * Updates the given data item and sends related notifications.
    */
@@ -796,19 +792,17 @@ DownloadsDataCtor.prototype = {
    *        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 loadedItemsArray = [dataItem
-                            for each (dataItem in this.dataItems)
-                            if (dataItem)];
+    let loadedItemsArray = [...this.dataItems];
     loadedItemsArray.sort((a, b) => b.startTime - a.startTime);
     loadedItemsArray.forEach(dataItem => aView.onDataItemAdded(dataItem, false));
 
     // Notify the view that all data is available.
     aView.onDataLoadCompleted();
   },
 
   //////////////////////////////////////////////////////////////////////////////
@@ -876,34 +870,26 @@ XPCOMUtils.defineLazyGetter(this, "Downl
  * The endTime property is initialized to the current date and time.
  *
  * @param aDownload
  *        The Download object with the current state.
  */
 function DownloadsDataItem(aDownload) {
   this._download = aDownload;
 
-  this.downloadGuid = "id:" + this._autoIncrementId;
   this.file = aDownload.target.path;
   this.target = OS.Path.basename(aDownload.target.path);
   this.uri = aDownload.source.url;
   this.endTime = Date.now();
 
   this.updateFromDownload();
 }
 
 DownloadsDataItem.prototype = {
   /**
-   * The JavaScript API does not need identifiers for Download objects, so they
-   * are generated sequentially for the corresponding DownloadDataItem.
-   */
-  get _autoIncrementId() ++DownloadsDataItem.prototype.__lastId,
-  __lastId: 0,
-
-  /**
    * Updates this object from the underlying Download object.
    */
   updateFromDownload() {
     // Collapse state using the correct priority.
     if (this._download.succeeded) {
       this.state = nsIDM.DOWNLOAD_FINISHED;
     } else if (this._download.error &&
                this._download.error.becauseBlockedByParentalControls) {
@@ -1472,17 +1458,17 @@ DownloadsIndicatorDataCtor.prototype = {
    * A generator function for the dataItems that this summary is currently
    * interested in. This generator is passed off to summarizeDownloads in order
    * to generate statistics about the dataItems we care about - in this case,
    * it's all dataItems for active downloads.
    */
   _activeDataItems() {
     let dataItems = this._isPrivate ? PrivateDownloadsData.dataItems
                                     : DownloadsData.dataItems;
-    for each (let dataItem in dataItems) {
+    for (let dataItem of dataItems) {
       if (dataItem && dataItem.inProgress) {
         yield dataItem;
       }
     }
   },
 
   /**
    * Computes aggregate values based on the current state of downloads.
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -826,27 +826,39 @@ const DownloadsView = {
   onDataItemChanged(aDataItem) {
     let viewItem = this._visibleViewItems.get(aDataItem);
     if (viewItem) {
       viewItem.onChanged();
     }
   },
 
   /**
+   * Associates each richlistitem for a download with its corresponding
+   * DownloadsViewItemController object.
+   */
+  _controllersForElements: new Map(),
+
+  controllerForElement(element) {
+    return this._controllersForElements.get(element);
+  },
+
+  /**
    * 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._visibleViewItems.set(aDataItem, viewItem);
+    let viewItemController = new DownloadsViewItemController(aDataItem);
+    this._controllersForElements.set(element, viewItemController);
     if (aNewest) {
       this.richListBox.insertBefore(element, this.richListBox.firstChild);
     } else {
       this.richListBox.appendChild(element);
     }
   },
 
   /**
@@ -857,16 +869,17 @@ const DownloadsView = {
     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);
     }
     this._visibleViewItems.delete(aDataItem);
+    this._controllersForElements.delete(element);
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// User interface event functions
 
   /**
    * Helper function to do commands on a specific download item.
    *
@@ -877,17 +890,17 @@ const DownloadsView = {
    * @param aCommand
    *        The command to be performed.
    */
   onDownloadCommand(aEvent, aCommand) {
     let target = aEvent.target;
     while (target.nodeName != "richlistitem") {
       target = target.parentNode;
     }
-    new DownloadsViewItemController(target).doCommand(aCommand);
+    DownloadsView.controllerForElement(target).doCommand(aCommand);
   },
 
   onDownloadClick(aEvent) {
     // Handle primary clicks only, and exclude the action button.
     if (aEvent.button == 0 &&
         !aEvent.originalTarget.hasAttribute("oncommand")) {
       goDoCommand("downloadsCmd_open");
     }
@@ -951,18 +964,18 @@ const DownloadsView = {
   },
 
   onDownloadDragStart(aEvent) {
     let element = this.richListBox.selectedItem;
     if (!element) {
       return;
     }
 
-    let controller = new DownloadsViewItemController(element);
-    let localFile = controller.dataItem.localFile;
+    let localFile = DownloadsView.controllerForElement(element)
+                                 .dataItem.localFile;
     if (!localFile.exists()) {
       return;
     }
 
     let dataTransfer = aEvent.dataTransfer;
     dataTransfer.mozSetDataAt("application/x-moz-file", localFile, 0);
     dataTransfer.effectAllowed = "copyMove";
     var url = Services.io.newFileURI(localFile).spec;
@@ -996,18 +1009,16 @@ function DownloadsViewItem(aDataItem, aE
   // as bug 239948 comment 12 is handled, the "file" property will be always a
   // file URL rather than a file name.  At that point we should remove the "//"
   // (double slash) from the icon URI specification (see test_moz_icon_uri.js).
   this.image = "moz-icon://" + this.dataItem.file + "?size=32";
 
   let attributes = {
     "type": "download",
     "class": "download-state",
-    "id": "downloadsItem_" + this.dataItem.downloadGuid,
-    "downloadGuid": this.dataItem.downloadGuid,
     "state": this.dataItem.state,
     "progress": this.dataItem.inProgress ? this.dataItem.percentComplete : 100,
     "target": this.dataItem.target,
     "image": this.image
   };
 
   for (let attributeName in attributes) {
     this._element.setAttribute(attributeName, attributes[attributeName]);
@@ -1267,32 +1278,32 @@ const DownloadsViewController = {
   isCommandEnabled(aCommand) {
     // Handle commands that are not selection-specific.
     if (aCommand == "downloadsCmd_clearList") {
       return DownloadsCommon.getData(window).canRemoveFinished;
     }
 
     // Other commands are selection-specific.
     let element = DownloadsView.richListBox.selectedItem;
-    return element &&
-           new DownloadsViewItemController(element).isCommandEnabled(aCommand);
+    return element && DownloadsView.controllerForElement(element)
+                                   .isCommandEnabled(aCommand);
   },
 
   doCommand(aCommand) {
     // If this command is not selection-specific, execute it.
     if (aCommand in this.commands) {
       this.commands[aCommand].apply(this);
       return;
     }
 
     // Other commands are selection-specific.
     let element = DownloadsView.richListBox.selectedItem;
     if (element) {
       // The doCommand function also checks if the command is enabled.
-      new DownloadsViewItemController(element).doCommand(aCommand);
+      DownloadsView.controllerForElement(element).doCommand(aCommand);
     }
   },
 
   onEvent() {},
 
   //////////////////////////////////////////////////////////////////////////////
   //// Other functions
 
@@ -1318,19 +1329,18 @@ const DownloadsViewController = {
 
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadsViewItemController
 
 /**
  * Handles all the user interaction events, in particular the "commands",
  * related to a single item in the downloads list widgets.
  */
-function DownloadsViewItemController(aElement) {
-  let downloadGuid = aElement.getAttribute("downloadGuid");
-  this.dataItem = DownloadsCommon.getData(window).dataItems[downloadGuid];
+function DownloadsViewItemController(aDataItem) {
+  this.dataItem = aDataItem;
 }
 
 DownloadsViewItemController.prototype = {
   //////////////////////////////////////////////////////////////////////////////
   //// Command dispatching
 
   /**
    * The DownloadDataItem controlled by this object.
--- a/browser/components/downloads/test/browser/browser_basic_functionality.js
+++ b/browser/components/downloads/test/browser/browser_basic_functionality.js
@@ -44,12 +44,12 @@ add_task(function* test_basic_functional
   let richlistbox = document.getElementById("downloadsListBox");
   /* disabled for failing intermittently (bug 767828)
     is(richlistbox.children.length, DownloadData.length,
        "There is the correct number of richlistitems");
   */
   let itemCount = richlistbox.children.length;
   for (let i = 0; i < itemCount; i++) {
     let element = richlistbox.children[itemCount - i - 1];
-    let dataItem = new DownloadsViewItemController(element).dataItem;
+    let dataItem = DownloadsView.controllerForElement(element).dataItem;
     is(dataItem.state, DownloadData[i].state, "Download states match up");
   }
 });