Closed
Bug 1458067
Opened 6 years ago
Closed 6 years ago
Implement ability to bookmark a selection of tabs
Categories
(Firefox :: Tabbed Browser, enhancement, P3)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 63
People
(Reporter: ablayelyfondou, Assigned: ablayelyfondou, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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.
Assignee | ||
Updated•6 years ago
|
Blocks: tabs-multiselect
Updated•6 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8989919 -
Flags: review?(mak77)
Comment 2•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 3•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8989919 -
Flags: review?(mak77)
Comment 5•6 years ago
|
||
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. :-)
Assignee | ||
Comment 6•6 years ago
|
||
Oops sorry, I forgot to add it to my commit. Thanks
Updated•6 years ago
|
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
Comment 7•6 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
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•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #14) > I'll take care of the landings. Yes, thank you.
Comment 16•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Attachment #8989919 -
Attachment is obsolete: true
Comment 19•6 years ago
|
||
mozreview-review |
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+
Comment 20•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/06633309e9f2 Implement ability to bookmark a selection of tabs. r=Gijs
Assignee | ||
Comment 21•6 years ago
|
||
mozreview-review-reply |
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•6 years ago
|
||
(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.
Assignee | ||
Comment 23•6 years ago
|
||
(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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06633309e9f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
status-firefox61:
affected → ---
Comment 25•6 years ago
|
||
Verified fixed that this implementation is present on the latest Nightly 65.0a1(2018-11-22)and the latest Beta 64.0b12 as well .
You need to log in
before you can comment on or make changes to this bug.
Description
•