Closed Bug 1458067 Opened 3 years ago Closed 3 years ago

Implement ability to bookmark a selection of tabs

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed
firefox64 --- verified
firefox65 --- verified

People

(Reporter: ablayelyfondou, Assigned: ablayelyfondou, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1458067 - Implement ability to bookmark a selection of tabs.
59 bytes, text/x-review-board-request
Gijs
: review+
mak
: review+
Details 
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.
Blocks: tabs-multiselect
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8989919 - Flags: review?(mak77)
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?
Attachment #8989919 - Flags: review?(gijskruitbosch+bugs) → review+
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!
Attachment #8989919 - Flags: review?(mak77)
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. :-)
Oops sorry, I forgot to add it to my commit.
Thanks
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
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?
Attachment #8989919 - Flags: review?(mak77) → review-
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 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 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)
Attachment #8989919 - Flags: review?(mak77) → review+
Do you need help landing this?
Flags: needinfo?(ablayelyfondou)
I'll take care of the landings.
Flags: needinfo?(ablayelyfondou)
(In reply to Marco Bonardo [::mak] from comment #14)
> I'll take care of the landings.

Yes, thank you.
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 on attachment 8992559 [details]
Bug 1458067 - Implement ability to bookmark a selection of tabs.

https://reviewboard.mozilla.org/r/257418/#review264306
Attachment #8992559 - Flags: review?(mak77) → review+
Attachment #8989919 - Attachment is obsolete: true
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.
Attachment #8992559 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/06633309e9f2
Implement ability to bookmark a selection of tabs. r=Gijs
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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/06633309e9f2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
No longer depends on: 1477758
Verified fixed that this implementation is present on the latest Nightly 65.0a1(2018-11-22)and the latest Beta 64.0b12 as well .
Status: RESOLVED → VERIFIED
status-firefox64: --- → verified
status-firefox65: --- → verified
You need to log in before you can comment on or make changes to this bug.