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

@@ -38,6 +38,10 @@
 # ***** 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;">
@@ -431,7 +435,8 @@
                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"
@@ -459,6 +464,7 @@
       </menu>
       <menuitem id="menu_bookmarkAllTabs"
                 label="&addCurPagesCmd.label;"
+                class="show-only-for-keyboard"
                 command="Browser:BookmarkAllTabs"
                 key="bookmarkAllTabsKb"/>
       <menuitem id="bookmarksShowAll"

browser/base/content/browser-places.js

@@ -386,30 +386,21 @@ var PlacesCommandHook = {
   },
 
   /**
-   * 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;
   },
 
   /**
@@ -417,10 +408,22 @@ var PlacesCommandHook = {
    * 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. 

browser/base/content/browser-sets.inc

@@ -91,11 +91,10 @@
     <!-- 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">
@@ -304,7 +303,7 @@
     <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"/>

browser/base/content/browser.css

@@ -192,6 +192,11 @@ splitmenu {
 }
 %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);

browser/base/content/browser.js

@@ -1550,9 +1550,6 @@ function delayedStartup(isLoadingBlank, mustLoadSidebar) {
 
   PlacesToolbarHelper.init();
 
-  // bookmark-all-tabs command
-  gBookmarkAllTabsHandler.init();
-
   ctrlTab.readPref();
   gPrefService.addObserver(ctrlTab.prefName, ctrlTab, false);
   gPrefService.addObserver(allTabs.prefName, allTabs, false);
@@ -7159,41 +7156,6 @@ function formatURL(aFormat, aIsPref) {
   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
  */
@@ -8303,6 +8265,12 @@ var TabContextMenu = {
     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;
   }

browser/base/content/test/browser_visibleTabs_bookmarkAllPages.js

@@ -50,7 +50,7 @@ function test() {
 
     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");

browser/base/content/test/browser_visibleTabs_bookmarkAllTabs.js

@@ -36,43 +36,67 @@
  * ***** 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");
 
-  // 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");
+  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");
 
-  // 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");
+    // 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");
 
-  // Show all tabs
-  let allTabs = [tab for each (tab in gBrowser.tabs)];
-  gBrowser.showOnlyTheseTabs(allTabs);
+    // 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");
 
-  // 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!");
+    // Show all tabs
+    let allTabs = [tab for each (tab in gBrowser.tabs)];
+    gBrowser.showOnlyTheseTabs(allTabs);
+
+    // 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";
 }
+
+function Hidden() {
+  document.popupNode = gBrowser.selectedTab;
+  TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu"));
+  return document.getElementById("context_bookmarkAllTabs").hidden;
+}