587295 - Remove "Bookmark This Tab" from the tabs context menu

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

From bug 587285:

> Bookmark This Tab (never use this, but probably gets used)

Also allows bookmarking a background tab, but this doesn't seem like an
important use case.

> Bookmark All Tabs... (seems rather niche)

It's duplicated in the bookmarks menu, so it might not be needed here.

Removing the two items about bookmarking seems okay to me.

Comment 1

[u88484]

Attachment: Patch v1

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

"Bookmark This Tab" in the Tab Context Menu (like "Bookmark This Page" on the old Menu) opens the Bookmark Panel, what allows you to Bookmark *and* classify (Name, Folder, Tag, Cancel) as a kind of "One-Click-Action" (as apposed to the Star-Button).

Saying this, the Removal should be bound to adding this Menu Entry to (or moving to) the new Bookmark Menu, no?

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 465989 [details] [diff] [review]
Patch v1

get ui review first

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Bookmark all tabs is about to get pulled out of the bookmarks menu (otherwise we are looking at possibly having 5 items before you get to an actual bookmark).  I agree that Bookmark this Tab is needlessly redundant though.

recent tweak to the bookmarks menu: https://bug583386.bugzilla.mozilla.org/attachment.cgi?id=466079

Comment 5

[u88484]

Comment on attachment 465989 [details] [diff] [review]
Patch v1

(In reply to comment #4)
> Bookmark all tabs is about to get pulled out of the bookmarks menu (otherwise
> we are looking at possibly having 5 items before you get to an actual
> bookmark).  I agree that Bookmark this Tab is needlessly redundant though.
> 
> recent tweak to the bookmarks menu:
> https://bug583386.bugzilla.mozilla.org/attachment.cgi?id=466079

So does that mean you want to remove 'Bookmark This Tab' and keep 'Bookmark All Tabs'?

Comment 6

[u88484]

(In reply to comment #5)
> Comment on attachment 465989 [details] [diff] [review]
> Patch v1
> 
> (In reply to comment #4)
> > Bookmark all tabs is about to get pulled out of the bookmarks menu (otherwise
> > we are looking at possibly having 5 items before you get to an actual
> > bookmark).  I agree that Bookmark this Tab is needlessly redundant though.
> > 
> > recent tweak to the bookmarks menu:
> > https://bug583386.bugzilla.mozilla.org/attachment.cgi?id=466079
> 
> So does that mean you want to remove 'Bookmark This Tab' and keep 'Bookmark All
> Tabs'?
Sorry.  If so, what about the menu separator because it will look a little strange to have a menu separator - Bookmark All Tabs - menu separator.  Is there any data on how often the command is even used?  I've personally never, in my at least 6 or 7 year use of Firefox, used that command.  I could see someone wanting to use it to maybe bookmark a bunch of pictures though.

Comment 7

[Dirkjan Ochtman (:djc)]

See also bug 587285. :)

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

>So does that mean you want to remove 'Bookmark This Tab' and keep 'Bookmark All
>Tabs'?

yeah

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 465989 [details] [diff] [review]
Patch v1

need to keep bookmark all tabs since this is the only place it will appear.

Comment 10

[u88484]

(In reply to comment #9)
> Comment on attachment 465989 [details] [diff] [review]
> Patch v1
> 
> need to keep bookmark all tabs since this is the only place it will appear.

What about the separator?  Are we going to leave one menu item between two separators in the middle of a context menu?

Comment 11

[u88484]

Attachment: Patch v2

Addresses Alex's ui-review by only removing 'Bookmark This Tab' from the tab context menu.  Still need an answer from Alex about the menuseparator though but this patch leaves the menuseparator.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 468838 [details] [diff] [review]
Patch v2

>-      <menuitem id="context_bookmarkTab"
>-                label="&bookmarkThisTab.label;"
>-                accesskey="&bookmarkThisTab.accesskey;"
>-                oncommand="BookmarkThisTab(TabContextMenu.contextTab);"/>

This makes the BookmarkThisTab function unused. We should remove it from browser.js.

Comment 13

[u88484]

Attachment: Patch v3

(In reply to comment #12)
> Comment on attachment 468838 [details] [diff] [review]
> Patch v2
> 
> >-      <menuitem id="context_bookmarkTab"
> >-                label="&bookmarkThisTab.label;"
> >-                accesskey="&bookmarkThisTab.accesskey;"
> >-                oncommand="BookmarkThisTab(TabContextMenu.contextTab);"/>
> 
> This makes the BookmarkThisTab function unused. We should remove it from
> browser.js.

Removed.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/134cbff3a612

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100922 Firefox/4.0b7pre

Litmus test has been updated:
https://litmus.mozilla.org/show_test.cgi?id=10010