From 435e84add2502444f0159d5388b9c87d733194c5 Mon Sep 17 00:00:00 2001
From: Jared Wein <jwein@mozilla.com>
Date: Tue, 2 Oct 2018 16:24:43 +0000
Subject: [PATCH] Bug 1480529 - Changed 'Bookmark All Tabs' to 'Bookmark Tab'
 for single tab selections. r=Felipe

I changed the toolbar context menuitem from 'Bookmark All Tabs' to 'Bookmark Selected Tabs' because it is separated from a specific tab and thus it is not clear which tab would get bookmarked if only one is selected. This seemed much clearer to me in my testing. The wording of 'Bookmark Selected Tabs' is already used elsewhere where we have 'Reload Selected Tabs', 'Close Selected Tabs', etc.

Differential Revision: https://phabricator.services.mozilla.com/D7217

--HG--
extra : moz-landing-system : lando
---
 browser/base/content/browser-places.js        | 33 ++++++++-----------
 browser/base/content/browser-sets.inc         |  2 --
 browser/base/content/browser.js               |  2 --
 browser/base/content/browser.xul              | 26 +++++++--------
 browser/base/content/tabbrowser.js            |  9 ++---
 .../tabs/browser_multiselect_tabs_bookmark.js |  8 ++---
 .../browser_visibleTabs_bookmarkAllTabs.js    | 21 +++---------
 .../browser_customization_context_menus.js    |  2 +-
 .../locales/en-US/chrome/browser/browser.dtd  |  8 ++---
 9 files changed, 42 insertions(+), 69 deletions(-)

diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
index 2659dded7269..03e5c5e503a4 100755
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -490,27 +490,21 @@ var PlacesCommandHook = {
    * Adds a folder with bookmarks to URIList given in param.
    */
   bookmarkPages(URIList) {
-    if (URIList.length > 1) {
-      PlacesUIUtils.showBookmarkDialog({ action: "add",
-                                         type: "folder",
-                                         URIList,
-                                       }, window);
-    }
-  },
-
-  /**
-   * Updates disabled state for the "Bookmark All Tabs" command.
-   */
-  updateBookmarkAllTabsCommand:
-  function PCH_updateBookmarkAllTabsCommand() {
-    // There's nothing to do in non-browser windows.
-    if (window.location.href != AppConstants.BROWSER_CHROME_URL)
+    if (!URIList.length) {
       return;
+    }
 
-    // Disable "Bookmark All Tabs" if there are less than two
-    // "unique current pages".
-    goSetCommandEnabled("Browser:BookmarkAllTabs",
-                        this.uniqueCurrentPages.length >= 2);
+    let bookmarkDialogInfo = {action: "add"};
+    if (URIList.length > 1) {
+      bookmarkDialogInfo.type = "folder";
+      bookmarkDialogInfo.URIList = URIList;
+    } else {
+      bookmarkDialogInfo.type = "bookmark";
+      bookmarkDialogInfo.title = URIList[0].title;
+      bookmarkDialogInfo.uri = URIList[0].uri;
+    }
+
+    PlacesUIUtils.showBookmarkDialog(bookmarkDialogInfo, window);
   },
 
   /**
@@ -1569,7 +1563,6 @@ var BookmarkingUI = {
       return;
 
     this.updateBookmarkPageMenuItem();
-    PlacesCommandHook.updateBookmarkAllTabsCommand();
     this._initMobileBookmarks(document.getElementById("menu_mobileBookmarks"));
   },
 
diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
index c28ac96efe8d..4f7539458461 100644
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -55,8 +55,6 @@
     <!-- work-around bug 392512 -->
     <command id="Browser:AddBookmarkAs"
              oncommand="PlacesCommandHook.bookmarkPage();"/>
-    <!-- The command disabled state must be manually updated through
-         PlacesCommandHook.updateBookmarkAllTabsCommand() -->
     <command id="Browser:BookmarkAllTabs"
              oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueCurrentPages);"/>
     <command id="Browser:Back"    oncommand="BrowserBack();" disabled="true"/>
diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
index 07ae9beec324..fd25c9b8f7ee 100644
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -5706,8 +5706,6 @@ function onViewToolbarsPopupShowing(aEvent, aInsertPoint) {
   }
 
   if (showTabStripItems) {
-    PlacesCommandHook.updateBookmarkAllTabsCommand();
-
     let haveMultipleTabs = gBrowser.visibleTabs.length > 1;
     document.getElementById("toolbar-context-reloadAllTabs").disabled = !haveMultipleTabs;
 
diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
index 485328ed6f74..ad738e6d8f3b 100644
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -117,6 +117,15 @@ xmlns="http://www.w3.org/1999/xhtml"
       <menuseparator/>
       <menuitem id="context_selectAllTabs" label="&selectAllTabs.label;" accesskey="&selectAllTabs.accesskey;"
                 oncommand="gBrowser.selectAllTabs();"/>
+      <menuitem id="context_bookmarkSelectedTabs"
+                hidden="true"
+                label="&bookmarkSelectedTabs.label;"
+                accesskey="&bookmarkSelectedTabs.accesskey;"
+                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages);"/>
+      <menuitem id="context_bookmarkTab"
+                label="&bookmarkTab.label;"
+                accesskey="&bookmarkTab.accesskey;"
+                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.getUniquePages([TabContextMenu.contextTab]));"/>
       <menuitem id="context_pinTab" label="&pinTab.label;"
                 accesskey="&pinTab.accesskey;"
                 oncommand="gBrowser.pinTab(TabContextMenu.contextTab);"/>
@@ -153,15 +162,6 @@ xmlns="http://www.w3.org/1999/xhtml"
       <menuitem id="context_reloadAllTabs" label="&reloadAllTabs.label;" accesskey="&reloadAllTabs.accesskey;"
                 tbattr="tabbrowser-multiple-visible"
                 oncommand="gBrowser.reloadAllTabs();"/>
-       <menuitem id="context_bookmarkSelectedTabs"
-                hidden="true"
-                label="&bookmarkSelectedTabs.label;"
-                accesskey="&bookmarkSelectedTabs.accesskey;"
-                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages);"/>
-      <menuitem id="context_bookmarkAllTabs"
-                label="&bookmarkAllTabs.label;"
-                accesskey="&bookmarkAllTabs.accesskey;"
-                command="Browser:BookmarkAllTabs"/>
       <menuitem id="context_closeTabsToTheEnd" label="&closeTabsToTheEnd.label;" accesskey="&closeTabsToTheEnd.accesskey;"
                 oncommand="gBrowser.removeTabsToTheEndFrom(TabContextMenu.contextTab, {animate: true});"/>
       <menuitem id="context_closeOtherTabs" label="&closeOtherTabs.label;" accesskey="&closeOtherTabs.accesskey;"
@@ -417,12 +417,12 @@ xmlns="http://www.w3.org/1999/xhtml"
                 oncommand="gBrowser.reloadAllTabs();"
                 label="&toolbarContextMenu.reloadAllTabs.label;"
                 accesskey="&toolbarContextMenu.reloadAllTabs.accesskey;"/>
-      <menuitem id="toolbar-context-bookmarkAllTabs"
+      <menuitem id="toolbar-context-bookmarkSelectedTabs"
                 class="toolbaritem-tabsmenu"
                 contexttype="tabbar"
-                command="Browser:BookmarkAllTabs"
-                label="&toolbarContextMenu.bookmarkAllTabs.label;"
-                accesskey="&toolbarContextMenu.bookmarkAllTabs.accesskey;"/>
+                oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages);"
+                label="&toolbarContextMenu.bookmarkSelectedTabs.label;"
+                accesskey="&toolbarContextMenu.bookmarkSelectedTabs.accesskey;"/>
       <menuitem id="toolbar-context-selectAllTabs"
                 class="toolbaritem-tabsmenu"
                 contexttype="tabbar"
diff --git a/browser/base/content/tabbrowser.js b/browser/base/content/tabbrowser.js
index 48694cadd1b6..f62ac6f45a91 100644
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -5343,13 +5343,10 @@ var TabContextMenu = {
     document.getElementById("context_closeTab").hidden = multiselectionContext;
     document.getElementById("context_closeSelectedTabs").hidden = !multiselectionContext;
 
-    // Hide "Bookmark All Tabs" for a pinned tab or multiselection.
+    // Hide "Bookmark Tab" for multiselection.
     // Update its state if visible.
-    let bookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
-    bookmarkAllTabs.hidden = this.contextTab.pinned || multiselectionContext;
-    if (!bookmarkAllTabs.hidden) {
-      PlacesCommandHook.updateBookmarkAllTabsCommand();
-    }
+    let bookmarkTab = document.getElementById("context_bookmarkTab");
+    bookmarkTab.hidden = multiselectionContext;
 
     // Show "Bookmark Selected Tabs" in a multiselect context and hide it otherwise.
     let bookmarkMultiSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
diff --git a/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js b/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
index b56abedf3a5f..6ed960b8cf9a 100644
--- a/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
+++ b/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
@@ -23,7 +23,7 @@ add_task(async function test() {
   let tab2 = await addTab();
   let tab3 = await addTab();
 
-  let menuItemBookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
+  let menuItemBookmarkTab = document.getElementById("context_bookmarkTab");
   let menuItemBookmarkSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
 
   is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
@@ -37,12 +37,12 @@ add_task(async function test() {
 
   // Check the context menu with a non-multiselected tab
   updateTabContextMenu(tab3);
-  is(menuItemBookmarkAllTabs.hidden, false, "Bookmark All Tabs is visible");
+  is(menuItemBookmarkTab.hidden, false, "Bookmark Tab is visible");
   is(menuItemBookmarkSelectedTabs.hidden, true, "Bookmark Selected Tabs is hidden");
 
   // Check the context menu with a multiselected tab and one unique page in the selection.
   updateTabContextMenu(tab2);
-  is(menuItemBookmarkAllTabs.hidden, true, "Bookmark All Tabs is visible");
+  is(menuItemBookmarkTab.hidden, true, "Bookmark Tab is visible");
   is(menuItemBookmarkSelectedTabs.hidden, false, "Bookmark Selected Tabs is hidden");
   is(PlacesCommandHook.uniqueSelectedPages.length, 1, "No more than one unique selected page");
 
@@ -55,7 +55,7 @@ add_task(async function test() {
 
   // Check the context menu with a multiselected tab and two unique pages in the selection.
   updateTabContextMenu(tab2);
-  is(menuItemBookmarkAllTabs.hidden, true, "Bookmark All Tabs is visible");
+  is(menuItemBookmarkTab.hidden, true, "Bookmark Tab is visible");
   is(menuItemBookmarkSelectedTabs.hidden, false, "Bookmark Selected Tabs is hidden");
   is(PlacesCommandHook.uniqueSelectedPages.length, 2, "More than one unique selected page");
 
diff --git a/browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js b/browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js
index 439ebe4b568e..2fb1c8fba653 100644
--- a/browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js
+++ b/browser/base/content/test/tabs/browser_visibleTabs_bookmarkAllTabs.js
@@ -8,34 +8,27 @@ function test() {
   // 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 disabled");
 
   // Add a tab
   let testTab1 = BrowserTestUtils.addTab(gBrowser);
   is(gBrowser.visibleTabs.length, 2, "2 tabs should be open");
-  is(Disabled(), true, "Bookmark All Tabs should be disabled since there are two tabs with the same address");
 
   let testTab2 = BrowserTestUtils.addTab(gBrowser, "about:mozilla");
   is(gBrowser.visibleTabs.length, 3, "3 tabs should be open");
   // Wait for tab load, the code checks for currentURI.
   testTab2.linkedBrowser.addEventListener("load", function() {
-    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");
 
     // Add a tab that will get pinned
     let pinned = BrowserTestUtils.addTab(gBrowser);
     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");
+    is(BookmarkTabHidden(), false, "Bookmark Tab should be visible on a normal tab");
     gBrowser.selectedTab = pinned;
-    is(Hidden(), true, "Bookmark All Tabs should be hidden on a pinned tab");
+    is(BookmarkTabHidden(), false, "Bookmark Tab should be visible on a pinned tab");
 
     // Show all tabs
     let allTabs = Array.from(gBrowser.tabs);
@@ -47,19 +40,13 @@ function test() {
     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();
   }, {capture: true, once: true});
 }
 
-function Disabled() {
+function BookmarkTabHidden() {
   updateTabContextMenu();
-  return document.getElementById("Browser:BookmarkAllTabs").getAttribute("disabled") == "true";
-}
-
-function Hidden() {
-  updateTabContextMenu();
-  return document.getElementById("context_bookmarkAllTabs").hidden;
+  return document.getElementById("context_bookmarkTab").hidden;
 }
diff --git a/browser/components/customizableui/test/browser_customization_context_menus.js b/browser/components/customizableui/test/browser_customization_context_menus.js
index 174536ab15f1..569f4fd87bb9 100644
--- a/browser/components/customizableui/test/browser_customization_context_menus.js
+++ b/browser/components/customizableui/test/browser_customization_context_menus.js
@@ -57,7 +57,7 @@ add_task(async function tabstrip_context() {
   info("Closed tabs: " + closedTabsAvailable);
   let expectedEntries = [
     ["#toolbar-context-reloadAllTabs", true],
-    ["#toolbar-context-bookmarkAllTabs", true],
+    ["#toolbar-context-bookmarkSelectedTabs", true],
     ["#toolbar-context-selectAllTabs", true],
     ["#toolbar-context-undoCloseTab", !closedTabsAvailable],
     ["---"],
diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd
index e452b332e67e..c3c68328b8a3 100644
--- a/browser/locales/en-US/chrome/browser/browser.dtd
+++ b/browser/locales/en-US/chrome/browser/browser.dtd
@@ -71,8 +71,8 @@ can reach it easily. -->
 <!ENTITY  moveToNewWindow.accesskey          "W">
 <!ENTITY  reopenInContainer.label            "Reopen in Container">
 <!ENTITY  reopenInContainer.accesskey        "e">
-<!ENTITY  bookmarkAllTabs.label              "Bookmark All Tabs…">
-<!ENTITY  bookmarkAllTabs.accesskey          "T">
+<!ENTITY  bookmarkTab.label                  "Bookmark Tab">
+<!ENTITY  bookmarkTab.accesskey              "T">
 <!ENTITY  undoCloseTab.label                 "Undo Close Tab">
 <!ENTITY  undoCloseTab.accesskey             "U">
 <!ENTITY  closeTab.label                     "Close Tab">
@@ -111,8 +111,8 @@ when there are no windows but Firefox is still running. -->
 
 <!ENTITY toolbarContextMenu.reloadAllTabs.label "Reload All Tabs">
 <!ENTITY toolbarContextMenu.reloadAllTabs.accesskey "A">
-<!ENTITY toolbarContextMenu.bookmarkAllTabs.label "Bookmark All Tabs…">
-<!ENTITY toolbarContextMenu.bookmarkAllTabs.accesskey "T">
+<!ENTITY toolbarContextMenu.bookmarkSelectedTabs.label "Bookmark Selected Tabs…">
+<!ENTITY toolbarContextMenu.bookmarkSelectedTabs.accesskey "T">
 <!ENTITY toolbarContextMenu.selectAllTabs.label "Select All Tabs">
 <!ENTITY toolbarContextMenu.selectAllTabs.accesskey "S">
 <!ENTITY toolbarContextMenu.undoCloseTab.label "Undo Close Tab">