Bug 1458067 - Implement ability to bookmark a selection of tabs. r?Gijs draft multiselect_bookmark
authorAbdoulaye O. Ly <ablayelyfondou@gmail.com>
Thu, 05 Jul 2018 06:14:54 +0000
branchmultiselect_bookmark
changeset 814384 694d8a7b102ade777b6ad8c0e20150cea2d924bf
parent 814179 a07cf0515fe3ab55353749a088429484d8cb8bb9
push id115178
push userbmo:ablayelyfondou@gmail.com
push dateThu, 05 Jul 2018 06:29:48 +0000
reviewersGijs
bugs1458067
milestone63.0a1
Bug 1458067 - Implement ability to bookmark a selection of tabs. r?Gijs MozReview-Commit-ID: BKx1IZbTAP0
browser/base/content/browser-places.js
browser/base/content/browser-sets.inc
browser/base/content/browser.js
browser/base/content/browser.xul
browser/base/content/test/tabs/browser.ini
browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js
browser/base/jar.mn
browser/locales/en-US/chrome/browser/browser.dtd
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -485,66 +485,113 @@ 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.
+   * List of nsIURI objects characterizing tabs given in param.
+   * Duplicates are discarded.
    */
-  get uniqueCurrentPages() {
+  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.
    */
-  bookmarkCurrentPages: function PCH_bookmarkCurrentPages() {
-    let pages = this.uniqueCurrentPages;
+  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.
+   */
+  get uniqueCurrentSelectedPages() {
+    return this.getUniquePages(gBrowser.selectedTabs);
+  },
+
+  /**
+   * Adds a folder with bookmarks to pages given in param
+   */
+  bookmarkPages: function PCH_bookmarkPages(pages) {
     if (pages.length > 1) {
     PlacesUIUtils.showBookmarkDialog({ action: "add",
                                        type: "folder",
                                        URIList: pages
                                      }, window);
     }
   },
 
   /**
+   * Adds a folder with bookmarks to all of the currently open tabs in this
+   * window.
+   */
+  bookmarkCurrentPages: function PCH_bookmarkCurrentPages() {
+    this.bookmarkPages(this.uniqueCurrentPages);
+  },
+
+  /**
+   * Adds a folder with bookmarks to all of the currently selected
+   * tabs in this window.
+   */
+  bookmarkCurrentSelectedPages: function PCH_bookmarkCurrentSelectedPages() {
+    this.bookmarkPages(this.uniqueCurrentSelectedPages);
+  },
+
+  /**
    * 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 != getBrowserURL())
       return;
 
     // Disable "Bookmark All Tabs" if there are less than two
     // "unique current pages".
     goSetCommandEnabled("Browser:BookmarkAllTabs",
                         this.uniqueCurrentPages.length >= 2);
   },
 
+    /**
+   * Updates disabled state for the "Bookmark Selected Tabs" command.
+   */
+  updateBookmarkSelectedTabsCommand:
+  function PCH_updateBookmarkSelectedTabsCommand() {
+    // There's nothing to do in non-browser windows.
+    if (window.location.href != getBrowserURL())
+      return;
+
+    // Disable "Bookmark Selected Tabs" if there are less than two
+    // "unique current selected pages".
+    goSetCommandEnabled("Browser:BookmarkSelectedTabs",
+                        this.uniqueCurrentSelectedPages.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.
    */
   async addLiveBookmark(url, feedTitle) {
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -57,16 +57,20 @@
 #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();"/>
+    <!-- The command disabled state must be manually updated through
+         PlacesCommandHook.updateBookmarkSelectedTabsCommand() -->
+    <command id="Browser:BookmarkSelectedTabs"
+             oncommand="PlacesCommandHook.bookmarkCurrentSelectedPages();"/>
     <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"/>
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -7897,22 +7897,30 @@ 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 or multiselected tab.
+    // 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.
+    // Update its state if visible.
+    let bookmarkMultiSelectedTabs = document.getElementById("context_bookmarkSelectedTabs");
+    bookmarkMultiSelectedTabs.hidden = !multiselectionContext;
+    if (!bookmarkMultiSelectedTabs.hidden)
+      PlacesCommandHook.updateBookmarkSelectedTabsCommand();
+
     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/browser.xul
+++ b/browser/base/content/browser.xul
@@ -1,28 +1,17 @@
 #filter substitution
 <?xml version="1.0"?>
 # -*- Mode: HTML -*-
 #
 # 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/.
 
-<?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/content/downloads/downloads.css"?>
-<?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/content/usercontext/usercontext.css" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/skin/controlcenter/panel.css" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/skin/customizableui/panelUI.css" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/skin/downloads/downloads.css"?>
-<?xml-stylesheet href="chrome://browser/skin/searchbar.css"?>
-<?xml-stylesheet href="chrome://browser/skin/places/places.css"?>
-<?xml-stylesheet href="chrome://browser/skin/places/editBookmark.css"?>
-<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
-<?xml-stylesheet href="chrome://browser/content/tabbrowser.css" type="text/css"?>
+<?xml-stylesheet href="chrome://browser/content/browser-window.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/compacttheme.css" type="text/css" alternate="yes" title="Light/Dark"?>
 
 # All DTD information is stored in a separate file so that it can be shared by
 # hiddenWindow.xul.
 <!DOCTYPE window [
 #include browser-doctype.inc
 ]>
 
@@ -136,16 +125,20 @@
             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"
+                label="&bookmarkSelectedTabs.label;" hidden="true"
+                accesskey="&bookmarkSelectedTabs.accesskey;"
+                command="Browser:BookmarkSelectedTabs"/>
       <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/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,62 @@
+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.uniqueCurrentSelectedPages.length, 1, "No more than one unique selected page");
+  is(menuItemBookmarkSelectedTabs.disabled, true, "Bookmark Selected Tabs is disabled");
+
+  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.uniqueCurrentSelectedPages.length, 2, "More than one unique selected page");
+  is(menuItemBookmarkSelectedTabs.disabled, false, "Bookmark Selected Tabs is disabled");
+
+  for (let tab of [tab1, tab2, tab3, tab4])
+    BrowserTestUtils.removeTab(tab);
+});
--- a/browser/base/jar.mn
+++ b/browser/base/jar.mn
@@ -50,16 +50,17 @@ browser.jar:
         content/browser/browser-safebrowsing.js       (content/browser-safebrowsing.js)
         content/browser/browser-sidebar.js            (content/browser-sidebar.js)
         content/browser/browser-siteIdentity.js       (content/browser-siteIdentity.js)
         content/browser/browser-sync.js               (content/browser-sync.js)
         content/browser/browser-tabsintitlebar.js       (content/browser-tabsintitlebar.js)
         content/browser/browser-thumbnails.js         (content/browser-thumbnails.js)
         content/browser/browser-trackingprotection.js (content/browser-trackingprotection.js)
         content/browser/browser-webrender.js          (content/browser-webrender.js)
+        content/browser/browser-window.css            (content/browser-window.css)
         content/browser/tab-content.js                (content/tab-content.js)
         content/browser/content.js                    (content/content.js)
         content/browser/defaultthemes/1.header.jpg    (content/defaultthemes/1.header.jpg)
         content/browser/defaultthemes/1.icon.jpg      (content/defaultthemes/1.icon.jpg)
         content/browser/defaultthemes/1.preview.jpg   (content/defaultthemes/1.preview.jpg)
         content/browser/defaultthemes/2.header.jpg    (content/defaultthemes/2.header.jpg)
         content/browser/defaultthemes/2.icon.jpg      (content/defaultthemes/2.icon.jpg)
         content/browser/defaultthemes/2.preview.jpg   (content/defaultthemes/2.preview.jpg)
--- a/browser/locales/en-US/chrome/browser/browser.dtd
+++ b/browser/locales/en-US/chrome/browser/browser.dtd
@@ -44,16 +44,19 @@ 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">