Bug 747903 - Limit the number of items in the Downloads Panel. r=mano
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Sat, 25 Aug 2012 11:37:55 +0200
changeset 103389 52540206fb9167b7224480906e5184b6d21ab9d1
parent 103388 a86ecc9f22ad8e2fb8dda4e1aa3490dbcb3eba22
child 103390 3d3e80ee5ec181003376ba5621fe2ace2d89413e
push id13969
push userpaolo.mozmail@amadzone.org
push dateSat, 25 Aug 2012 09:39:24 +0000
treeherdermozilla-inbound@3d3e80ee5ec1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmano
bugs747903
milestone17.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 747903 - Limit the number of items in the Downloads Panel. r=mano
browser/components/downloads/content/downloads.js
browser/components/downloads/content/downloadsOverlay.xul
browser/components/downloads/src/DownloadsCommon.jsm
browser/components/downloads/test/browser/browser_basic_functionality.js
browser/locales/en-US/chrome/browser/downloads/downloads.dtd
browser/locales/en-US/chrome/browser/downloads/downloads.properties
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -351,17 +351,17 @@ const DownloadsOverlayLoader = {
   _loadedOverlays: {},
 
   /**
    * Loads the specified overlay and invokes the given callback when finished.
    *
    * @param aOverlay
    *        String containing the URI of the overlay to load in the current
    *        window.  If this overlay has already been loaded using this
-   *        function, then the overlay is not loaded again. 
+   *        function, then the overlay is not loaded again.
    * @param aCallback
    *        Invoked when loading is completed.  If the overlay is already
    *        loaded, the function is called immediately.
    */
   ensureOverlayLoaded: function DOL_ensureOverlayLoaded(aOverlay, aCallback)
   {
     // The overlay is already loaded, invoke the callback immediately.
     if (aOverlay in this._loadedOverlays) {
@@ -420,47 +420,79 @@ const DownloadsOverlayLoader = {
  * download state and real-time data.  In addition, handles part of the user
  * interaction events raised by the downloads list widget.
  */
 const DownloadsView = {
   //////////////////////////////////////////////////////////////////////////////
   //// Functions handling download items in the list
 
   /**
+   * Maximum number of items shown by the list at any given time.
+   */
+  kItemCountLimit: 3,
+
+  /**
    * Indicates whether we are still loading downloads data asynchronously.
    */
   loading: false,
 
   /**
-   * Object containing all the available DownloadsViewItem objects, indexed by
-   * their numeric download identifier.
+   * 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.
    */
   _viewItems: {},
 
   /**
    * Called when the number of items in the list changes.
    */
   _itemCountChanged: function DV_itemCountChanged()
   {
-    if (Object.keys(this._viewItems).length > 0) {
+    let count = this._dataItems.length;
+    let hiddenCount = count - this.kItemCountLimit;
+
+    if (count > 0) {
       DownloadsPanel.panel.setAttribute("hasdownloads", "true");
     } else {
       DownloadsPanel.panel.removeAttribute("hasdownloads");
     }
+
+    let s = DownloadsCommon.strings;
+    this.downloadsHistory.label = (hiddenCount > 0)
+                                  ? s.showMoreDownloads(hiddenCount)
+                                  : s.showAllDownloads;
+    this.downloadsHistory.accessKey = s.showDownloadsAccessKey;
   },
 
   /**
    * Element corresponding to the list of downloads.
    */
   get richListBox()
   {
     delete this.richListBox;
     return this.richListBox = document.getElementById("downloadsListBox");
   },
 
+  /**
+   * Element corresponding to the button for showing more downloads.
+   */
+  get downloadsHistory()
+  {
+    delete this.downloadsHistory;
+    return this.downloadsHistory = document.getElementById("downloadsHistory");
+  },
+
   //////////////////////////////////////////////////////////////////////////////
   //// Callback functions from DownloadsData
 
   /**
    * Called before multiple downloads are about to be loaded.
    */
   onDataLoadStarting: function DV_onDataLoadStarting()
   {
@@ -469,16 +501,20 @@ const DownloadsView = {
 
   /**
    * Called after data loading finished.
    */
   onDataLoadCompleted: function DV_onDataLoadCompleted()
   {
     this.loading = false;
 
+    // We suppressed item count change notifications during the batch load, at
+    // this point we should just call the function once.
+    this._itemCountChanged();
+
     // Notify the panel that all the initially available downloads have been
     // loaded.  This ensures that the interface is visible, if still required.
     DownloadsPanel.onViewLoadCompleted();
   },
 
   /**
    * Called when the downloads database becomes unavailable (for example,
    * entering Private Browsing Mode).  References to existing data should be
@@ -488,16 +524,17 @@ const DownloadsView = {
   {
     DownloadsPanel.terminate();
 
     // Clear the list by replacing with a shallow copy.
     let emptyView = this.richListBox.cloneNode(false);
     this.richListBox.parentNode.replaceChild(emptyView, this.richListBox);
     this.richListBox = emptyView;
     this._viewItems = {};
+    this._dataItems = [];
   },
 
   /**
    * Called when a new download data item is available, either during the
    * asynchronous data load or when a new download is started.
    *
    * @param aDataItem
    *        DownloadsDataItem object that was just added.
@@ -505,59 +542,119 @@ const DownloadsView = {
    *        When true, indicates that this item is the most recent and should be
    *        added in the topmost position.  This happens when a new download is
    *        started.  When false, indicates that the item is the least recent
    *        and should be appended.  The latter generally happens during the
    *        asynchronous data load.
    */
   onDataItemAdded: function DV_onDataItemAdded(aDataItem, aNewest)
   {
-    // Make the item and add it in the appropriate place in the list.
-    let element = document.createElement("richlistitem");
-    let viewItem = new DownloadsViewItem(aDataItem, element);
-    this._viewItems[aDataItem.downloadId] = viewItem;
     if (aNewest) {
-      this.richListBox.insertBefore(element, this.richListBox.firstChild);
+      this._dataItems.unshift(aDataItem);
     } else {
-      this.richListBox.appendChild(element);
+      this._dataItems.push(aDataItem);
     }
 
-    this._itemCountChanged();
+    let itemsNowOverflow = this._dataItems.length > this.kItemCountLimit;
+    if (aNewest || !itemsNowOverflow) {
+      // The newly added item is visible in the panel and we must add the
+      // corresponding element.  This is either because it is the first item, or
+      // because it was added at the bottom but the list still doesn't overflow.
+      this._addViewItem(aDataItem, aNewest);
+    }
+    if (aNewest && itemsNowOverflow) {
+      // If the list overflows, remove the last item from the panel to make room
+      // for the new one that we just added at the top.
+      this._removeViewItem(this._dataItems[this.kItemCountLimit]);
+    }
+
+    // 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();
+    }
   },
 
   /**
    * Called when a data item is removed.  Ensures that the widget associated
    * with the view item is removed from the user interface.
    *
    * @param aDataItem
    *        DownloadsDataItem object that is being removed.
    */
   onDataItemRemoved: function DV_onDataItemRemoved(aDataItem)
   {
-    let element = this.getViewItem(aDataItem)._element;
-    let previousSelectedIndex = this.richListBox.selectedIndex;
-    this.richListBox.removeChild(element);
-    this.richListBox.selectedIndex = Math.min(previousSelectedIndex,
-                                              this.richListBox.itemCount - 1);
-    delete this._viewItems[aDataItem.downloadId];
+    let itemIndex = this._dataItems.indexOf(aDataItem);
+    this._dataItems.splice(itemIndex, 1);
+
+    if (itemIndex < this.kItemCountLimit) {
+      // The item to remove is visible in the panel.
+      this._removeViewItem(aDataItem);
+      if (this._dataItems.length >= this.kItemCountLimit) {
+        // 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: function DV_getViewItem(aDataItem)
   {
-    return this._viewItems[aDataItem.downloadId];
+    // If the item is visible, just return it, otherwise return a mock object
+    // that doesn't react to notifications.
+    if (aDataItem.downloadId in this._viewItems) {
+      return this._viewItems[aDataItem.downloadId];
+    }
+    return this._invisibleViewItem;
+  },
+
+  /**
+   * Mock DownloadsDataItem object that doesn't react to notifications.
+   */
+  _invisibleViewItem: Object.freeze({
+    onStateChange: function () { },
+    onProgressChange: function () { }
+  }),
+
+  /**
+   * Creates a new view item associated with the specified data item, and adds
+   * it to the top or the bottom of the list.
+   */
+  _addViewItem: function DV_addViewItem(aDataItem, aNewest)
+  {
+    let element = document.createElement("richlistitem");
+    let viewItem = new DownloadsViewItem(aDataItem, element);
+    this._viewItems[aDataItem.downloadId] = 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: function DV_removeViewItem(aDataItem)
+  {
+    let element = this.getViewItem(aDataItem)._element;
+    let previousSelectedIndex = this.richListBox.selectedIndex;
+    this.richListBox.removeChild(element);
+    this.richListBox.selectedIndex = Math.min(previousSelectedIndex,
+                                              this.richListBox.itemCount - 1);
+    delete this._viewItems[aDataItem.downloadId];
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// User interface event functions
 
   /**
    * Helper function to do commands on a specific download item.
    *
--- a/browser/components/downloads/content/downloadsOverlay.xul
+++ b/browser/components/downloads/content/downloadsOverlay.xul
@@ -103,14 +103,12 @@
                    flex="1"
                    context="downloadsContextMenu"
                    onkeypress="DownloadsView.onDownloadKeyPress(event);"
                    oncontextmenu="DownloadsView.onDownloadContextMenu(event);"
                    ondragstart="DownloadsView.onDownloadDragStart(event);"/>
 
       <button id="downloadsHistory"
               class="plain"
-              label="&downloadshistory.label;"
-              accesskey="&downloadshistory.accesskey;"
               oncommand="DownloadsPanel.showDownloadsHistory();"/>
     </panel>
   </popupset>
 </overlay>
--- a/browser/components/downloads/src/DownloadsCommon.jsm
+++ b/browser/components/downloads/src/DownloadsCommon.jsm
@@ -47,16 +47,18 @@ const Cr = Components.results;
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(this, "gBrowserGlue",
                                    "@mozilla.org/browser/browserglue;1",
                                    "nsIBrowserGlue");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
+                                  "resource://gre/modules/PluralForm.jsm");
 
 const nsIDM = Ci.nsIDownloadManager;
 
 const kDownloadsStringBundleUrl =
   "chrome://browser/locale/downloads/downloads.properties";
 
 const kDownloadsStringsRequiringFormatting = {
   sizeWithUnits: true,
@@ -64,16 +66,20 @@ const kDownloadsStringsRequiringFormatti
   shortTimeLeftMinutes: true,
   shortTimeLeftHours: true,
   shortTimeLeftDays: true,
   statusSeparator: true,
   statusSeparatorBeforeNumber: true,
   fileExecutableSecurityWarning: true
 };
 
+const kDownloadsStringsRequiringPluralForm = {
+  showMoreDownloads: true
+};
+
 XPCOMUtils.defineLazyGetter(this, "DownloadsLocalFileCtor", function () {
   return Components.Constructor("@mozilla.org/file/local;1",
                                 "nsILocalFile", "initWithPath");
 });
 
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadsCommon
 
@@ -97,16 +103,24 @@ const DownloadsCommon = {
       let stringName = string.key;
       if (stringName in kDownloadsStringsRequiringFormatting) {
         strings[stringName] = function () {
           // Convert "arguments" to a real array before calling into XPCOM.
           return sb.formatStringFromName(stringName,
                                          Array.slice(arguments, 0),
                                          arguments.length);
         };
+      } else if (stringName in kDownloadsStringsRequiringPluralForm) {
+        strings[stringName] = function (aCount) {
+          // Convert "arguments" to a real array before calling into XPCOM.
+          let formattedString = sb.formatStringFromName(stringName,
+                                         Array.slice(arguments, 0),
+                                         arguments.length);
+          return PluralForm.get(aCount, formattedString);
+        };
       } else {
         strings[stringName] = string.value;
       }
     }
     delete this.strings;
     return this.strings = strings;
   },
 
@@ -430,33 +444,36 @@ const DownloadsData = {
 
     if (aActiveOnly) {
       if (this._loadState == this.kLoadNone) {
         // Indicate to the views that a batch loading operation is in progress.
         this._views.forEach(
           function (view) view.onDataLoadStarting()
         );
 
-        // Reload the list using the Download Manager service.
+        // Reload the list using the Download Manager service.  The list is
+        // returned in no particular order.
         let downloads = Services.downloads.activeDownloads;
         while (downloads.hasMoreElements()) {
           let download = downloads.getNext().QueryInterface(Ci.nsIDownload);
           this._getOrAddDataItem(download, true);
         }
         this._loadState = this.kLoadActive;
 
         // Indicate to the views that the batch loading operation is complete.
         this._views.forEach(
           function (view) view.onDataLoadCompleted()
         );
       }
     } else {
       if (this._loadState != this.kLoadAll) {
         // Load only the relevant columns from the downloads database.  The
-        // columns are read in the init_FromDataRow method of DownloadsDataItem.
+        // columns are read in the _initFromDataRow method of DownloadsDataItem.
+        // Order by descending download identifier so that the most recent
+        // downloads are notified first to the listening views.
         let statement = Services.downloads.DBConnection.createAsyncStatement(
           "SELECT id, target, name, source, referrer, state, "
         +        "startTime, endTime, currBytes, maxBytes "
         + "FROM moz_downloads "
         + "ORDER BY id DESC"
         );
         try {
           this._pendingStatement = statement.executeAsync(this);
--- a/browser/components/downloads/test/browser/browser_basic_functionality.js
+++ b/browser/components/downloads/test/browser/browser_basic_functionality.js
@@ -19,16 +19,23 @@ function gen_test()
     { endTime: 1180493839859234, state: nsIDM.DOWNLOAD_FINISHED },
     { endTime: 1180493839859233, state: nsIDM.DOWNLOAD_FAILED },
     { endTime: 1180493839859232, state: nsIDM.DOWNLOAD_CANCELED },
     { endTime: 1180493839859231, state: nsIDM.DOWNLOAD_BLOCKED_PARENTAL },
     { endTime: 1180493839859230, state: nsIDM.DOWNLOAD_DIRTY },
     { endTime: 1180493839859229, state: nsIDM.DOWNLOAD_BLOCKED_POLICY },
   ];
 
+  // For testing purposes, show all the download items at once.
+  var originalCountLimit = DownloadsView.kItemCountLimit;
+  DownloadsView.kItemCountLimit = DownloadData.length;
+  registerCleanupFunction(function () {
+    DownloadsView.kItemCountLimit = originalCountLimit;
+  });
+
   try {
     // Ensure that state is reset in case previous tests didn't finish.
     for (let yy in gen_resetState()) yield;
 
     // Populate the downloads database with the data required by this test.
     for (let yy in gen_addDownloadRows(DownloadData)) yield;
 
     // Open the user interface and wait for data to be fully loaded.
--- a/browser/locales/en-US/chrome/browser/downloads/downloads.dtd
+++ b/browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ -39,10 +39,8 @@
 <!ENTITY cmd.goToDownloadPage.accesskey   "G">
 <!ENTITY cmd.copyDownloadLink.label       "Copy Download Link">
 <!ENTITY cmd.copyDownloadLink.accesskey   "L">
 <!ENTITY cmd.removeFromList.label         "Remove From List">
 <!ENTITY cmd.removeFromList.accesskey     "e">
 <!ENTITY cmd.clearList.label              "Clear List">
 <!ENTITY cmd.clearList.accesskey          "a">
 
-<!ENTITY downloadshistory.label           "Show All Downloads">
-<!ENTITY downloadshistory.accesskey       "S">
--- a/browser/locales/en-US/chrome/browser/downloads/downloads.properties
+++ b/browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ -62,11 +62,25 @@ shortTimeLeftDays=%1$Sd
 # that we use a wider space after the separator when it is followed by a number,
 # just to avoid visually confusing it with with a minus sign with some fonts.
 # If you use a different separator, this might not be necessary.  However, there
 # is usually no need to change the separator or the order of the substitutions,
 # even for right-to-left languages, unless the defaults are not suitable.
 statusSeparator=%1$S \u2014 %2$S
 statusSeparatorBeforeNumber=%1$S \u2014  %2$S
 
+# LOCALIZATION NOTE (showMoreDownloads):
+# This string is shown in the Downloads Panel when there are more active
+# downloads than can fit in the available space.  The phrase should be read as
+# "Show N more of my recent downloads".  Use a semi-colon list of plural forms.
+# See: http://developer.mozilla.org/en/Localization_and_Plurals
+showMoreDownloads=Show 1 More Recent Download;Show %1$S More Recent Downloads
+# LOCALIZATION NOTE (showAllDownloads):
+# This string is shown in place of showMoreDownloads when all the downloads fit
+# in the available space, or when there are no downloads in the panel at all.
+showAllDownloads=Show All Downloads
+# LOCALIZATION NOTE (showDownloadsAccessKey):
+# This access key applies to both showMoreDownloads and showAllDownloads.
+showDownloadsAccessKey=S
+
 fileExecutableSecurityWarning="%S" is an executable file. Executable files may contain viruses or other malicious code that could harm your computer. Use caution when opening this file. Are you sure you want to launch "%S"?
 fileExecutableSecurityWarningTitle=Open Executable File?
 fileExecutableSecurityWarningDontAsk=Don't ask me this again