1172320 - Browser Content Toolbox should have an accesskey

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150604162752

Steps to reproduce:

I use the browser content toolbox a lot, but it is quite hard to access it.
I like using the accesskeys (like Alt-twe in order to access the Browser Toolbox), and I'd like to be able to do so for the Browser Content Toolbox, like Alt-twx.

Florent

Comment 1

[Florent Fayolle]

Attachment: 1172320.patch

Paul, I am not sure you're the right person to ask a review for that, but I think that could be quite easy to review :) (unless the accesskey choice has to be discussed).

Thanks in advance!

Florent

Comment 2

[Florent Fayolle]

Attachment: 1172320.patch

Pff, not even having tested before submitting my patch, sorry. Patch updated and works now.

Florent

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8616457 [details] [diff] [review]
1172320.patch

Review of attachment 8616457 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +274,5 @@
>  <!-- LOCALIZATION NOTE (browserContentToolboxMenu.label): This is the label for the
>    -  application menu item that opens the browser content toolbox UI in the Tools menu.
>    -  This toolbox allows to debug the chrome of the content process in multiprocess builds.  -->
>  <!ENTITY browserContentToolboxMenu.label     "Browser Content Toolbox">
> +<!ENTITY browserContentToolboxMenu.accesskey     "x">

Nit: can you line up the "x" right below the previous string value?

Comment 4

[Florent Fayolle]

Attachment: 1172320.patch

Done

Florent

Comment 5

[Florent Fayolle]

One last question: is a try-push mandatory in all cases? (for this patch, it doesn't seem useful to make a try-push, but maybe the process is strict about it?)

Thanks in advance

Florent

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to fayolle-florent from comment #5)
> One last question: is a try-push mandatory in all cases? (for this patch, it
> doesn't seem useful to make a try-push, but maybe the process is strict
> about it?)
> 
> Thanks in advance
> 
> Florent

I've seen weird unrelated breakage before when changing keyboard shortcuts, but I think in this case you would be safe to request checkin without a full try run.  From https://wiki.mozilla.org/Tree_Rules/Integration#What_are_the_tree_rules_for_integration_repositories.3F:

Integration repositories are no replacement for Try. You still need to test your patches before pushing. (This doesn't mean that you need an all-platforms try run for every patch. But it does mean that you should do enough testing so that you rarely cause red or orange on the integration repository. But breaking it rarely is ok. Never missing a plane means you're spending too much time in airports; never breaking the tree means you're running too many tests before landing.)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Can you please make sure that future patches include your name and email address as well? If you run ./mach mercurial-setup, it'll help you do that.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fdaad6bb56fd

Comment 9

[Florent Fayolle]

Sorry, I thought I have already done that, but I'll check that.

Florent

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Carmen]

I was able to reproduce the issue using Aurora 40.0a2 (2015-06-07) on Win10 Pro x64.

I verified the fix on latest Aurora (2016-09-19) on Win 10 Pro x64 and the shortcut works fine.
On Ubuntu 16.04 LTS x64 and Mac OS X the shortcut is not working.