Bug 588011 - "Bookmark All Tabs" should ignore App Tabs.
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 28 Jan 2011 17:46:49 +0100
changeset 61548 e2f9d9c8b18ca0a3ff3909db12417729b2c01124
parent 61547 cfd957a4ac9f9d8a07083f94b326ca2b18fd9472
child 61549 5e576c6e3d75482fe989bc4aaa1b4e8e29a80fb9
push id1
push userroot
push dateTue, 10 Dec 2013 15:46:25 +0000
bugs588011, 607227
milestone2.0b11pre
Bug 588011 - "Bookmark All Tabs" should ignore App Tabs. Includes fix for Bug 607227 - Remove "Bookmark all Tabs" from the bookmarks menu. r=dao ui-r=faaborg a=blocker
browser/base/content/browser-menubar.inc
browser/base/content/browser-places.js
browser/base/content/browser-sets.inc
browser/base/content/browser.css
browser/base/content/browser.js
browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js
browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js
--- a/browser/base/content/browser-menubar.inc
+++ b/browser/base/content/browser-menubar.inc
@@ -33,16 +33,20 @@
 # decision by deleting the provisions above and replace them with the notice
 # and other provisions required by the GPL or the LGPL. If you do not delete
 # the provisions above, a recipient may use your version of this file under
 # the terms of any one of the MPL, the GPL or the LGPL.
 #
 # ***** END LICENSE BLOCK *****
 
        <menubar id="main-menubar"
+                onpopupshowing="if (event.target.parentNode.parentNode == this &amp;&amp;
+                                    !('@mozilla.org/widget/nativemenuservice;1' in Cc))
+                                  this.setAttribute('openedwithkey',
+                                                    event.target.parentNode.openedWithKey);"
                 style="border:0px;padding:0px;margin:0px;-moz-appearance:none">
             <menu id="file-menu" label="&fileMenu.label;"
                   accesskey="&fileMenu.accesskey;">
               <menupopup id="menu_FilePopup">
                 <menuitem id="menu_newNavigatorTab"
                           label="&tabCmd.label;"
                           command="cmd_newNavigatorTab"
                           key="key_newNavigatorTab"
@@ -426,17 +430,18 @@
     <menupopup id="bookmarksMenuPopup"
 #ifndef XP_MACOSX
                placespopup="true"
 #endif
                context="placesContext"
                openInTabs="children"
                oncommand="BookmarksEventHandler.onCommand(event);"
                onclick="BookmarksEventHandler.onClick(event);"
-               onpopupshowing="if (!this.parentNode._placesView)
+               onpopupshowing="PlacesCommandHook.updateBookmarkAllTabsCommand();
+                               if (!this.parentNode._placesView)
                                  new PlacesMenu(event, 'place:folder=BOOKMARKS_MENU');"
                tooltip="bhTooltip" popupsinherittooltip="true">
       <menuitem id="menu_bookmarkThisPage"
                 label="&bookmarkThisPageCmd.label;"
                 command="Browser:AddBookmarkAs"
                 key="addBookmarkAsKb"/>
       <menuitem id="subscribeToPageMenuitem"
 #ifndef XP_MACOSX
@@ -454,16 +459,17 @@
             observes="multipleFeedsMenuState">
         <menupopup id="subscribeToPageSubmenuMenupopup"
                    onpopupshowing="return FeedHandler.buildFeedList(event.target);"
                    oncommand="return FeedHandler.subscribeToFeed(null, event);"
                    onclick="checkForMiddleClick(this, event);"/>
       </menu>
       <menuitem id="menu_bookmarkAllTabs"
                 label="&addCurPagesCmd.label;"
