1st round fix. draft
authorSean Lee <selee@mozilla.com>
Mon, 15 Aug 2016 17:24:42 +0800
changeset 401134 84bc2212f76cd0d98d9f19877483018376c70d21
parent 401133 3f8cd17ec70cafe68f34022f29c48eeeb89c5750
child 401135 41749af7f07a6cd8915d2c68969cf4ef86071e65
push id26368
push userbmo:selee@mozilla.com
push dateTue, 16 Aug 2016 10:34:53 +0000
milestone51.0a1
1st round fix. MozReview-Commit-ID: 5qhYjkCCsH4
browser/components/downloads/DownloadsCommon.jsm
browser/components/downloads/content/downloads.css
browser/components/downloads/content/downloads.js
browser/components/downloads/content/downloadsOverlay.xul
browser/components/downloads/test/browser/browser_downloads_panel_footer.js
browser/components/downloads/test/browser/head.js
browser/locales/en-US/chrome/browser/downloads/downloads.dtd
browser/themes/shared/downloads/downloads.inc.css
--- a/browser/components/downloads/DownloadsCommon.jsm
+++ b/browser/components/downloads/DownloadsCommon.jsm
@@ -518,16 +518,41 @@ this.DownloadsCommon = {
             .getService(Ci.nsIExternalProtocolService)
             .loadUrl(NetUtil.newURI(parent));
         }
       }
     }
   },
 
   /**
+   * Show the folder of a file in the system file manager.
+   *
+   * @param aDirectory
+   *        a directory to be opened with system file manager.
+   */
+  showDirectory(aDirectory) {
+    if (!(aDirectory instanceof Ci.nsIFile)) {
+      throw new Error("aDirectory must be a nsIFile object");
+    }
+    if (!aDirectory.isDirectory()) {
+      throw new Error("aDirectory must be a directory");
+    }
+    try {
+      // Open the parent directory to show where the file should be.
+      aDirectory.launch();
+    } catch (ex) {
+      // If launch also fails (probably because it's not implemented), let
+      // the OS handler try to open the directory.
+      Cc["@mozilla.org/uriloader/external-protocol-service;1"]
+        .getService(Ci.nsIExternalProtocolService)
+        .loadUrl(NetUtil.newURI(aDirectory));
+    }
+  },
+
+  /**
    * Displays an alert message box which asks the user if they want to
    * unblock the downloaded file or not.
    *
    * @param options
    *        An object with the following properties:
    *        {
    *          verdict:
    *            The detailed reason why the download was blocked, according to
--- a/browser/components/downloads/content/downloads.css
+++ b/browser/components/downloads/content/downloads.css
@@ -24,17 +24,17 @@ richlistitem[type="download"].download-s
   display: none;
 }
 
 #downloadsPanel[hasdownloads] #downloadsHistory {
   padding-left: 58px !important;
 }
 
 #downloadsFooter .rollOverSplitter {
-  margin: 4px 0;
+  margin: 7px 0;
   width: 1px !important;
   -moz-appearance: none !important;
   border: none;
   background-image: none;
   background-color: rgb(212, 212, 212);
 }
 
 #downloadsFooter:hover .rollOverSplitter {
@@ -63,17 +63,17 @@ richlistitem[type="download"].download-s
   filter: url("chrome://browser/skin/filters.svg#fill");
 }
 
 #downloadsDropmarker:not([open=true]):hover {
   outline: 1px solid hsla(210,4%,10%,.07);
   background-color: hsla(210,4%,10%,.07);
 }
 
-#downloadsDropmarker[open=true]:hover {
+#downloadsDropmarker[open=true] {
   outline: 1px solid hsla(210,4%,10%,.12);
   background-color: hsla(210,4%,10%,.12);
   box-shadow: 0 1px 0 hsla(210,4%,10%,.05) inset;
 }
 
 /*** Downloads View ***/
 
 /**
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -73,18 +73,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 
-const nsIDM = Ci.nsIDownloadManager;
-
 ////////////////////////////////////////////////////////////////////////////////
 //// DownloadsPanel
 
 /**
  * Main entry point for the downloads panel interface.
  */
 const DownloadsPanel = {
   //////////////////////////////////////////////////////////////////////////////
@@ -364,31 +362,43 @@ const DownloadsPanel = {
 
     // Allow the anchor to be hidden.
     DownloadsButton.releaseAnchor();
 
     // Allow the panel to be reopened.
     this._state = this.kStateHidden;
   },
 
+  onFooterPopupShowing(aEvent) {
+    document.getElementById("downloadsDropdownItemClearList").setAttribute(
+      "hidden", !DownloadsCommon.getData(window).canRemoveFinished);
+  },
+
   //////////////////////////////////////////////////////////////////////////////
   //// Related operations
 
   /**
    * Shows or focuses the user interface dedicated to downloads history.
    */
   showDownloadsHistory() {
     DownloadsCommon.log("Showing download history.");
     // Hide the panel before showing another window, otherwise focus will return
     // to the browser window when the panel closes automatically.
     this.hidePanel();
 
     BrowserDownloadsUI();
   },
 
+  openDownloadsFolder() {
+    Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
+      DownloadsCommon.showDirectory(new FileUtils.File(downloadsPath));
+    }).catch(Cu.reportError);
+    this.hidePanel();
+  },
+
   //////////////////////////////////////////////////////////////////////////////
   //// Internal functions
 
   /**
    * Attach event listeners to a panel element. These listeners should be
    * removed in _unattachEventListeners. This is called automatically after the
    * panel has successfully loaded.
    */
