345603 - move Clear Recent History from the Tools to the History menu

Status: RESOLVED FIXED

Description

[Steffen Wilberg]

Bug 329750 moved Clear Private Data from the Tools menu to the History menu, but into the #ifdef MOZ_PLACES section. As places was disabled on the branch, bug 333930 added it back to the Tools menu for the #ifndef MOZ_PLACES case.

So the current status on the branch is this:

#ifdef MOZ_PLACES: History menu
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser-menubar.inc&rev=1.57.2.38&mark=327,328,366-369#327

#ifndef MOZ_PLACES: Tools menu
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser-menubar.inc&rev=1.57.2.38&mark=512,532-538#512

Should we finally move it to the History menu?

Comment 1

[Steffen Wilberg]

This is no longer valid.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Why is this no longer valid?  (I've got a patch that moves it, if we want…)

Thanks,
Blake.

Comment 3

[Marco Bonardo [::mak]]

we should still do this.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Created attachment 708633 [details] [diff] [review]
Move the menu item.

Cool, here's a patch!  (I set Dao as the reviewer, because he's extra-thorough, and because I like him, but feel free to steal the review, if you want, Mak.)

Thanks,
Blake.

Comment 5

[Dão Gottwald [::dao]]

Comment on attachment 708633 [details] [diff] [review]
Move the menu item.

>--- a/browser/base/content/browser-menubar.inc
>+++ b/browser/base/content/browser-menubar.inc
>@@ -360,17 +360,22 @@
>                 <menuseparator id="historyMenuHomeSeparator"
>                                class="show-only-for-keyboard"/>
>                 <menuitem id="menu_showAllHistory"
>                           label="&showAllHistoryCmd2.label;"
> #ifndef XP_MACOSX
>                           key="showAllHistoryKb"
> #endif
>                           command="Browser:ShowAllHistory"/>
>-                <menuseparator id="showAllHistorySeparator"/>
>+                <menuitem id="sanitizeItem"
>+                          accesskey="&clearRecentHistory.accesskey;"
>+                          label="&clearRecentHistory.label;"
>+                          key="key_sanitize"
>+                          command="Tools:Sanitize"/>
>+                <menuseparator id="sanitizeSeparator"/>

The history menu intentionally doesn't have access keys, so don't add one here and also remove clearRecentHistory.accesskey from browser.dtd.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Created attachment 708660 [details] [diff] [review]
Version 2 of the patch to move the menu item.

Done and done.  Thank you!

Comment 7

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 708660 [details] [diff] [review]
Version 2 of the patch to move the menu item.

Hooray!

Comment 8

[:Ehsan Akhgari]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb1cc4ca87f

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bfb1cc4ca87f

Comment 10

[bhavana bajaj [:bajaj]]

CCing, :tyler & :Matt to help cover as part of Support, if we see anything around this instead of release noting .