+                class="show-only-for-keyboard"
                 command="Browser:BookmarkAllTabs"
                 key="bookmarkAllTabsKb"/>
       <menuitem id="bookmarksShowAll"
                 label="&showAllBookmarks2.label;"
                 command="Browser:ShowAllBookmarks"
                 key="manBookmarkKb"/>
       <menuseparator id="organizeBookmarksSeparator"/>
       <menu id="bookmarksToolbarFolderMenu"
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -381,52 +381,55 @@ var PlacesCommandHook = {
       PlacesUIUtils.showMinimalAddBookmarkUI(linkURI, aTitle);
     else {
       PlacesUIUtils.showItemProperties(itemId,
                                        PlacesUtils.bookmarks.TYPE_BOOKMARK);
     }
   },
 
   /**
-   * This function returns a list of nsIURI objects characterizing the
-   * tabs currently open in the browser.  The URIs will appear in the
-   * list in the order in which their corresponding tabs appeared.  However,
-   * only the first instance of each URI will be returned.
-   *
-   * @returns a list of nsIURI objects representing unique locations open
+   * List of nsIURI objects characterizing the tabs currently open in the
+   * browser, modulo pinned tabs.  The URIs will be in the order in which their
+   * corresponding tabs appeared and duplicates are discarded.
    */
-  _getUniqueTabInfo: function BATC__getUniqueTabInfo() {
-    var tabList = [];
-    var seenURIs = {};
-
-    let tabs = gBrowser.visibleTabs;
-    for (let i = 0; i < tabs.length; ++i) {
-      let uri = tabs[i].linkedBrowser.currentURI;
-
-      // skip redundant entries
-      if (uri.spec in seenURIs)
-        continue;
-
-      // add to the set of seen URIs
-      seenURIs[uri.spec] = null;
-      tabList.push(uri);
-    }
-    return tabList;
+  get uniqueCurrentPages() {
+    let uniquePages = {};
+    let URIs = [];
+    gBrowser.visibleTabs.forEach(function (tab) {
+      let spec = tab.linkedBrowser.currentURI.spec;
+      if (!tab.pinned && !(spec in uniquePages)) {
+        uniquePages[spec] = null;
+        URIs.push(tab.linkedBrowser.currentURI);
+      }
+    });
+    return URIs;
   },
 
   /**
    * Adds a folder with bookmarks to all of the currently open tabs in this 
    * window.
    */
   bookmarkCurrentPages: function PCH_bookmarkCurrentPages() {
-    var tabURIs = this._getUniqueTabInfo();
-    PlacesUIUtils.showMinimalAddMultiBookmarkUI(tabURIs);
+    let pages = this.uniqueCurrentPages;
+    if (pages.length > 1) {
+      PlacesUIUtils.showMinimalAddMultiBookmarkUI(pages);
+    }
   },
 
-  
+  /**
+   * Updates disabled state for the "Bookmark All Tabs" command.
+   */
+  updateBookmarkAllTabsCommand:
+  function PCH_updateBookmarkAllTabsCommand() {
+    // Disable "Bookmark All Tabs" if there are less than two
+    // "unique current pages".
+    goSetCommandEnabled("Browser:BookmarkAllTabs",
+                        this.uniqueCurrentPages.length >= 2);
+  },
+
   /**
    * Adds a Live Bookmark to a feed associated with the current page. 
    * @param     url
    *            The nsIURI of the page the feed was attached to
    * @title     title
    *            The title of the feed. Optional.
    * @subtitle  subtitle
    *            A short description of the feed. Optional.
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -86,21 +86,20 @@
              oncommand="gFindBar.onFindAgainCommand(false);"
              observes="isImage"/>
     <command id="cmd_findPrevious"
              oncommand="gFindBar.onFindAgainCommand(true);"
              observes="isImage"/>
     <!-- work-around bug 392512 -->
     <command id="Browser:AddBookmarkAs"
              oncommand="PlacesCommandHook.bookmarkCurrentPage(true, PlacesUtils.bookmarksMenuFolderId);"/>
-    <!-- The command is disabled for the hidden window. Otherwise its enabled
-         state is handled by gBookmarkAllTabsHandler. -->
+    <!-- The command disabled state must be manually updated through
+         PlacesCommandHook.updateBookmarkAllTabsCommand() -->
     <command id="Browser:BookmarkAllTabs"
-             oncommand="gBookmarkAllTabsHandler.doCommand();"
-             disabled="true"/>
+             oncommand="PlacesCommandHook.bookmarkCurrentPages();"/>
     <command id="Browser:Home"    oncommand="BrowserHome();"/>
     <command id="Browser:Back"    oncommand="BrowserBack();" disabled="true"/>
     <command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event);" disabled="true">
       <observes element="Browser:Back" attribute="disabled"/>
     </command>
     <command id="Browser:Forward" oncommand="BrowserForward();" disabled="true"/>
     <command id="Browser:ForwardOrForwardDuplicate" oncommand="BrowserForward(event);" disabled="true">
       <observes element="Browser:Forward" attribute="disabled"/>
@@ -299,17 +298,17 @@
     <key id="key_findAgain" key="&findAgainCmd.commandkey;" command="cmd_findAgain" modifiers="accel"/>
     <key id="key_findPrevious" key="&findAgainCmd.commandkey;" command="cmd_findPrevious" modifiers="accel,shift"/>
     <key keycode="&findAgainCmd.commandkey2;" command="cmd_findAgain"/>
     <key keycode="&findAgainCmd.commandkey2;"  command="cmd_findPrevious" modifiers="shift"/>
 
     <key id="addBookmarkAsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:AddBookmarkAs" modifiers="accel"/>
 # Accel+Shift+A-F are reserved on GTK2
 #ifndef MOZ_WIDGET_GTK2
-    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" command="Browser:BookmarkAllTabs" modifiers="accel,shift"/>
+    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" oncommand="PlacesCommandHook.bookmarkCurrentPages();" modifiers="accel,shift"/>
     <key id="manBookmarkKb" key="&bookmarksCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
 #else
     <key id="manBookmarkKb" key="&bookmarksGtkCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
 #endif
     <key id="viewBookmarksSidebarKb" key="&bookmarksCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>
 #ifdef XP_WIN
 # Cmd+I is conventially mapped to Info on MacOS X, thus it should not be
 # overridden for other purposes there.
--- a/browser/base/content/browser.css
+++ b/browser/base/content/browser.css
@@ -187,16 +187,21 @@ splitmenu {
 }
 %endif
 
 #appmenu_offlineModeRecovery:not([checked=true]) {
   display: none;
 }
 %endif
 
+/* Hide menu elements intended for keyboard access support */
+#main-menubar[openedwithkey=false] .show-only-for-keyboard {
+  display: none;
+}
+
 /* ::::: location bar ::::: */
 #urlbar {
   -moz-binding: url(chrome://browser/content/urlbarBindings.xml#urlbar);
 }
 
 .uri-element-right-align:-moz-locale-dir(rtl),
 html|input.uri-element-right-align:-moz-locale-dir(rtl),
 .ac-url-text:-moz-locale-dir(rtl),
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -1545,19 +1545,16 @@ function delayedStartup(isLoadingBlank, 
       .getService(Ci.nsISessionStore)
       .init(window);
   } catch (ex) {
     dump("nsSessionStore could not be initialized: " + ex + "\n");
   }
 
   PlacesToolbarHelper.init();
 