@@ -520,30 +530,16 @@ const DownloadsPanel = {
       if (DownloadsView.richListBox.itemCount > 0) {
         DownloadsView.richListBox.focus();
       } else {
         DownloadsFooter.focus();
       }
     }
   },
 
-  clearPreviewPanel() {
-    DownloadsCommon.getData(window).removeFinished();
-    this.hidePanel();
-  },
-
-  openDownloadsFolder() {
-    Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
-      let file = new FileUtils.File(downloadsPath);
-      DownloadsCommon.showDownloadedFile(file);
-      this.hidePanel();
-      return;
-    });
-  },
-
   /**
    * Opens the downloads panel when data is ready to be displayed.
    */
   _openPopupIfDataReady() {
     // We don't want to open the popup if we already displayed it, or if we are
     // still loading data.
     if (this._state != this.kStateWaitingData || DownloadsView.loading) {
       return;
@@ -726,18 +722,16 @@ const DownloadsView = {
     }
 
     DownloadsBlockedSubview.view.setHeightToFit();
 
     // If we've got some hidden downloads, we should activate the
     // DownloadsSummary. The DownloadsSummary will determine whether or not
     // it's appropriate to actually display the summary.
     DownloadsSummary.active = hiddenCount > 0;
-    DownloadsSummary.showingClearPreviewPanelItem =
-      DownloadsCommon.getData(window).canRemoveFinished;
   },
 
   /**
    * Element corresponding to the list of downloads.
    */
   get richListBox() {
     delete this.richListBox;
     return this.richListBox = document.getElementById("downloadsListBox");
@@ -822,18 +816,16 @@ const DownloadsView = {
     }
   },
 
   onDownloadStateChanged(download) {
     let viewItem = this._visibleViewItems.get(download);
     if (viewItem) {
       viewItem.onStateChanged();
     }
-    DownloadsSummary.showingClearPreviewPanelItem =
-      DownloadsCommon.getData(window).canRemoveFinished;
   },
 
   onDownloadChanged(download) {
     let viewItem = this._visibleViewItems.get(download);
     if (viewItem) {
       viewItem.onChanged();
     }
   },
@@ -1324,20 +1316,16 @@ const DownloadsSummary = {
    * Returns the active state of the downloads summary.
    */
   get active() {
     return this._active;
   },
 
   _active: false,
 
-  set showingClearPreviewPanelItem(aShowingItem) {
-    document.getElementById("clearPreviewPanelButton").setAttribute("hidden", !aShowingItem);
-  },
-
   /**
    * Sets whether or not we show the progress bar.
    *
    * @param aShowingProgress
    *        True if we should show the progress bar.
    */
   set showingProgress(aShowingProgress) {
     if (aShowingProgress) {
--- a/browser/components/downloads/content/downloadsOverlay.xul
+++ b/browser/components/downloads/content/downloadsOverlay.xul
@@ -99,17 +99,17 @@
                   accesskey="&cmd.goToDownloadPage.accesskey;"/>
         <menuitem command="downloadsCmd_copyLocation"
                   label="&cmd.copyDownloadLink.label;"
                   accesskey="&cmd.copyDownloadLink.accesskey;"/>
 
         <menuseparator/>
 
         <menuitem command="downloadsCmd_clearList"
-                  label="&cmd.clearList.label;"
+                  label="&cmd.clearList2.label;"
                   accesskey="&cmd.clearList.accesskey;"/>
       </menupopup>
 
       <panelmultiview id="downloadsPanel-multiView"
                       mainViewId="downloadsPanel-mainView"
                       align="stretch">
 
         <panelview id="downloadsPanel-mainView"
@@ -143,36 +143,36 @@
                                min="0"
                                max="100"
                                mode="normal" />
                 <description id="downloadsSummaryDetails"
                              style="width: &downloadDetails.width;"
                              crop="end"/>
               </vbox>
             </hbox>
-            <hbox id="downloadsFooterButtons" flex="1">
+            <hbox id="downloadsFooterButtons">
               <button id="downloadsHistory"
                       class="plain downloadsPanelFooterButton"
                       label="&downloadsHistory.label;"
                       accesskey="&downloadsHistory.accesskey;"
                       flex="1"
                       oncommand="DownloadsPanel.showDownloadsHistory();"/>
               <toolbarseparator class="rollOverSplitter"/>
               <button id="downloadsDropmarker"
                       type="menu"
-                      flex="1"
-                      popup="downloadSubPanel">
-                <menupopup id="downloadSubPanel" position="after_end">
-                  <menuitem id="clearPreviewPanelButton"
-                            oncommand="DownloadsPanel.clearPreviewPanel();event.stopPropagation();"
-                            hidden="true"
-                            label="&clearPreviewPanelButton.label;"/>
-                  <menuitem id="openDownloadsFolderButton"
-                            oncommand="DownloadsPanel.openDownloadsFolder();event.stopPropagation();"
-                            label="&openDownloadsFolderButton.label;"/>
+                      flex="1">
+                <menupopup id="downloadSubPanel"
+                           onpopupshown="DownloadsPanel.onFooterPopupShowing(event);"
+                           position="after_end">
+                  <menuitem id="downloadsDropdownItemClearList"
+                            command="downloadsCmd_clearList"
+                            label="&cmd.clearList2.label;"/>
+                  <menuitem id="downloadsDropdownItemOpenDownloadsFolder"
+                            oncommand="DownloadsPanel.openDownloadsFolder();"
+                            label="&openDownloadsFolder.label;"/>
                 </menupopup>
               </button>
             </hbox>
           </vbox>
         </panelview>
 
         <panelview id="downloadsPanel-blockedSubview"
                    orient="vertical"
--- a/browser/components/downloads/test/browser/browser_downloads_panel_footer.js
+++ b/browser/components/downloads/test/browser/browser_downloads_panel_footer.js
@@ -1,98 +1,93 @@
 "use strict";
 
-add_task(function* mainTest() {
-  yield verify_openDownloadsFolder();
+add_task(function* test_openDownloadsFolder() {
+  yield task_openPanel();
+
+  let downloadsDropmarker = document.getElementById("downloadsDropmarker");
+  EventUtils.synthesizeMouseAtCenter(downloadsDropmarker, {}, window);
+
+  let downloadSubPanel = document.getElementById("downloadSubPanel");
+  yield BrowserTestUtils.waitForEvent(downloadSubPanel, "popupshown");
 
-  const TEST_PATTERN = [{
+  yield new Promise(resolve => {
+    sinon.stub(DownloadsCommon, "showDirectory", file => {
+      Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
+        is(file.path, downloadsPath, "Check the download folder path.");
+        resolve();
+      });
+    });
+
+    let itemOpenDownloadsFolder =
+      document.getElementById("downloadsDropdownItemOpenDownloadsFolder");
+    EventUtils.synthesizeMouseAtCenter(itemOpenDownloadsFolder, {}, window);
+  });
+
+  yield task_resetState();
+});
+
+add_task(function* test_clearList() {
+  const kTestCases = [{
     downloads: [
       { state: nsIDM.DOWNLOAD_NOTSTARTED },
       { state: nsIDM.DOWNLOAD_FINISHED },
       { state: nsIDM.DOWNLOAD_FAILED },
       { state: nsIDM.DOWNLOAD_CANCELED },
     ],
-    showClearPreviewButton: true,
-    expectedItemNumber: 0
+    expectClearListShown: true,
+    expectedItemNumber: 0,
   },{
     downloads: [
       { state: nsIDM.DOWNLOAD_NOTSTARTED },
       { state: nsIDM.DOWNLOAD_FINISHED },
       { state: nsIDM.DOWNLOAD_FAILED },
       { state: nsIDM.DOWNLOAD_PAUSED },
       { state: nsIDM.DOWNLOAD_CANCELED },
     ],
-    showClearPreviewButton: true,
-    expectedItemNumber: 1
+    expectClearListShown: true,
+    expectedItemNumber: 1,
   },{
     downloads: [
       { state: nsIDM.DOWNLOAD_PAUSED },
     ],
-    showClearPreviewButton: false,
-    expectedItemNumber: 1
+    expectClearListShown: false,
+    expectedItemNumber: 1,
   }];
 
-  for (let pattern in TEST_PATTERN) {
-    yield verify_clearPreviewList(TEST_PATTERN[pattern]);
+  for (let testCase of kTestCases) {
+    yield verify_clearList(testCase);
   }
 });
 
-function *verify_openDownloadsFolder() {
-  // a. Open Downloads Panel.
+function *verify_clearList(testCase) {
+  let downloads = testCase.downloads;
+  yield task_addDownloads(downloads);
+
   yield task_openPanel();
+  is(DownloadsView._downloads.length, downloads.length,
+    "Expect the number of download items");
 
-  // b. Open the dropdown menu.
   let downloadsDropmarker = document.getElementById("downloadsDropmarker");
   EventUtils.synthesizeMouseAtCenter(downloadsDropmarker, {}, window);
-  yield promisePolling(() => {
-    return "true" === downloadsDropmarker.getAttribute("_moz-menuactive");
-  });
+
+  let downloadSubPanel = document.getElementById("downloadSubPanel");
+  yield BrowserTestUtils.waitForEvent(downloadSubPanel, "popupshown");
 
-  yield new Promise(resolve => {
-    let stubShowDownloadedFile = sinon.stub(DownloadsCommon, "showDownloadedFile", file => {
-      // d. Check the stub of DownloadsCommon.showDownloadedFile is been called.
-      Downloads.getPreferredDownloadsDirectory().then(downloadsPath => {
-        is(file.path, downloadsPath, "Check the download folder path.");
-        resolve();
-      });
-    });
+  let itemClearList =
+    document.getElementById("downloadsDropdownItemClearList");
+  if (testCase.expectClearListShown) {
+    isnot("true", itemClearList.getAttribute("hidden"),
+      "Should show Clear Preview Panel button");
+    EventUtils.synthesizeMouseAtCenter(itemClearList, {}, window);
+  } else {
+    is("true", itemClearList.getAttribute("hidden"),
+      "Should not show Clear Preview Panel button");
+  }
 
-    // c. Click "Open Downloads Folder"
-    let openDownloadsFolderButton = document.getElementById("openDownloadsFolderButton");
-    EventUtils.synthesizeMouseAtCenter(openDownloadsFolderButton, {}, window);
+  yield BrowserTestUtils.waitForCondition(() => {
+    return DownloadsView._downloads.length === testCase.expectedItemNumber;
   });
+  is(DownloadsView._downloads.length, testCase.expectedItemNumber,
+    "Download items remained.");
 
   yield task_resetState();
 }
-
-function *verify_clearPreviewList(testData) {
-  // a. Add the download items.
-  const DownloadData = testData.downloads;
-  yield task_addDownloads(DownloadData);
-
-  // b. Open Downloads Panel.
-  yield task_openPanel();
-  is(DownloadsView._downloads.length, DownloadData.length, "There are " + DownloadData.length + " download items.");
-
-  // c. Open the dropdown menu.
-  let downloadsDropmarker = document.getElementById("downloadsDropmarker");
-  EventUtils.synthesizeMouseAtCenter(downloadsDropmarker, {}, window);
-  yield promisePolling(() => {
-    return "true" === downloadsDropmarker.getAttribute("_moz-menuactive");
-  });
-
-  // d. Check and click "Clear Preview List"
-  let clearPreviewPanelButton = document.getElementById("clearPreviewPanelButton");
-  if (testData.showClearPreviewButton) {
-    isnot("true", clearPreviewPanelButton.getAttribute("hidden"), "Should show Clear Preview Panel button");
-    EventUtils.synthesizeMouseAtCenter(clearPreviewPanelButton, {}, window);
-  } else {
-    is("true", clearPreviewPanelButton.getAttribute("hidden"), "Should not show Clear Preview Panel button");
-  }
-
-  // e. Check if the items are cleared.
-  yield promisePolling(() => {
-    return DownloadsView._downloads.length === testData.expectedItemNumber;
-  });
-  is(DownloadsView._downloads.length, testData.expectedItemNumber, "There are " + DownloadData.length + " download items remained.");
-
-  yield task_resetState();
-}
--- a/browser/components/downloads/test/browser/head.js
+++ b/browser/components/downloads/test/browser/head.js
@@ -22,18 +22,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/Task.jsm");
 const nsIDM = Ci.nsIDownloadManager;
 
 var gTestTargetFile = FileUtils.getFile("TmpD", ["dm-ui-test.file"]);
 gTestTargetFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
 
 // Load mocking/stubbing library, sinon
 // docs: http://sinonjs.org/docs/
-let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader);
-loader.loadSubScript("resource://testing-common/sinon-1.16.1.js");
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-1.16.1.js");
 
 registerCleanupFunction(function () {
   gTestTargetFile.remove(false);
 
   delete window.sinon;
   delete window.setImmediate;
   delete window.clearImmediate;
 });
@@ -105,28 +104,16 @@ function promiseWindowClosed(win)
         resolve();
       }
     }, "domwindowclosed", false);
   });
   win.close();
   return promise;
 }
 
-function promisePolling(condition) {
-  return new Promise(resolve => {
-    let interval = setInterval(() => {
-      if (condition()) {
-        clearInterval(interval);
-        setTimeout(resolve, 1000);
-        return;
-      }
-    }, 0);
-  });
-}
-
 function promiseFocus()
 {
   let deferred = Promise.defer();
   waitForFocus(deferred.resolve);
   return deferred.promise;
 }
 
 function promisePanelOpened()
--- a/browser/locales/en-US/chrome/browser/downloads/downloads.dtd
+++ b/browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ -57,18 +57,20 @@
 <!ENTITY cmd.showMac.accesskey            "F">
 <!ENTITY cmd.retry.label                  "Retry">
 <!ENTITY cmd.goToDownloadPage.label       "Go To Download Page">
 <!ENTITY cmd.goToDownloadPage.accesskey   "G">
 <!ENTITY cmd.copyDownloadLink.label       "Copy Download Link">
 <!ENTITY cmd.copyDownloadLink.accesskey   "L">
 <!ENTITY cmd.removeFromHistory.label      "Remove From History">
 <!ENTITY cmd.removeFromHistory.accesskey  "e">
-<!ENTITY cmd.clearList.label              "Clear Preview Panel">
+<!ENTITY cmd.clearList.label              "Clear List">
 <!ENTITY cmd.clearList.accesskey          "a">
+<!ENTITY cmd.clearList2.label              "Clear Preview Panel">
+<!ENTITY cmd.clearList2.accesskey          "a">
 <!ENTITY cmd.clearDownloads.label         "Clear Downloads">
 <!ENTITY cmd.clearDownloads.accesskey     "D">
 <!-- LOCALIZATION NOTE (cmd.unblock2.label):
      This command is shown in the context menu when downloads are blocked.
      -->
 <!ENTITY cmd.unblock2.label               "Allow Download">
 <!ENTITY cmd.unblock2.accesskey           "o">
 <!-- LOCALIZATION NOTE (cmd.removeFile.label):
@@ -104,21 +106,17 @@
 <!-- LOCALIZATION NOTE (downloadsHistory.label, downloadsHistory.accesskey):
      This string is shown at the bottom of the Downloads Panel when all the
      downloads fit in the available space, or when there are no downloads in
      the panel at all.
      -->
 <!ENTITY downloadsHistory.label           "Show All Downloads">
 <!ENTITY downloadsHistory.accesskey       "S">
 
-<!ENTITY downloadFooterSubPanelButton.accesskey       "P">
-
-<!ENTITY clearPreviewPanelButton.label       "Clear Preview Panel">
-
-<!ENTITY openDownloadsFolderButton.label       "Open Downloads Folder">
+<!ENTITY openDownloadsFolder.label       "Open Downloads Folder">
 
 <!ENTITY clearDownloadsButton.label       "Clear Downloads">
 <!ENTITY clearDownloadsButton.tooltip     "Clears completed, canceled and failed downloads">
 
 <!-- LOCALIZATION NOTE (downloadsListEmpty.label):
      This string is shown when there are no items in the Downloads view, when it
      is displayed inside a browser tab.
      -->
--- a/browser/themes/shared/downloads/downloads.inc.css
+++ b/browser/themes/shared/downloads/downloads.inc.css
@@ -26,17 +26,17 @@
 #downloadsListBox {
   background: transparent;
   padding: 4px;
   color: inherit;
 }
 
 #emptyDownloads {
   height: 40px;
-  padding: 16px 20px;
+  padding: 15px 20px 12px 20px;
   /* The panel can be wider than this description after the blocked subview is
      shown, so center the text. */
   text-align: center;
 }
 
 .downloadsPanelFooter {
   background-color: hsla(210,4%,10%,.07);
   border-top: 1px solid hsla(210,4%,10%,.14);