Bug 1117145 - Part 4 - Unify how commands are handled in the Downloads Panel and the Library. r=jaws draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Sun, 14 Feb 2016 16:45:21 +0000
changeset 330929 613897cae143156ab20438cd5a75198aa4414a99
parent 330928 d2938e83954c4162825a442311352b3e20d0a13c
child 330930 f66b321a86da8d3d7550a14414dd7f3a81e3f657
push id10859
push userpaolo.mozmail@amadzone.org
push dateSun, 14 Feb 2016 17:41:24 +0000
reviewersjaws
bugs1117145
milestone47.0a1
Bug 1117145 - Part 4 - Unify how commands are handled in the Downloads Panel and the Library. r=jaws MozReview-Commit-ID: 2Irgwu5YI72
browser/components/downloads/content/allDownloadsViewOverlay.js
browser/components/downloads/content/downloads.js
--- a/browser/components/downloads/content/allDownloadsViewOverlay.js
+++ b/browser/components/downloads/content/allDownloadsViewOverlay.js
@@ -29,22 +29,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
 const nsIDM = Ci.nsIDownloadManager;
 
 const DESTINATION_FILE_URI_ANNO  = "downloads/destinationFileURI";
 const DOWNLOAD_META_DATA_ANNO    = "downloads/metaData";
 
-const DOWNLOAD_VIEW_SUPPORTED_COMMANDS =
- ["cmd_delete", "cmd_copy", "cmd_paste", "cmd_selectAll",
-  "downloadsCmd_pauseResume", "downloadsCmd_cancel", "downloadsCmd_unblock",
-  "downloadsCmd_confirmBlock", "downloadsCmd_open", "downloadsCmd_show",
-  "downloadsCmd_retry", "downloadsCmd_openReferrer", "downloadsCmd_clearDownloads"];
-
 /**
  * Represents a download from the browser history. It implements part of the
  * interface of the Download object.
  *
  * @param aPlacesNode
  *        The Places node from which the history download should be initialized.
  */
 function HistoryDownload(aPlacesNode) {
@@ -312,17 +306,16 @@ HistoryDownloadElementShell.prototype = 
     // This cannot be placed within onStateChanged because
     // when a download goes from hasBlockedData to !hasBlockedData
     // it will still remain in the same state.
     this.element.classList.toggle("temporary-block",
                                   !!this.download.hasBlockedData);
     this._updateProgress();
   },
 
-  /* nsIController */
   isCommandEnabled(aCommand) {
     // The only valid command for inactive elements is cmd_delete.
     if (!this.active && aCommand != "cmd_delete") {
       return false;
     }
     switch (aCommand) {
       case "downloadsCmd_open":
         // This property is false if the download did not succeed.
@@ -351,76 +344,76 @@ HistoryDownloadElementShell.prototype = 
         return !!this._sessionDownload;
       case "downloadsCmd_confirmBlock":
       case "downloadsCmd_unblock":
         return this.download.hasBlockedData;
     }
     return false;
   },
 
