Bug 822343 - Downloads view takes far too long to render. Step 1 - fix some trivial stuff (use document fragments and resue nsiuri objects). This alone has somewhat limited impact on performance. r=mak
authorAsaf Romano <mano@mozilla.com>
Wed, 19 Dec 2012 07:18:56 +0000
changeset 125617 46b1c88b81c2cf9748686d93bb1dcd675341f879
parent 125616 3ef80b6f262298b14851b41bf754aebf46657f12
child 125618 3dead209468440b4bcaf50796bf66baa133c31cd
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs822343
milestone20.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 822343 - Downloads view takes far too long to render. Step 1 - fix some trivial stuff (use document fragments and resue nsiuri objects). This alone has somewhat limited impact on performance. r=mak
browser/components/places/content/downloadsView.js
--- a/browser/components/places/content/downloadsView.js
+++ b/browser/components/places/content/downloadsView.js
@@ -113,16 +113,22 @@ DownloadElementShell.prototype = {
   get downloadURI() {
     if (this._dataItem)
      return this._dataItem.uri;
     if (this._placesNode)
       return this._placesNode.uri;
     throw new Error("Unexpected download element state");
   },
 
+  get _downloadURIObj() {
+    if (!("__downloadURIObj" in this))
+      this.__downloadURIObj = NetUtil.newURI(this.downloadURI);
+    return this.__downloadURIObj;
+  },
+
   get _icon() {
     if (this._targetFileURI)
       return "moz-icon://" + this._targetFileURI + "?size=32";
     if (this._placesNode)
       return this.placesNode.icon;
     if (this._dataItem)
       throw new Error("Session-download items should always have a target file uri");
     throw new Error("Unexpected download element state");
@@ -131,17 +137,17 @@ DownloadElementShell.prototype = {
   // Helper for getting a places annotation set for the download.
   _getAnnotation: function DES__getAnnotation(aAnnotation, aDefaultValue) {
     if (this._annotations.has(aAnnotation))
       return this._annotations.get(aAnnotation);
 
     let value;
     try {
       value = PlacesUtils.annotations.getPageAnnotation(
-        NetUtil.newURI(this.downloadURI), aAnnotation);
+        this._downloadURIObj, aAnnotation);
     }
     catch(ex) {
       if (aDefaultValue === undefined) {
         throw new Error("Could not get required annotation '" + aAnnotation +
                         "' for download with url '" + this.downloadURI + "'");
       }
       value = aDefaultValue;
     }
@@ -461,17 +467,17 @@ DownloadElementShell.prototype = {
       case "downloadsCmd_cancel": {
         this._dataItem.cancel();
         break;
       }
       case "cmd_delete": {
         if (this._dataItem)
           this._dataItem.remove();
         if (this._placesNode)
-          PlacesUtils.bhistory.removePage(NetUtil.newURI(this.downloadURI));
+          PlacesUtils.bhistory.removePage(this._downloadURIObj);
         break;
        }
       case "downloadsCmd_retry": {
         if (this._dataItem)
           this._dataItem.retry();
         else
           this._retryAsHistoryDownload();
         break;
@@ -572,28 +578,45 @@ DownloadsPlacesView.prototype = {
     if (this._downloadElementsShellsForURI.has(aURI)) {
       let downloadElementShells = this._downloadElementsShellsForURI.get(aURI);
       for (let des of downloadElementShells) {
         aCallback(des);
       }
     }
   },
 
-  // Given a data item for a session download, or a places node for a past
-  // download, updates the view as necessary.
-  // 1. If the given data is a places node, we check whether there are any
-  //    element for the same download url. If there are, then we just reset
-  //    their places node. Otherwise we add a new download element.
-  // 2. If the given data is a data item, we first check if there's an history
-  //    download in the list that is not associated with a data item. If we found
-  //    one, we use it for the data item as well and reposition it alongside the
-  //    other session downloads. If we don't, then we go ahead and create a new
-  //    element for the download.
+  /**
+   * Given a data item for a session download, or a places node for a past
+   * download, updates the view as necessary.
+   *  1. If the given data is a places node, we check whether there are any
+   *     elements for the same download url. If there are, then we just reset
+   *     their places node. Otherwise we add a new download element.
+   *  2. If the given data is a data item, we first check if there's a history
+   *     download in the list that is not associated with a data item. If we
+   *     found one, we use it for the data item as well and reposition it
+   *     alongside the other session downloads. If we don't, then we go ahead
+   *     and create a new element for the download.
+   *
+   * @param aDataItem
+   *        The data item of a session download. Set to null for history
+   *        downloads data.
+   * @param [optional] aPlacesNode
+   *        The places node for a history download. Required if there's no data
+   *        item.
+   * @param [optional] aNewest
+   *        @see onDataItemAdded. Ignored for history downlods.
+   * @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:
-  function DPV_addDownload(aDataItem, aPlacesNode, aNewest) {
+  function DPV_addDownload(aDataItem, aPlacesNode, aNewest = false, aDocumentFragment = null) {
     let downloadURI = aPlacesNode ? aPlacesNode.uri : aDataItem.uri;
     let shellsForURI = this._downloadElementsShellsForURI.get(downloadURI, null);
     if (!shellsForURI) {
       shellsForURI = new Set();
       this._downloadElementsShellsForURI.set(downloadURI, shellsForURI);
     }
 
     let newOrUpdatedShell = null;
@@ -653,22 +676,23 @@ DownloadsPlacesView.prototype = {
         this._richlistbox.insertBefore(newOrUpdatedShell.element,
                                        this._richlistbox.firstChild);
         if (!this._lastSessionDownloadElement) {
           this._lastSessionDownloadElement = newOrUpdatedShell.element;
         }
       }
       else if (aDataItem) {
         let before = this._lastSessionDownloadElement ?
-          this._lastSessionDownloadElement.nextSibling : this._richlistbox.firstChild
-        this._richlistbox.insertBefore(newOrUpdatedShell.element, before)
+          this._lastSessionDownloadElement.nextSibling : this._richlistbox.firstChild;
+        this._richlistbox.insertBefore(newOrUpdatedShell.element, before);
         this._lastSessionDownloadElement = newOrUpdatedShell.element;
       }
       else {
-        this._richlistbox.appendChild(newOrUpdatedShell.element);
+        let appendTo = aDocumentFragment || this._richlistbox;
+        appendTo.appendChild(newOrUpdatedShell.element);
       }
 
       if (this.searchTerm) {
         newOrUpdatedShell.element.hidden =
           !newOrUpdatedShell.element._shell.matchesSearchTerm(this.searchTerm);
       }
     }
   },
@@ -816,28 +840,32 @@ DownloadsPlacesView.prototype = {
 
     // Remove the invalidated history downloads from the list and unset the
     // places node for data downloads.
     for (let element of this._richlistbox.childNodes) {
       if (element._shell.placesNode)
         this._removeHistoryDownloadFromView(element._shell.placesNode);
     }
 
+    let elementsToAppendFragment = document.createDocumentFragment();
     for (let i = 0; i < aContainer.childCount; i++) {
       try {
-        this._addDownloadData(null, aContainer.getChild(i), false)
+        this._addDownloadData(null, aContainer.getChild(i), false,
+                              elementsToAppendFragment);
       }
       catch(ex) {
         Cu.reportError(ex);
       }
     }
+
+    this._richlistbox.appendChild(elementsToAppendFragment);
   },
 
   nodeInserted: function DPV_nodeInserted(aParent, aPlacesNode) {
-    this._addDownloadData(null, aPlacesNode, false);
+    this._addDownloadData(null, aPlacesNode);
   },
 
   nodeRemoved: function DPV_nodeRemoved(aParent, aPlacesNode, aOldIndex) {
     this._removeHistoryDownloadFromView(aPlacesNode);
   },
 
   nodeIconChanged: function DPV_nodeIconChanged(aNode) {
     this._forEachDownloadElementShellForURI(aNode.uri, function(aDownloadElementShell) {
@@ -890,17 +918,17 @@ DownloadsPlacesView.prototype = {
   onDataLoadStarting: function() { },
   onDataLoadCompleted: function() { },
 
   onDataItemAdded: function DPV_onDataItemAdded(aDataItem, aNewest) {
     this._addDownloadData(aDataItem, null, aNewest);
   },
 
   onDataItemRemoved: function DPV_onDataItemRemoved(aDataItem) {
-    this._removeSessionDownloadFromView(aDataItem)
+    this._removeSessionDownloadFromView(aDataItem);
   },
 
   getViewItem: function(aDataItem)
     this._viewItemsForDataItems.get(aDataItem, null),
 
   supportsCommand: function(aCommand)
     DOWNLOAD_VIEW_SUPPORTED_COMMANDS.indexOf(aCommand) != -1,
 
@@ -934,17 +962,17 @@ DownloadsPlacesView.prototype = {
                 createInstance(Ci.nsITransferable);
     trans.init(null);
 
     let flavors = ["text/x-moz-url", "text/unicode"];
     flavors.forEach(trans.addDataFlavor);
 
     Services.clipboard.getData(trans, Services.clipboard.kGlobalClipboard);
 
-    // Getting the data or creating the nsIURI might fail
+    // Getting the data or creating the nsIURI might fail.
     try {
       let data = {};
       trans.getAnyTransferData({}, data, {});
       let [url, name] = data.value.QueryInterface(Ci.nsISupportsString)
                             .data.split("\n");
       if (url)
         return [NetUtil.newURI(url, null, null).spec, name];
     }