420371 - rename quitApplicationCmdMac.key to reflect reuse on Linux

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

The fix for bug 189290 will reuse the quitApplicationCmdMac.key entity for Linux because it's late in the Fx3 cycle to be taking changes to these entities, but ultimately that should be called something like quitApplicationCmdUnix.key or perhaps just quitApplicationCmd.key.

Comment 1

[John]

Hello, I have never contributed to Mozilla and would like to take this bug if it still needs addressing. If it does still need addressing, the patch should look something like: 
-key="&quitApplicationCmdMac.key;"
+key="&quitApplicationCmdUnix.key;"
for all instances of "quitApplicationCmdMac", correct? Also, should this extend to instances that are "quitApplicationCmdMac\..*"  (note that only the "quitApplicationCmdMac" would be replaced, not anything after) or only "quitApplicationCmdMac.key"?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey John! Thanks for your interest in this bug!

So I think this is what I think we should do:

Here's what needs to be done:

quitApplicationCmdMac.key usage should be changed to quitApplicationCmdUnix.key in here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#388

We also need to change the place where we define what quitApplicationCmdMac.key is - that's over here:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#539

So that entity name will need to be changed to.

(In reply to John from comment #1)
> Also, should this
> extend to instances that are "quitApplicationCmdMac\..*"  (note that only
> the "quitApplicationCmdMac" would be replaced, not anything after) or only
> "quitApplicationCmdMac.key"?

No, I think quitApplicationCmdMac.label is still necessary. In this case, I think we only care about the keyboard shortcut.

Once you've got a Firefox build up and running (https://developer.mozilla.org/en/docs/Simple_Firefox_build), make those changes, and then execute this command:

./mach build browser

And then restart your browser, making sure the change went through without breaking the keyboard shortcut.

Once you've verified that, we'll need a patch from you. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F is a good primer for that, but I'm happy to help you through it when you get there.

I'm mconley on irc.mozilla.org - come and find me if you have any questions, or send me email.

Thanks!

Comment 3

[John]

Attachment: Proposed Patch

(In reply to Mike Conley (:mconley) from comment #2)
> quitApplicationCmdMac.key usage should be changed to
> quitApplicationCmdUnix.key in here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> sets.inc#388
> 
> We also need to change the place where we define what
> quitApplicationCmdMac.key is - that's over here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#539

I have done this, built the browser, and tested it on my Linux (ubuntu derivative) machine.

I also noticed from searching the repository that "quitApplicationCmdMac.key" is used twice in webapprt, should these be changed as well?

/browser/locales/en-US/webapprt/webapp.dtd

    line 21 -- <!ENTITY quitApplicationCmdMac.key "Q">

/webapprt/content/webapp.xul

    line 48 -- key="&quitApplicationCmdMac.key;"

I have not changed these in my current patch.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 725593 [details] [diff] [review]
Proposed Patch

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

Looks good John! Just two suggestions - see below.

(In reply to John from comment #3)
> Created attachment 725593 [details] [diff] [review]
> Proposed Patch
> 
> I also noticed from searching the repository that
> "quitApplicationCmdMac.key" is used twice in webapprt, should these be
> changed as well?
>

No, let's file a separate bug for them.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +535,5 @@
>  <!ENTITY goForwardCmd.commandKey "]">
>  <!ENTITY quitApplicationCmd.label       "Quit"> 
>  <!ENTITY quitApplicationCmd.accesskey   "Q">
>  <!ENTITY quitApplicationCmdMac.label    "Quit &brandShortName;">
> +<!ENTITY quitApplicationCmdUnix.key      "Q">

Nit - please make this line up with the items above by removing one of those space characters.

Also, can you add this comment above:

<!-- LOCALIZATION NOTE(quitApplicationCmdUnix.key): This keyboard shortcut is used by both Linux and OSX builds. -->

Comment 5

[John]

Attachment: Proposed Patch v2

Fixed, built, tested on Linux (Ubuntu derivative) machine.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 725610 [details] [diff] [review]
Proposed Patch v2

John, not your fault but feedback and review requests with no target ("to the wind") tend to go unnoticed.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 725610 [details] [diff] [review]
Proposed Patch v2

Sorry for the delay - this looks good to me! r=me, while I'm at it.

Comment 8

[John]

(In reply to Mike Conley (:mconley) from comment #7)
> Comment on attachment 725610 [details] [diff] [review]
> Proposed Patch v2
> 
> Sorry for the delay - this looks good to me! r=me, while I'm at it.

Alright, great. Is there anything I need still need to do or am I good?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to John from comment #8)
> (In reply to Mike Conley (:mconley) from comment #7)
> > Comment on attachment 725610 [details] [diff] [review]
> > Proposed Patch v2
> > 
> > Sorry for the delay - this looks good to me! r=me, while I'm at it.
> 
> Alright, great. Is there anything I need still need to do or am I good?

Nope, I don't believe so. I've set checkin-needed on the bug, so someone will come by and commit it for you soon. Assuming that all goes smoothly, your job is done. :) Just keep an eye on the bug in case it doesn't, for some reason, go smoothly.

Thanks for the patch!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb3df70550a

Thanks for the patch, John! You'll see this bug resolved in the next day or so once this patch is uplifted to mozilla-central.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fb3df70550a