-  // bookmark-all-tabs command
-  gBookmarkAllTabsHandler.init();
-
   ctrlTab.readPref();
   gPrefService.addObserver(ctrlTab.prefName, ctrlTab, false);
   gPrefService.addObserver(allTabs.prefName, allTabs, false);
 
   // Initialize the microsummary service by retrieving it, prompting its factory
   // to create its singleton, whose constructor initializes the service.
   // Started 4 seconds after delayedStartup (before the livemarks service below).
   setTimeout(function() {
@@ -7155,51 +7152,16 @@ function BrowserOpenSyncTabs() {
  * Currently supported built-ins are LOCALE, APP, and any value from nsIXULAppInfo, uppercased.
  */
 function formatURL(aFormat, aIsPref) {
   var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
   return aIsPref ? formatter.formatURLPref(aFormat) : formatter.formatURL(aFormat);
 }
 
 /**
- * This also takes care of updating the command enabled-state when tabs are
- * created or removed.
- */
-var gBookmarkAllTabsHandler = {
-  init: function () {
-    this._command = document.getElementById("Browser:BookmarkAllTabs");
-    gBrowser.tabContainer.addEventListener("TabOpen", this, true);
-    gBrowser.tabContainer.addEventListener("TabClose", this, true);
-    gBrowser.tabContainer.addEventListener("TabShow", this, true);
-    gBrowser.tabContainer.addEventListener("TabHide", this, true);
-    this._updateCommandState();
-  },
-
-  _updateCommandState: function BATH__updateCommandState() {
-    let remainingTabs = gBrowser.visibleTabs.filter(function(tab) {
-      return gBrowser._removingTabs.indexOf(tab) == -1;
-    });
-
-    if (remainingTabs.length > 1)
-      this._command.removeAttribute("disabled");
-    else
-      this._command.setAttribute("disabled", "true");
-  },
-
-  doCommand: function BATH_doCommand() {
-    PlacesCommandHook.bookmarkCurrentPages();
-  },
-
-  // nsIDOMEventListener
-  handleEvent: function(aEvent) {
-    this._updateCommandState();
-  }
-};
-
-/**
  * Utility object to handle manipulations of the identity indicators in the UI
  */
 var gIdentityHandler = {
   // Mode strings used to control CSS display
   IDENTITY_MODE_IDENTIFIED       : "verifiedIdentity", // High-quality identity information
   IDENTITY_MODE_DOMAIN_VERIFIED  : "verifiedDomain",   // Minimal SSL CA-signed domain verification
   IDENTITY_MODE_UNKNOWN          : "unknownIdentity",  // No trusted identity information
   IDENTITY_MODE_MIXED_CONTENT    : "unknownIdentity mixedContent",  // SSL with unauthenticated content
@@ -8298,16 +8260,22 @@ var TabContextMenu = {
     document.getElementById("context_unpinTab").hidden = !this.contextTab.pinned;
 
     // Disable "Close other Tabs" if there is only one unpinned tab and
     // hide it when the user rightclicked on a pinned tab.
     let unpinnedTabs = gBrowser.visibleTabs.length - gBrowser._numPinnedTabs;
     document.getElementById("context_closeOtherTabs").disabled = unpinnedTabs <= 1;
     document.getElementById("context_closeOtherTabs").hidden = this.contextTab.pinned;
 
+    // Hide "Bookmark All Tabs" for a pinned tab.  Update its state if visible.
+    let bookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
+    bookmarkAllTabs.hidden = this.contextTab.pinned;
+    if (!bookmarkAllTabs.hidden)
+      PlacesCommandHook.updateBookmarkAllTabsCommand();
+
     // Hide "Move to Group" if it's a pinned tab.
     document.getElementById("context_tabViewMenu").hidden = this.contextTab.pinned;
   }
 };
 
 XPCOMUtils.defineLazyGetter(this, "HUDConsoleUI", function () {
   Cu.import("resource:///modules/HUDService.jsm");
   try {
--- a/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js
+++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js
@@ -45,17 +45,17 @@ function test() {
   let browser = gBrowser.getBrowserForTab(tabTwo);
   let onLoad = function() {
     browser.removeEventListener("load", onLoad, true);
 
     gBrowser.showOnlyTheseTabs([tabTwo]);
 
     is(gBrowser.visibleTabs.length, 1, "Only one tab is visible");
 
-    let uris = PlacesCommandHook._getUniqueTabInfo();
+    let uris = PlacesCommandHook.uniqueCurrentPages;
     is(uris.length, 1, "Only one uri is returned");
 
     is(uris[0].spec, tabTwo.linkedBrowser.currentURI.spec, "It's the correct URI");
 
     gBrowser.removeTab(tabOne);
     gBrowser.removeTab(tabTwo);
     Array.forEach(gBrowser.tabs, function(tab) {
       gBrowser.showTab(tab);
--- a/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js
+++ b/browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js
@@ -31,48 +31,72 @@
  * decision by deleting the provisions above and replace them with the notice
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 function test() {
+  waitForExplicitFinish();
+
   // There should be one tab when we start the test
   let [origTab] = gBrowser.visibleTabs;
   is(gBrowser.visibleTabs.length, 1, "1 tab should be open");  
-  is(Disabled(), true, "Bookmark All Tabs should be hidden");
+  is(Disabled(), true, "Bookmark All Tabs should be disabled");
 
   // Add a tab
-  let testTab = gBrowser.addTab();
+  let testTab1 = gBrowser.addTab();
   is(gBrowser.visibleTabs.length, 2, "2 tabs should be open");
-  is(Disabled(), false, "Bookmark All Tabs should be available");
+  is(Disabled(), true, "Bookmark All Tabs should be disabled since there are two tabs with the same address");
+
+  let testTab2 = gBrowser.addTab("about:robots");
+  is(gBrowser.visibleTabs.length, 3, "3 tabs should be open");
+  // Wait for tab load, the code checks for currentURI.
+  testTab2.linkedBrowser.addEventListener("load", function () {
+    testTab2.linkedBrowser.removeEventListener("load", arguments.callee, true);
+    is(Disabled(), false, "Bookmark All Tabs should be enabled since there are two tabs with different addresses");
+
+    // Hide the original tab
+    gBrowser.selectedTab = testTab2;
+    gBrowser.showOnlyTheseTabs([testTab2]);
+    is(gBrowser.visibleTabs.length, 1, "1 tab should be visible");  
+    is(Disabled(), true, "Bookmark All Tabs should be disabled as there is only one visible tab");
 
-  // Hide the original tab
-  gBrowser.selectedTab = testTab;
-  gBrowser.showOnlyTheseTabs([testTab]);
-  is(gBrowser.visibleTabs.length, 1, "1 tab should be visible");  
-  is(Disabled(), true, "Bookmark All Tabs should be hidden as there is only one visible tab");
-  
-  // Add a tab that will get pinned
-  let pinned = gBrowser.addTab();
-  gBrowser.pinTab(pinned);
-  is(gBrowser.visibleTabs.length, 2, "2 tabs should be visible now");
-  is(Disabled(), false, "Bookmark All Tabs should be available as there are two visible tabs");
+    // Add a tab that will get pinned
+    let pinned = gBrowser.addTab();
+    is(gBrowser.visibleTabs.length, 2, "2 tabs should be visible now");
+    is(Disabled(), false, "Bookmark All Tabs should be available as there are two visible tabs");
+    gBrowser.pinTab(pinned);
+    is(Hidden(), false, "Bookmark All Tabs should be visible on a normal tab");
+    is(Disabled(), true, "Bookmark All Tabs should not be available since one tab is pinned");
+    gBrowser.selectedTab = pinned;
+    is(Hidden(), true, "Bookmark All Tabs should be hidden on a pinned tab");
 
-  // Show all tabs
-  let allTabs = [tab for each (tab in gBrowser.tabs)];
-  gBrowser.showOnlyTheseTabs(allTabs);
+    // Show all tabs
+    let allTabs = [tab for each (tab in gBrowser.tabs)];
+    gBrowser.showOnlyTheseTabs(allTabs);
 
-  // reset the environment  
-  gBrowser.removeTab(testTab);
-  gBrowser.removeTab(pinned);
-  is(gBrowser.visibleTabs.length, 1, "only orig is left and visible");
-  is(gBrowser.tabs.length, 1, "sanity check that it matches");
-  is(Disabled(), true, "Bookmark All Tabs should be hidden");
-  is(gBrowser.selectedTab, origTab, "got the orig tab");
-  is(origTab.hidden, false, "and it's not hidden -- visible!");
+    // reset the environment  
+    gBrowser.removeTab(testTab2);
+    gBrowser.removeTab(testTab1);
+    gBrowser.removeTab(pinned);
+    is(gBrowser.visibleTabs.length, 1, "only orig is left and visible");
+    is(gBrowser.tabs.length, 1, "sanity check that it matches");
+    is(Disabled(), true, "Bookmark All Tabs should be hidden");
+    is(gBrowser.selectedTab, origTab, "got the orig tab");
+    is(origTab.hidden, false, "and it's not hidden -- visible!");
+    finish();
+  }, true);
 }
 
 function Disabled() {
+  document.popupNode = gBrowser.selectedTab;
+  TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu"));
   let command = document.getElementById("Browser:BookmarkAllTabs");
   return command.hasAttribute("disabled") && command.getAttribute("disabled") === "true";
-}
\ No newline at end of file
+}
+
+function Hidden() {
+  document.popupNode = gBrowser.selectedTab;
+  TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu"));
+  return document.getElementById("context_bookmarkAllTabs").hidden;
+}