Slice MD downloads into more components

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 ebd003f47fc0e37e53ba7f5370eda0efaa46291a..74aed83d4e5d299c67b5cab2b83832768bff8b78 100644
--- a/chrome/browser/resources/md_downloads/manager.js
+++ b/chrome/browser/resources/md_downloads/manager.js
@@ -6,40 +6,84 @@ cr.define('downloads', function() {
   /**
    * Class to own and manage download items.
    * @constructor
+   * @implements {downloads.ActionService}
    */
   function Manager() {}
 
   cr.addSingletonGetter(Manager);
 
+  /**
+   * @param {string} chromeSendName
+   * @return {function(string):void} A chrome.send() callback with curried name.
+   */
+  function chromeSendWithId(chromeSendName) {
+    return function(id) { chrome.send(chromeSendName, [id]); };
+  }
+
   Manager.prototype = {
-    /** @private {string} */
-    searchText_: '',
+    /** @override */
+    cancel: chromeSendWithId('cancel'),
 
-    /**
-     * Sets the search text, updates related UIs, and tells the browser.
-     * @param {string} searchText Text we're searching for.
-     * @private
-     */
-    setSearchText_: function(searchText) {
-      this.searchText_ = searchText;
+    /** @override */
+    clearAll: function() {
+      if (loadTimeData.getBoolean('allowDeletingHistory')) {
+        chrome.send('clearAll');
+        this.search('');
+      }
+    },
+
+    /** @override */
+    discardDangerous: chromeSendWithId('discardDangerous'),
+
+    /** @override */
+    drag: chromeSendWithId('drag'),
+
+    /** @override */
+    openDownloadsFolder: function() {
+      chrome.send('openDownloadsFolder');
+    },
+
+    /** @override */
+    openFile: chromeSendWithId('openFile'),
+
+    /** @override */
+    pause: chromeSendWithId('pause'),
 
-      $('downloads-summary-text').textContent = this.searchText_ ?
-          loadTimeData.getStringF('searchResultsFor', this.searchText_) : '';
+    /** @override */
+    remove: chromeSendWithId('remove'),
 
+    /** @override */
+    resume: chromeSendWithId('resume'),
+
+    /** @override */
+    saveDangerous: chromeSendWithId('saveDangerous'),
+
+    /** @override */
+    search: function(searchText) {
       // Split quoted terms (e.g., 'The "lazy" dog' => ['The', 'lazy', 'dog']).
       function trim(s) { return s.trim(); }
       chrome.send('getDownloads', searchText.split(/"([^"]*)"/).map(trim));
     },
 
+    /** @override */
+    show: chromeSendWithId('show'),
+
+    /**
+     * @return {boolean} Whether all can be cleared.
+     * @private
+     */
+    canClearAll_: function() {
+      return loadTimeData.getBoolean('allowDeletingHistory') &&
+             this.size_() > 0;
+    },
+
     /**
      * @return {number} A guess at how many items could be visible at once.
      * @private
      */
     guesstimateNumberOfVisibleItems_: function() {
-      var toolbarHeight = $('downloads-toolbar').offsetHeight;
-      var summaryHeight = $('downloads-summary').offsetHeight;
-      var nonItemSpace = toolbarHeight + summaryHeight;
-      return Math.floor((window.innerHeight - nonItemSpace) / 46) + 1;
+      var toolbarHeight = this.toolbar_.offsetHeight;
+      return Math.floor((window.innerHeight - toolbarHeight) / 46) + 1;
     },
 
     /**
@@ -67,7 +111,8 @@ cr.define('downloads', function() {
         var id = data.id;
 
         // Re-use old items when possible (saves work, preserves focus).
-        var item = oldIdMap[id] || new downloads.ItemView(this.iconLoader_);
+        var item =
+            oldIdMap[id] || new downloads.ItemView(this.iconLoader_, this);
 
         this.idMap_[id] = item;  // Associated by ID for fast lookup.
         this.items_.push(item);  // Add to sorted list for order.
@@ -102,19 +147,15 @@ cr.define('downloads', function() {
             before = this.items_[j];
         }
         // If |before| is null, |item| will just get added at the end.
-        this.node_.insertBefore(item, before);
+        this.list_.insertBefore(item, before);
       }
 
-      var noDownloadsOrResults = $('no-downloads-or-results');
-      noDownloadsOrResults.textContent = loadTimeData.getString(
-          this.searchText_ ? 'noSearchResults' : 'noDownloads');
-
       var hasDownloads = this.size_() > 0;
-      this.node_.hidden = !hasDownloads;
-      noDownloadsOrResults.hidden = hasDownloads;
+      this.list_.hidden = !hasDownloads;
+      $('no-downloads').hidden = hasDownloads;
 
       if (loadTimeData.getBoolean('allowDeletingHistory'))
-        $('clear-all').hidden = !hasDownloads || this.searchText_.length > 0;
+        this.toolbar_.canClearAll = hasDownloads;
     },
 
     /**
@@ -133,53 +174,6 @@ cr.define('downloads', function() {
       return this.items_.length;
     },
 
-    /** @private */
-    clearAll_: function() {
-      if (loadTimeData.getBoolean('allowDeletingHistory')) {
-        chrome.send('clearAll');
-        this.setSearchText_('');
-      }
-    },
-
-    /** @private */
-    onLoad_: function() {
-      this.node_ = $('downloads-display');
-
-      $('clear-all').onclick = function() {
-        this.clearAll_();
-      }.bind(this);
-
-      $('open-downloads-folder').onclick = function() {
-        chrome.send('openDownloadsFolder');
-      };
-
-      $('search-button').onclick = function() {
-        if (!$('search-term').hidden)
-          return;
-        $('clear-search').hidden = false;
-        $('search-term').hidden = false;
-      };
-
-      $('clear-search').onclick = function() {
-        $('clear-search').hidden = true;
-        $('search-term').hidden = true;
-        $('search-term').value = '';
-        this.setSearchText_('');
-      }.bind(this);
-
-      // TODO(dbeam): this previously used onsearch, which batches keystrokes
-      // together. This should probably be re-instated eventually.
-      $('search-term').oninput = function(e) {
-        this.setSearchText_($('search-term').value);
-      }.bind(this);
-
-      cr.ui.decorate('command', cr.ui.Command);
-      document.addEventListener('canExecute', this.onCanExecute_.bind(this));
-      document.addEventListener('command', this.onCommand_.bind(this));
-
-      this.setSearchText_('');
-    },
-
     /**
      * @param {Event} e
      * @private
@@ -188,7 +182,9 @@ cr.define('downloads', function() {
       e = /** @type {cr.ui.CanExecuteEvent} */(e);
       switch (e.command.id) {
         case 'undo-command':
-          e.canExecute = !$('search-term').contains(document.activeElement);
+          var activeElement = document.activeElement;
+          var searchTerm = this.toolbar_ && this.toolbar_.searchTerm;
+          e.canExecute = searchTerm && !searchTerm.contains(activeElement);

[Jeremy Klein, 2015/07/16 18:54:53]

This is a cool trick for determining focus. I've never seen that, but i like it
:-).

[Dan Beam, 2015/07/18 01:02:20]

Acknowledged.

           break;
         case 'clear-all-command':
           e.canExecute = true;
@@ -204,20 +200,25 @@ cr.define('downloads', function() {
       if (e.command.id == 'undo-command')
         chrome.send('undo');
       else if (e.command.id == 'clear-all-command')
-        this.clearAll_();
+        this.clearAll();
     },
-  };
 
-  Manager.updateAll = function(list) {
-    Manager.getInstance().updateAll_(list);
-  };
+    /** @private */
+    onLoad_: function() {
+      this.list_ = $('downloads-list');
 
-  Manager.updateItem = function(item) {
-    Manager.getInstance().updateItem_(item);
-  };
+      var toolbar = document.querySelector('downloads-toolbar');
+      /** @private {!downloads.Toolbar} */
+      this.toolbar_ = assertInstanceof(toolbar, downloads.Toolbar);
+      this.toolbar_.setActionService(this);

[Jeremy Klein, 2015/07/16 18:54:53]

This creates a circular dependency that makes these two components really
tightly coupled. I realize that the interface technically breaks that loop, but
it still seems like the coupling here isn't ideal. Events would mean that the
toolbar wouldn't need any knowledge of the action service at all, but would
create a lot of extra boilerplate. The other option is an ActionService which
has no explicit knowledge of the toolbar's existence. That would also make this
a bit easier to test independently.

Regardless, I won't block on this, but it's something to consider.

[Dan Beam, 2015/07/18 01:02:20]

Done.

+
+      cr.ui.decorate('command', cr.ui.Command);
+      document.addEventListener('canExecute', this.onCanExecute_.bind(this));
+      document.addEventListener('command', this.onCommand_.bind(this));
 
-  Manager.setSearchText = function(searchText) {
-    Manager.getInstance().setSearchText_(searchText);
+      // Shows all downloads.
+      this.search('');
+    },
   };
 
   Manager.onLoad = function() {
@@ -228,6 +229,14 @@ cr.define('downloads', function() {
     return Manager.getInstance().size_();
   };
 
+  Manager.updateAll = function(list) {
+    Manager.getInstance().updateAll_(list);
+  };
+
+  Manager.updateItem = function(item) {
+    Manager.getInstance().updateItem_(item);
+  };
+
   return {Manager: Manager};
 });