Bug 1458067 - Implement ability to bookmark a selection of tabs. r?Gijs,mak draft multiselect_bookmark_rebase
authorAbdoulaye O. Ly <ablayelyfondou@gmail.com>
Thu, 12 Jul 2018 00:25:35 +0000
branchmultiselect_bookmark_rebase
changeset 817232 c711ca9f7d0cfee8963c6575d3466f1db2bc0bee
parent 817076 3aca103e49150145dbff910be15e7886b7c4495a
push id115995
push userbmo:ablayelyfondou@gmail.com
push dateThu, 12 Jul 2018 08:31:34 +0000
reviewersGijs, mak
bugs1458067
milestone63.0a1
Bug 1458067 - Implement ability to bookmark a selection of tabs. r?Gijs,mak MozReview-Commit-ID: 4KE3qSf3j0y
browser/base/content/browser-places.js
browser/base/content/browser-sets.inc
browser/base/content/browser.xul
browser/base/content/tabbrowser.js
browser/base/content/test/tabs/browser.ini
browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
browser/locales/en-US/chrome/browser/browser.dtd
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -471,48 +471,64 @@ var PlacesCommandHook = {
                                        uri: makeURI(url),
                                        title,
                                        defaultInsertionPoint,
                                        hiddenRows: [ "location", "keyword" ]
                                      }, window.top);
   },
 
   /**
-   * 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.
-   */
-  get uniqueCurrentPages() {
+   * List of nsIURI objects characterizing tabs given in param.
+   * Duplicates are discarded.
+  */
+  getUniquePages(tabs) {
     let uniquePages = {};
     let URIs = [];
 
-    gBrowser.visibleTabs.forEach(tab => {
+    tabs.forEach(tab => {
       let browser = tab.linkedBrowser;
       let uri = browser.currentURI;
       let title = browser.contentTitle || tab.label;
       let spec = uri.spec;
-      if (!tab.pinned && !(spec in uniquePages)) {
+      if (!(spec in uniquePages)) {
         uniquePages[spec] = null;
         URIs.push({ uri, title });
       }
     });
     return URIs;
   },
 
   /**
-   * Adds a folder with bookmarks to all of the currently open tabs in this
-   * window.
+   * 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.
+   */
+  get uniqueCurrentPages() {
+    let visibleUnpinnedTabs = gBrowser.visibleTabs
+                                      .filter(tab => !tab.pinned);
+    return this.getUniquePages(visibleUnpinnedTabs);
+  },
+
+   /**
+   * List of nsIURI objects characterizing the tabs currently
+   * selected in the window. Duplicates are discarded.
    */
-  bookmarkCurrentPages: function PCH_bookmarkCurrentPages() {
-    let pages = this.uniqueCurrentPages;
-    if (pages.length > 1) {
-    PlacesUIUtils.showBookmarkDialog({ action: "add",
-                                       type: "folder",
-                                       URIList: pages
-                                     }, window);
+  get uniqueSelectedPages() {
+    return this.getUniquePages(gBrowser.selectedTabs);
+  },
+
+  /**
+   * 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() {
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -56,17 +56,17 @@
     <command id="cmd_findSelection" oncommand="gLazyFindCommand('onFindSelectionCommand')"/>
 #endif
     <!-- 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.bookmarkCurrentPages();"/>
+             oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueCurrentPages);"/>
     <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"/>
@@ -301,17 +301,17 @@
     <key id="key_findSelection" key="&findSelectionCmd.commandkey;" command="cmd_findSelection" modifiers="accel"/>
 #endif
     <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 GTK
 #ifndef MOZ_WIDGET_GTK
-    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" oncommand="PlacesCommandHook.bookmarkCurrentPages();" modifiers="accel,shift"/>
+    <key id="bookmarkAllTabsKb" key="&bookmarkThisPageCmd.commandkey;" oncommand="PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueCurrentPages);" 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.xul
+++ b/browser/base/content/browser.xul
@@ -127,16 +127,21 @@
             accesskey="&sendTabToDevice.accesskey;">
         <menupopup id="context_sendTabToDevicePopupMenu"
                    onpopupshowing="gSync.populateSendTabToDevicesMenu(event.target, TabContextMenu.contextTab.linkedBrowser.currentURI.spec, TabContextMenu.contextTab.linkedBrowser.contentTitle);"/>
       </menu>
       <menuseparator/>
       <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;"
                 oncommand="gBrowser.removeAllTabsBut(TabContextMenu.contextTab);"/>
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -4998,23 +4998,28 @@ var TabContextMenu = {
       unpinnedTabsToClose--;
     }
     document.getElementById("context_closeOtherTabs").disabled = unpinnedTabsToClose < 1;
 
     // Only one of close_tab/close_selected_tabs should be visible
     document.getElementById("context_closeTab").hidden = multiselectionContext;
     document.getElementById("context_closeSelectedTabs").hidden = !multiselectionContext;
 
-    // Hide "Bookmark All Tabs" for a pinned tab.  Update its state if visible.
+    // Hide "Bookmark All Tabs" for a pinned tab or multiselection.
+    // Update its state if visible.
     let bookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
-    bookmarkAllTabs.hidden = this.contextTab.pinned;
+    bookmarkAllTabs.hidden = this.contextTab.pinned || multiselectionContext;
     if (!bookmarkAllTabs.hidden) {
       PlacesCommandHook.updateBookmarkAllTabsCommand();
     }
 
+    // Show "Bookmark Selected Tabs" in a multiselect context and hide it otherwise.
+    let bookmarkMultiSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
+    bookmarkMultiSelectedTabs.hidden = !multiselectionContext;
+
     let toggleMute = document.getElementById("context_toggleMuteTab");
     let toggleMultiSelectMute = document.getElementById("context_toggleMuteSelectedTabs");
 
     // Only one of mute_unmute_tab/mute_unmute_selected_tabs should be visible
     toggleMute.hidden = multiselectionContext;
     toggleMultiSelectMute.hidden = !multiselectionContext;
 
     // Adjust the state of the toggle mute menu item.
--- a/browser/base/content/test/tabs/browser.ini
+++ b/browser/base/content/test/tabs/browser.ini
@@ -15,16 +15,17 @@ tags = audiochannel
 [browser_bug580956.js]
 [browser_close_tab_by_dblclick.js]
 [browser_contextmenu_openlink_after_tabnavigated.js]
 skip-if = (verify && debug && (os == 'linux'))
 support-files =
   test_bug1358314.html
 [browser_isLocalAboutURI.js]
 [browser_multiselect_tabs_active_tab_selected_by_default.js]
+[browser_multiselect_tabs_bookmark.js]
 [browser_multiselect_tabs_close_using_shortcuts.js]
 [browser_multiselect_tabs_close.js]
 [browser_multiselect_tabs_mute_unmute.js]
 [browser_multiselect_tabs_pin_unpin.js]
 [browser_multiselect_tabs_positional_attrs.js]
 [browser_multiselect_tabs_reload.js]
 [browser_multiselect_tabs_using_Ctrl.js]
 [browser_multiselect_tabs_using_Shift.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
@@ -0,0 +1,64 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const PREF_MULTISELECT_TABS = "browser.tabs.multiselect";
+
+async function addTab_example_com() {
+  const tab = BrowserTestUtils.addTab(gBrowser,
+    "http://example.com/", { skipAnimation: true });
+  const browser = gBrowser.getBrowserForTab(tab);
+  await BrowserTestUtils.browserLoaded(browser);
+  return tab;
+}
+
+add_task(async function setPref() {
+  await SpecialPowers.pushPrefEnv({
+    set: [[PREF_MULTISELECT_TABS, true]]
+  });
+});
+
+add_task(async function test() {
+  let tab1 = await addTab();
+  let tab2 = await addTab();
+  let tab3 = await addTab();
+
+  let menuItemBookmarkAllTabs = document.getElementById("context_bookmarkAllTabs");
+  let menuItemBookmarkSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
+
+  is(gBrowser.multiSelectedTabsCount, 0, "Zero multiselected tabs");
+
+  await BrowserTestUtils.switchTab(gBrowser, tab1);
+  await triggerClickOn(tab2, { ctrlKey: true });
+
+  ok(tab1.multiselected, "Tab1 is multiselected");
+  ok(tab2.multiselected, "Tab2 is multiselected");
+  ok(!tab3.multiselected, "Tab3 is not multiselected");
+
+  // Check the context menu with a non-multiselected tab
+  updateTabContextMenu(tab3);
+  is(menuItemBookmarkAllTabs.hidden, false, "Bookmark All Tabs 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(menuItemBookmarkSelectedTabs.hidden, false, "Bookmark Selected Tabs is hidden");
+  is(PlacesCommandHook.uniqueSelectedPages.length, 1, "No more than one unique selected page");
+
+  info("Add a different page to selection");
+  let tab4 = await addTab_example_com();
+  await triggerClickOn(tab4, { ctrlKey: true });
+
+  ok(tab4.multiselected, "Tab4 is multiselected");
+  is(gBrowser.multiSelectedTabsCount, 3, "Three multiselected tabs");
+
+  // 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(menuItemBookmarkSelectedTabs.hidden, false, "Bookmark Selected Tabs is hidden");
+  is(PlacesCommandHook.uniqueSelectedPages.length, 2, "More than one unique selected page");
+
+  for (let tab of [tab1, tab2, tab3, tab4])
+    BrowserTestUtils.removeTab(tab);
+});
--- a/browser/locales/en-US/chrome/browser/browser.dtd
+++ b/browser/locales/en-US/chrome/browser/browser.dtd
@@ -44,16 +44,18 @@ but will never be visible at the same ti
 <!ENTITY  unpinSelectedTabs.label            "Unpin Tabs">
 <!-- Unpin Tab and Unpin Selected Tabs have the same accesskey
 but will never be visible at the same time. -->
 <!ENTITY  unpinSelectedTabs.accesskey        "b">
 <!-- Reload Tab and Reload Selected Tabs have the same accesskey
 but will never be visible at the same time. -->
 <!ENTITY  reloadSelectedTabs.label           "Reload Selected Tabs">
 <!ENTITY  reloadSelectedTabs.accesskey       "R">
+<!ENTITY  bookmarkSelectedTabs.label         "Bookmark Tabs…">
+<!ENTITY  bookmarkSelectedTabs.accesskey     "k">
 
 <!-- LOCALIZATION NOTE (pinTab.label, unpinTab.label): "Pin" is being
 used as a metaphor for expressing the fact that these tabs are "pinned" to the
 left edge of the tabstrip. Really we just want the string to express the idea
 that this is a lightweight and reversible action that keeps your tab where you
 can reach it easily. -->
 <!ENTITY  pinTab.label                       "Pin Tab">
 <!ENTITY  pinTab.accesskey                   "P">