1458067 - Implement ability to bookmark a selection of tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Abdoulaye O. LY]

When a group of tabs are selected, right-clicking on any tab in the selection should:
 1. Provide a context menu option of "Bookmark selected tabs".
 2. The "Bookmark all tabs" option will be hidden.

Comment 1

[Abdoulaye O. LY]

Attachment: Bug 1458067 - Implement ability to bookmark a selection of tabs.

Review commit: https://reviewboard.mozilla.org/r/254928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254928/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/254928/#review261932

This generally looks OK to me, but I'd like mak to have a look, too, as it affects how the browser talks to places.

::: browser/base/content/browser-places.js:549
(Diff revision 1)
> +  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);
> +  },

For the 2 helper functions, instead I would make `bookmarkPages` take an argument (maybe boolean, maybe a string key) that indicated whether we wanted to bookmark selected or all pages, and then just get the set of pages inside `bookmarkPages` based on that argument, to reduce the duplication here. But I defer to mak, who might prefer this way for some reason. I'll add him as a reviewer.

::: browser/base/content/browser.xul:136
(Diff revision 1)
>                  tbattr="tabbrowser-multiple-visible"
>                  oncommand="gBrowser.reloadAllTabs();"/>
> +      <menuitem id="context_bookmarkSelectedTabs"
> +                label="&bookmarkSelectedTabs.label;" hidden="true"
> +                accesskey="&bookmarkSelectedTabs.accesskey;"
> +                command="Browser:BookmarkSelectedTabs"/>

I'm not sure adding a command for this helps - we could just have an `oncommand` attribute?

::: browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js:58
(Diff revision 1)
> +  // 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");

Nit: the message should read "is enabled", I think?

Comment 3

[Abdoulaye O. LY]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/254928/#review261932

Yes, great idea, Thank you!

Comment 4

[Abdoulaye O. LY]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254928/diff/1-2/

Comment 5

[:Gijs (he/him)]

Restored Marco's r?. Abdoulaye, if you amend your commit so it ends with r?Gijs,mak , it shouldn't get removed again when you push updates. :-)

Comment 6

[Abdoulaye O. LY]

Oops sorry, I forgot to add it to my commit.
Thanks

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/254928/#review263070

it looks great overall, I have a few suggestions to (imo) improve the code a bit

::: browser/base/content/browser-places.js:541
(Diff revision 2)
> +   * - Case PlacesCommandHook.bookmarkTabsEnum.ALL: all of the currently
> +   *   open pages in window.
> +   * - Case bookmarkTabsEnum.bookmarkTabsEnum.MULTI_SELECTED: all of the currently
> +   *   selected pages in window.
>     */
> -  bookmarkCurrentPages: function PCH_bookmarkCurrentPages() {
> +  bookmarkPages: function PCH_bookmarkPages(aBookmarkTabs) {

please while touching this convert it to a shorthand: just "bookmarkPages(....) {"

The enum at this stage looks like a forced abstraction, until we have the actual necessity to introduce further modes, I'd simplify this.

First, rename uniqueCurrentSelectedPages to just uniqueSelectedPages;

Second, make bookmarkPages directly take a "URIList" argument in input, so it will be:
PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueCurrentPages) or PlacesCommandHook.bookmarkPages(PlacesCommandHook.uniqueSelectedPages)

::: browser/base/content/browser-places.js:552
(Diff revision 2)
> +      case this.bookmarkTabsEnum.MULTI_SELECTED:
> +        pages = this.uniqueCurrentSelectedPages;
> +        break;
> +    }
>      if (pages.length > 1) {
>      PlacesUIUtils.showBookmarkDialog({ action: "add",

nit: could you please fix the indentation here while touching this code? (provided it's not mozreview kidding me)

::: browser/base/content/browser-places.js:554
(Diff revision 2)
> +        break;
> +    }
>      if (pages.length > 1) {
>      PlacesUIUtils.showBookmarkDialog({ action: "add",
>                                         type: "folder",
>                                         URIList: pages

and at that point you can use a shorthand here as well, just "URIList,"

::: browser/base/content/browser.js:7913
(Diff revision 2)
>  
>      // 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 multiselected.

"or multiselection."

::: browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js:1
(Diff revision 2)
> +const PREF_MULTISELECT_TABS = "browser.tabs.multiselect";

please add a PD license boilerplate from here https://www.mozilla.org/en-US/MPL/headers/

::: browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js:8
(Diff revision 2)
> +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;

Maybe I'm missing the reason to do otherwise, but couldn't you replace all of this method with a single and direct call to BrowserTestUtils.openNewForegroundTab?

Comment 8

[Abdoulaye O. LY]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/254928/#review263070

> Maybe I'm missing the reason to do otherwise, but couldn't you replace all of this method with a single and direct call to BrowserTestUtils.openNewForegroundTab?

Here we want to have control of tab opening animation which we can't I think with openNewForgeroundTab.

Comment 9

[Abdoulaye O. LY]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254928/diff/2-3/

Comment 10

[Abdoulaye O. LY]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/254928/#review263070

I updated the code and rebased it. So it looks a bit messy as some functions are now migrated from browser.js to tabbrowser.js in the meanwhile.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/254928/#review263344

::: browser/base/content/browser.xul:136
(Diff revision 3)
>        <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"

please move hidden attribute to its own line BEFORE label and accesskey (so they are close each other)

Comment 12

[Abdoulaye O. LY]

Comment on attachment 8989919 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254928/diff/3-4/

Comment 13

[Marco Bonardo [:mak]]

Do you need help landing this?

Comment 14

[Marco Bonardo [:mak]]

I'll take care of the landings.

Comment 15

[Abdoulaye O. LY]

(In reply to Marco Bonardo [::mak] from comment #14)
> I'll take care of the landings.

Yes, thank you.

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 02f5f430c88f9a9c185a5603b313c92fb6d5256a -d 86eaab5999fb: rebasing 473037:02f5f430c88f "Bug 1458067 - Implement ability to bookmark a selection of tabs. r=Gijs,mak" (tip)
merging browser/base/content/browser-places.js
merging browser/base/content/browser-sets.inc
merging browser/base/content/browser.xul
merging browser/base/content/tabbrowser.js
merging browser/base/content/test/tabs/browser.ini
warning: conflicts while merging browser/base/content/test/tabs/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Marco Bonardo [:mak]]

Attachment: Bug 1458067 - Implement ability to bookmark a selection of tabs.

Review commit: https://reviewboard.mozilla.org/r/257418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257418/

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8992559 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/257418/#review264306

Comment 19

[:Gijs (he/him)]

Comment on attachment 8992559 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/257418/#review264348

rs=me, I'm assuming you're just rebasing this.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06633309e9f2
Implement ability to bookmark a selection of tabs. r=Gijs

Comment 21

[Abdoulaye O. LY]

Comment on attachment 8992559 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/257418/#review264348

Yes, I should update this very soon.

Comment 22

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #19)
> rs=me, I'm assuming you're just rebasing this.

Yep, just rebased.

(In reply to Abdoulaye O. LY from comment #21)
> Yes, I should update this very soon.

Not necessary, I rebased it (was a trivial browser.ini conflict) and it landed.

Comment 23

[Abdoulaye O. LY]

(In reply to Marco Bonardo [::mak] from comment #22)
> Not necessary, I rebased it (was a trivial browser.ini conflict) and it
> landed.

Oops I just saw that you did it. Thank you.

Comment 24

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/06633309e9f2

Comment 25

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Verified fixed that this implementation is present on the latest Nightly 65.0a1(2018-11-22)and the latest Beta 64.0b12 as well .