MD Downloads: track downloads in C++, dispatch discrete JS updates

chrome/browser/resources/md_downloads/manager.js

Index: chrome/browser/resources/md_downloads/manager.js
diff --git a/chrome/browser/resources/md_downloads/manager.js b/chrome/browser/resources/md_downloads/manager.js
index 21acd2941db98108ca9c29c4ab292964b56ae899..a4907ea98faeba23a757536cc44b6564bd5c77a1 100644
--- a/chrome/browser/resources/md_downloads/manager.js
+++ b/chrome/browser/resources/md_downloads/manager.js
@@ -8,15 +8,56 @@ cr.define('downloads', function() {
 
     properties: {
       hasDownloads_: {
+        observer: 'hasDownloadsChanged_',
         type: Boolean,
-        value: false,
       },
 
       items_: {
         type: Array,
+        value: function() { return []; },
       },
     },
 
+    observers: [
+      'itemsChanged_(items_.*)',
+    ],
+
+    /** @private */
+    clearAll_: function() {
+      this.set('items_', []);
+    },
+
+    /** @private */
+    hasDownloadsChanged_: function() {
+      if (loadTimeData.getBoolean('allowDeletingHistory'))
+        this.$.toolbar.downloadsShowing = this.hasDownloads_;
+
+      if (this.hasDownloads_) {
+        this.$['downloads-list'].fire('iron-resize');
+      } else {
+        var isSearching = downloads.ActionService.getInstance().isSearching();
+        var messageToShow = isSearching ? 'noSearchResults' : 'noDownloads';
+        this.$['no-downloads'].querySelector('span').textContent =
+            loadTimeData.getString(messageToShow);
+      }
+    },
+
+    /**
+     * @param {number} index
+     * @param {!Array<!downloads.Data>} list
+     * @private
+     */
+    insertItems_: function(index, list) {
+      this.splice.apply(this, ['items_', index, 0].concat(list));
+      this.updateHideDates_(index, index + list.length);
+      this.$.panel.classList.remove('loading');
+    },
+
+    /** @private */
+    itemsChanged_: function() {
+      this.hasDownloads_ = this.size_() > 0;
+    },
+
     /**
      * @param {Event} e
      * @private
@@ -55,6 +96,15 @@ cr.define('downloads', function() {
     },
 
     /**
+     * @param {number} index
+     * @private
+     */
+    removeItem_: function(index) {
+      this.splice('items_', index, 1);
+      this.updateHideDates_(index, index);
+    },
+
+    /**
      * @return {number} The number of downloads shown on the page.
      * @private
      */
@@ -63,70 +113,58 @@ cr.define('downloads', function() {
     },
 
     /**
-     * Called when all items need to be updated.
-     * @param {!Array<!downloads.Data>} list A list of new download data.
+     * @param {number} start
+     * @param {number} end
      * @private
      */
-    updateAll_: function(list) {
-      /** @private {!Object<number>} */
-      this.idToIndex_ = {};
-
-      for (var i = 0; i < list.length; ++i) {
-        var data = list[i];
-
-        this.idToIndex_[data.id] = data.index = i;
-
-        var prev = list[i - 1];
-        data.hideDate = !!prev && prev.date_string == data.date_string;
-      }
-
-      // TODO(dbeam): this resets the scroll position, which is a huge bummer.
-      // Removing something from the bottom of the list should not scroll you
-      // back to the top. The grand plan is to restructure how the C++ sends the
-      // JS data so that it only gets updates (rather than the most recent set
-      // of items). TL;DR - we can't ship with this bug.
-      this.items_ = list;
-
-      var hasDownloads = this.size_() > 0;
-      if (!hasDownloads) {
-        var isSearching = downloads.ActionService.getInstance().isSearching();
-        var messageToShow = isSearching ? 'noSearchResults' : 'noDownloads';
-        this.$['no-downloads'].querySelector('span').textContent =
-            loadTimeData.getString(messageToShow);
+    updateHideDates_: function(start, end) {
+      for (var i = start; i <= end; ++i) {
+        var current = this.items_[i];
+        if (!current)
+          continue;
+        var prev = this.items_[i - 1];
+        current.hideDate = !!prev && prev.date_string == current.date_string;
       }
-      this.hasDownloads_ = hasDownloads;
-
-      if (loadTimeData.getBoolean('allowDeletingHistory'))
-        this.$.toolbar.downloadsShowing = this.hasDownloads_;
-
-      this.$.panel.classList.remove('loading');
     },
 
     /**
+     * @param {number} index
      * @param {!downloads.Data} data
      * @private
      */
-    updateItem_: function(data) {
-      var index = this.idToIndex_[data.id];
+    updateItem_: function(index, data) {
       this.set('items_.' + index, data);
       this.$['downloads-list'].updateSizeForItem(index);
     },
   });
 
-  Manager.size = function() {
-    return document.querySelector('downloads-manager').size_();
+  Manager.clearAll = function() {
+    Manager.get().clearAll_();
   };
 
-  Manager.updateAll = function(list) {
-    document.querySelector('downloads-manager').updateAll_(list);
+  /** @return {downloads.Manager} */

[dpapad, 2015/11/20 21:55:15]

@return {!downloads.Manager}

[Dan Beam, 2015/11/21 01:40:43]

technically this can return null if the component is loaded but not
instantiated/attached (as I was doing in testing).  anyways, programmatically
enforcing via assert() now

+  Manager.get = function() {
+    return document.querySelector('downloads-manager');

[dpapad, 2015/11/20 21:55:15]

Can the result of this call be cached, to avoid searching document every time
get() is called?

[Dan Beam, 2015/11/21 01:40:43]

this seems like a premature optimization that may cause head banging in tests

   };
 
-  Manager.updateItem = function(item) {
-    document.querySelector('downloads-manager').updateItem_(item);
+  Manager.insertItems = function(index, list) {
+    Manager.get().insertItems_(index, list);
   };
 
   Manager.onLoad = function() {
-    document.querySelector('downloads-manager').onLoad_();
+    Manager.get().onLoad_();
+  };
+
+  Manager.removeItem = function(index) {
+    Manager.get().removeItem_(index);
+  };
+
+  Manager.size = function() {

[dpapad, 2015/11/20 23:00:19]

I couldn't find any uses of Manager.size(). Does it need to be exposed?

[Dan Beam, 2015/11/21 01:40:43]

Done.

+    return Manager.get().size_();
+  };
+
+  Manager.updateItem = function(index, data) {
+    Manager.get().updateItem_(index, data);
   };
 
   return {Manager: Manager};