-  /* nsIController */
   doCommand(aCommand) {
-    switch (aCommand) {
-      case "downloadsCmd_open": {
-        let file = new FileUtils.File(this.download.target.path);
-        DownloadsCommon.openDownloadedFile(file, null, window);
-        break;
-      }
-      case "downloadsCmd_show": {
-        let file = new FileUtils.File(this.download.target.path);
-        DownloadsCommon.showDownloadedFile(file);
-        break;
-      }
-      case "downloadsCmd_openReferrer": {
-        openURL(this.download.source.referrer);
-        break;
-      }
-      case "downloadsCmd_cancel": {
-        this.download.cancel().catch(() => {});
-        this.download.removePartialData().catch(Cu.reportError);
-        break;
+    if (aCommand.startsWith("cmd_") || aCommand.startsWith("downloadsCmd_")) {
+      this[aCommand](this);
+    }
+  },
+
+  downloadsCmd_open() {
+    let file = new FileUtils.File(this.download.target.path);
+    DownloadsCommon.openDownloadedFile(file, null, window);
+  },
+
+  downloadsCmd_show() {
+    let file = new FileUtils.File(this.download.target.path);
+    DownloadsCommon.showDownloadedFile(file);
+  },
+
+  downloadsCmd_openReferrer() {
+    openURL(this.download.source.referrer);
+  },
+
+  downloadsCmd_cancel() {
+    this.download.cancel().catch(() => {});
+    this.download.removePartialData().catch(Cu.reportError);
+  },
+
+  cmd_delete() {
+    if (this._sessionDownload) {
+      DownloadsCommon.removeAndFinalizeDownload(this.download);
+    }
+    if (this._historyDownload) {
+      let uri = NetUtil.newURI(this.download.source.url);
+      PlacesUtils.bhistory.removePage(uri);
+    }
+  },
+
+  downloadsCmd_retry() {
+    // Errors when retrying are already reported as download failures.
+    this.download.start().catch(() => {});
+  },
+
+  downloadsCmd_pauseResume() {
+    // This command is only enabled for session downloads.
+    if (this.download.stopped) {
+      this.download.start();
+    } else {
+      this.download.cancel();
+    }
+  },
+
+  downloadsCmd_unblock() {
+    DownloadsCommon.confirmUnblockDownload(DownloadsCommon.BLOCK_VERDICT_MALWARE,
+                                           window).then((confirmed) => {
+      if (confirmed) {
+        return this.download.unblock();
       }
-      case "cmd_delete": {
-        if (this._sessionDownload) {
-          DownloadsCommon.removeAndFinalizeDownload(this.download);
-        }
-        if (this._historyDownload) {
-          let uri = NetUtil.newURI(this.download.source.url);
-          PlacesUtils.bhistory.removePage(uri);
-        }
-        break;
-      }
-      case "downloadsCmd_retry": {
-        // Errors when retrying are already reported as download failures.
-        this.download.start().catch(() => {});
-        break;
-      }
-      case "downloadsCmd_pauseResume": {
-        // This command is only enabled for session downloads.
-        if (this.download.stopped) {
-          this.download.start();
-        } else {
-          this.download.cancel();
-        }
-        break;
-      }
-      case "downloadsCmd_unblock": {
-        DownloadsCommon.confirmUnblockDownload(DownloadsCommon.BLOCK_VERDICT_MALWARE,
-                                               window).then((confirmed) => {
-          if (confirmed) {
-            return this.download.unblock();
-          }
-        }).catch(Cu.reportError);
-        break;
-      }
-      case "downloadsCmd_confirmBlock": {
-        this.download.confirmBlock().catch(Cu.reportError);
-        break;
-      }
-    }
+    }).catch(Cu.reportError);
+  },
+
+  downloadsCmd_confirmBlock() {
+    this.download.confirmBlock().catch(Cu.reportError);
   },
 
   // Returns whether or not the download handled by this shell should
   // show up in the search results for the given term.  Both the display
   // name for the download and the url are searched.
   matchesSearchTerm(aTerm) {
     if (!aTerm) {
       return true;
@@ -1181,34 +1174,38 @@ DownloadsPlacesView.prototype = {
   onDownloadChanged(download) {
     this._viewItemsForDownloads.get(download).onChanged();
   },
 
   onDownloadRemoved(download) {
     this._removeSessionDownloadFromView(download);
   },
 
+  // nsIController
   supportsCommand(aCommand) {
-    if (DOWNLOAD_VIEW_SUPPORTED_COMMANDS.indexOf(aCommand) != -1) {
-      // The clear-downloads command may be performed by the toolbar-button,
-      // which can be focused on OS X.  Thus enable this command even if the
-      // richlistbox is not focused.
-      // For other commands, be prudent and disable them unless the richlistview
-      // is focused. It's important to make the decision here rather than in
-      // isCommandEnabled.  Otherwise our controller may "steal" commands from
-      // other controls in the window (see goUpdateCommand &
-      // getControllerForCommand).
-      if (document.activeElement == this._richlistbox ||
-          aCommand == "downloadsCmd_clearDownloads") {
-        return true;
-      }
+    // Firstly, determine if this is a command that we can handle.
+    if (!aCommand.startsWith("cmd_") &&
+        !aCommand.startsWith("downloadsCmd_")) {
+      return false;
+    }
+    if (!(aCommand in this) &&
+        !(aCommand in HistoryDownloadElementShell.prototype)) {
+      return false;
     }
-    return false;
+    // If this function returns true, other controllers won't get a chance to
+    // process the command even if isCommandEnabled returns false, so it's
+    // important to check if the list is focused here to handle common commands
+    // like copy and paste correctly. The clear downloads command, instead, is
+    // specific to the downloads list but can be invoked from the toolbar, so we
+    // can just return true unconditionally.
+    return aCommand == "downloadsCmd_clearDownloads" ||
+           document.activeElement == this._richlistbox;
   },
 
+  // nsIController
   isCommandEnabled(aCommand) {
     switch (aCommand) {
       case "cmd_copy":
         return this._richlistbox.selectedItems.length > 0;
       case "cmd_selectAll":
         return true;
       case "cmd_paste":
         return this._canDownloadClipboardURL();
@@ -1275,53 +1272,61 @@ DownloadsPlacesView.prototype = {
 
   _downloadURLFromClipboard() {
     let [url, name] = this._getURLFromClipboardData();
     let browserWin = RecentWindow.getMostRecentBrowserWindow();
     let initiatingDoc = browserWin ? browserWin.document : document;
     DownloadURL(url, name, initiatingDoc);
   },
 
+  // nsIController
   doCommand(aCommand) {
-    switch (aCommand) {
-      case "cmd_copy":
-        this._copySelectedDownloadsToClipboard();
-        break;
-      case "cmd_selectAll":
-        this._richlistbox.selectAll();
-        break;
-      case "cmd_paste":
-        this._downloadURLFromClipboard();
-        break;
-      case "downloadsCmd_clearDownloads":
-        this._downloadsData.removeFinished();
-        if (this.result) {
-          Cc["@mozilla.org/browser/download-history;1"]
-            .getService(Ci.nsIDownloadHistory)
-            .removeAllDownloads();
-        }
-        // There may be no selection or focus change as a result
-        // of these change, and we want the command updated immediately.
-        goUpdateCommand("downloadsCmd_clearDownloads");
-        break;
-      default: {
-        // Cloning the nodelist into an array to get a frozen list of selected items.
-        // Otherwise, the selectedItems nodelist is live and doCommand may alter the
-        // selection while we are trying to do one particular action, like removing
-        // items from history.
-        let selectedElements = [... this._richlistbox.selectedItems];
-        for (let element of selectedElements) {
-          element._shell.doCommand(aCommand);
-        }
-      }
+    // If this command is not selection-specific, execute it.
+    if (aCommand in this) {
+      this[aCommand]();
+      return;
+    }
+
+    // Cloning the nodelist into an array to get a frozen list of selected items.
+    // Otherwise, the selectedItems nodelist is live and doCommand may alter the
+    // selection while we are trying to do one particular action, like removing
+    // items from history.
+    let selectedElements = [...this._richlistbox.selectedItems];
+    for (let element of selectedElements) {
+      element._shell.doCommand(aCommand);
     }
   },
 
+  // nsIController
   onEvent() {},
 
+  cmd_copy() {
+    this._copySelectedDownloadsToClipboard();
+  },
+
+  cmd_selectAll() {
+    this._richlistbox.selectAll();
+  },
+
+  cmd_paste() {
+    this._downloadURLFromClipboard();
+  },
+
+  downloadsCmd_clearDownloads() {
+    this._downloadsData.removeFinished();
+    if (this.result) {
+      Cc["@mozilla.org/browser/download-history;1"]
+        .getService(Ci.nsIDownloadHistory)
+        .removeAllDownloads();
+    }
+    // There may be no selection or focus change as a result
+    // of these change, and we want the command updated immediately.
+    goUpdateCommand("downloadsCmd_clearDownloads");
+  },
+
   onContextMenu(aEvent) {
     let element = this._richlistbox.selectedItem;
     if (!element || !element._shell) {
       return false;
     }
 
     // Set the state attribute so that only the appropriate items are displayed.
     let contextMenu = document.getElementById("downloadsContextMenu");
@@ -1452,12 +1457,18 @@ DownloadsPlacesView.prototype = {
 for (let methodName of ["load", "applyFilter", "selectNode", "selectItems"]) {
   DownloadsPlacesView.prototype[methodName] = function () {
     throw new Error("|" + methodName +
                     "| is not implemented by the downloads view.");
   }
 }
 
 function goUpdateDownloadCommands() {
-  for (let command of DOWNLOAD_VIEW_SUPPORTED_COMMANDS) {
-    goUpdateCommand(command);
+  function updateCommandsForObject(object) {
+    for (let name in object) {
+      if (name.startsWith("cmd_") || name.startsWith("downloadsCmd_")) {
+        goUpdateCommand(name);
+      }
+    }
   }
+  updateCommandsForObject(this);
+  updateCommandsForObject(HistoryDownloadElementShell.prototype);
 }
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -1087,122 +1087,115 @@ DownloadsViewItem.prototype = {
       case "downloadsCmd_confirmBlock":
         return this.download.hasBlockedData;
     }
     return false;
   },
 
   doCommand(aCommand) {
     if (this.isCommandEnabled(aCommand)) {
-      this.commands[aCommand].apply(this);
+      this[aCommand](this);
     }
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Item commands
 
-  /**
-   * This object contains one key for each command that operates on this item.
-   *
-   * In commands, the "this" identifier points to the DownloadsViewItem.
-   */
-  commands: {
-    cmd_delete() {
-      DownloadsCommon.removeAndFinalizeDownload(this.download);
-      PlacesUtils.bhistory.removePage(
-                             NetUtil.newURI(this.download.source.url));
-    },
+  cmd_delete() {
+    DownloadsCommon.removeAndFinalizeDownload(this.download);
+    PlacesUtils.bhistory.removePage(
+                           NetUtil.newURI(this.download.source.url));
+  },
 
-    downloadsCmd_cancel() {
-      this.download.cancel().catch(() => {});
-      this.download.removePartialData().catch(Cu.reportError);
-    },
+  downloadsCmd_cancel() {
+    this.download.cancel().catch(() => {});
+    this.download.removePartialData().catch(Cu.reportError);
+  },
 
-    downloadsCmd_unblock() {
-      DownloadsPanel.hidePanel();
-      DownloadsCommon.confirmUnblockDownload(DownloadsCommon.BLOCK_VERDICT_MALWARE,
-                                             window).then((confirmed) => {
-        if (confirmed) {
-          return this.download.unblock();
-        }
-      }).catch(Cu.reportError);
-    },
+  downloadsCmd_unblock() {
+    DownloadsPanel.hidePanel();
+    DownloadsCommon.confirmUnblockDownload(DownloadsCommon.BLOCK_VERDICT_MALWARE,
+                                           window).then((confirmed) => {
+      if (confirmed) {
+        return this.download.unblock();
+      }
+    }).catch(Cu.reportError);
+  },
 
-    downloadsCmd_confirmBlock() {
-      this.download.confirmBlock().catch(Cu.reportError);
-    },
+  downloadsCmd_confirmBlock() {
+    this.download.confirmBlock().catch(Cu.reportError);
+  },
 
-    downloadsCmd_open() {
-      this.download.launch().catch(Cu.reportError);
+  downloadsCmd_open() {
+    this.download.launch().catch(Cu.reportError);
 
-      // We explicitly close the panel here to give the user the feedback that
-      // their click has been received, and we're handling the action.
-      // Otherwise, we'd have to wait for the file-type handler to execute
-      // before the panel would close. This also helps to prevent the user from
-      // accidentally opening a file several times.
-      DownloadsPanel.hidePanel();
-    },
+    // We explicitly close the panel here to give the user the feedback that
+    // their click has been received, and we're handling the action.
+    // Otherwise, we'd have to wait for the file-type handler to execute
+    // before the panel would close. This also helps to prevent the user from
+    // accidentally opening a file several times.
+    DownloadsPanel.hidePanel();
+  },
 
-    downloadsCmd_show() {
-      let file = new FileUtils.File(this.download.target.path);
-      DownloadsCommon.showDownloadedFile(file);
+  downloadsCmd_show() {
+    let file = new FileUtils.File(this.download.target.path);
+    DownloadsCommon.showDownloadedFile(file);
 
-      // We explicitly close the panel here to give the user the feedback that
-      // their click has been received, and we're handling the action.
-      // Otherwise, we'd have to wait for the operating system file manager
-      // window to open before the panel closed. This also helps to prevent the
-      // user from opening the containing folder several times.
-      DownloadsPanel.hidePanel();
-    },
+    // We explicitly close the panel here to give the user the feedback that
+    // their click has been received, and we're handling the action.
+    // Otherwise, we'd have to wait for the operating system file manager
+    // window to open before the panel closed. This also helps to prevent the
+    // user from opening the containing folder several times.
+    DownloadsPanel.hidePanel();
+  },
 
-    downloadsCmd_pauseResume() {
-      if (this.download.stopped) {
-        this.download.start();
-      } else {
-        this.download.cancel();
-      }
-    },
+  downloadsCmd_pauseResume() {
+    if (this.download.stopped) {
+      this.download.start();
+    } else {
+      this.download.cancel();
+    }
+  },
 
-    downloadsCmd_retry() {
-      this.download.start().catch(() => {});
-    },
+  downloadsCmd_retry() {
+    this.download.start().catch(() => {});
+  },
 
-    downloadsCmd_openReferrer() {
-      openURL(this.download.source.referrer);
-    },
+  downloadsCmd_openReferrer() {
+    openURL(this.download.source.referrer);
+  },
 
-    downloadsCmd_copyLocation() {
-      let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"]
-                      .getService(Ci.nsIClipboardHelper);
-      clipboard.copyString(this.download.source.url);
-    },
+  downloadsCmd_copyLocation() {
+    let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"]
+                    .getService(Ci.nsIClipboardHelper);
+    clipboard.copyString(this.download.source.url);
+  },
 
-    downloadsCmd_doDefault() {
-      const nsIDM = Ci.nsIDownloadManager;
+  downloadsCmd_doDefault() {
+    const nsIDM = Ci.nsIDownloadManager;
 
-      // Determine the default command for the current item.
-      let defaultCommand = function () {
-        switch (DownloadsCommon.stateOfDownload(this.download)) {
-          case nsIDM.DOWNLOAD_NOTSTARTED:       return "downloadsCmd_cancel";
-          case nsIDM.DOWNLOAD_FINISHED:         return "downloadsCmd_open";
-          case nsIDM.DOWNLOAD_FAILED:           return "downloadsCmd_retry";
-          case nsIDM.DOWNLOAD_CANCELED:         return "downloadsCmd_retry";
-          case nsIDM.DOWNLOAD_PAUSED:           return "downloadsCmd_pauseResume";
-          case nsIDM.DOWNLOAD_QUEUED:           return "downloadsCmd_cancel";
-          case nsIDM.DOWNLOAD_BLOCKED_PARENTAL: return "downloadsCmd_openReferrer";
-          case nsIDM.DOWNLOAD_SCANNING:         return "downloadsCmd_show";
-          case nsIDM.DOWNLOAD_DIRTY:            return "downloadsCmd_openReferrer";
-          case nsIDM.DOWNLOAD_BLOCKED_POLICY:   return "downloadsCmd_openReferrer";
-        }
-        return "";
-      }.apply(this);
-      if (defaultCommand && this.isCommandEnabled(defaultCommand)) {
-        this.doCommand(defaultCommand);
+    // Determine the default command for the current item.
+    let defaultCommand = function () {
+      switch (DownloadsCommon.stateOfDownload(this.download)) {
+        case nsIDM.DOWNLOAD_NOTSTARTED:       return "downloadsCmd_cancel";
+        case nsIDM.DOWNLOAD_FINISHED:         return "downloadsCmd_open";
+        case nsIDM.DOWNLOAD_FAILED:           return "downloadsCmd_retry";
+        case nsIDM.DOWNLOAD_CANCELED:         return "downloadsCmd_retry";
+        case nsIDM.DOWNLOAD_PAUSED:           return "downloadsCmd_pauseResume";
+        case nsIDM.DOWNLOAD_QUEUED:           return "downloadsCmd_cancel";
+        case nsIDM.DOWNLOAD_BLOCKED_PARENTAL: return "downloadsCmd_openReferrer";
+        case nsIDM.DOWNLOAD_SCANNING:         return "downloadsCmd_show";
+        case nsIDM.DOWNLOAD_DIRTY:            return "downloadsCmd_openReferrer";
+        case nsIDM.DOWNLOAD_BLOCKED_POLICY:   return "downloadsCmd_openReferrer";
       }
-    },
+      return "";
+    }.apply(this);
+    if (defaultCommand && this.isCommandEnabled(defaultCommand)) {
+      this.doCommand(defaultCommand);
+    }
   },
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadsViewController
 
 /**
  * Handles part of the user interaction events raised by the downloads list
@@ -1221,18 +1214,22 @@ const DownloadsViewController = {
     window.controllers.removeController(this);
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// nsIController
 
   supportsCommand(aCommand) {
     // Firstly, determine if this is a command that we can handle.
-    if (!(aCommand in this.commands) &&
-        !(aCommand in DownloadsViewItem.prototype.commands)) {
+    if (!aCommand.startsWith("cmd_") &&
+        !aCommand.startsWith("downloadsCmd_")) {
+      return false;
+    }
+    if (!(aCommand in this) &&
+        !(aCommand in DownloadsViewItem.prototype)) {
       return false;
     }
     // Secondly, determine if focus is on a control in the downloads list.
     let element = document.commandDispatcher.focusedElement;
     while (element && element != DownloadsView.richListBox) {
       element = element.parentNode;
     }
     // We should handle the command only if the downloads list is among the
@@ -1249,18 +1246,18 @@ const DownloadsViewController = {
     // Other commands are selection-specific.
     let element = DownloadsView.richListBox.selectedItem;
     return element && DownloadsView.itemForElement(element)
                                    .isCommandEnabled(aCommand);
   },
 
   doCommand(aCommand) {
     // If this command is not selection-specific, execute it.
-    if (aCommand in this.commands) {
-      this.commands[aCommand].apply(this);
+    if (aCommand in this) {
+      this[aCommand]();
       return;
     }
 
     // Other commands are selection-specific.
     let element = DownloadsView.richListBox.selectedItem;
     if (element) {
       // The doCommand function also checks if the command is enabled.
       DownloadsView.itemForElement(element).doCommand(aCommand);
@@ -1268,33 +1265,33 @@ const DownloadsViewController = {
   },
 
   onEvent() {},
 
   //////////////////////////////////////////////////////////////////////////////
   //// Other functions
 
   updateCommands() {
-    Object.keys(this.commands).forEach(goUpdateCommand);
-    Object.keys(DownloadsViewItem.prototype.commands)
-          .forEach(goUpdateCommand);
+    function updateCommandsForObject(object) {
+      for (let name in object) {
+        if (name.startsWith("cmd_") || name.startsWith("downloadsCmd_")) {
+          goUpdateCommand(name);
+        }
+      }
+    }
+    updateCommandsForObject(this);
+    updateCommandsForObject(DownloadsViewItem.prototype);
   },
 
   //////////////////////////////////////////////////////////////////////////////
   //// Selection-independent commands
 
-  /**
-   * This object contains one key for each command that operates regardless of
-   * the currently selected item in the list.
-   */
-  commands: {
-    downloadsCmd_clearList() {
-      DownloadsCommon.getData(window).removeFinished();
-    }
-  }
+  downloadsCmd_clearList() {
+    DownloadsCommon.getData(window).removeFinished();
+  },
 };
 
 XPCOMUtils.defineConstant(this, "DownloadsViewController", DownloadsViewController);
 
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadsSummary
 
 /**