rename quitApplicationCmdMac.key to reflect reuse on Linux

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Menus
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: myk, Assigned: John)

Tracking

Trunk
Firefox 23
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=mconley][lang=xul])

Attachments

(1 attachment, 1 obsolete attachment)

2.46 KB, patch
mconley
: review+
mconley
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.

Updated

4 years ago
Whiteboard: [good first bug]

Updated

4 years ago
Whiteboard: [good first bug] → [good first bug][mentor=mconley][lang=xul]
(Assignee)

Comment 1

4 years ago
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"?

Updated

4 years ago
Flags: needinfo?(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!
Flags: needinfo?(mconley)
(Assignee)

Comment 3

4 years ago
Created attachment 725593 [details] [diff] [review]
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.
Attachment #725593 - Flags: feedback+
(Assignee)

Updated

4 years ago
Attachment #725593 - Flags: feedback+ → feedback?
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. -->
Attachment #725593 - Flags: feedback? → feedback+

Updated

4 years ago
Assignee: nobody → jlboezeman
(Assignee)

Comment 5

4 years ago
Created attachment 725610 [details] [diff] [review]
Proposed Patch v2

Fixed, built, tested on Linux (Ubuntu derivative) machine.
Attachment #725593 - Attachment is obsolete: true
Attachment #725610 - Flags: feedback?

Comment 6

4 years ago
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.
Attachment #725610 - Flags: feedback? → feedback?(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.
Attachment #725610 - Flags: review+
Attachment #725610 - Flags: feedback?(mconley)
Attachment #725610 - Flags: feedback+

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

4 years ago
(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?
(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!
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fb3df70550a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.