Last Comment Bug 812762 - Use &brandShortName; instead of Firefox [gcli]
: Use &brandShortName; instead of Firefox [gcli]
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Graphic Commandline and Toolbar (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 22
Assigned To: Raymond Heldt
:
: Joe Walker [:jwalker] (needinfo me or ping on irc)
Mentors:
Depends on:
Blocks: 845279
  Show dependency treegraph
 
Reported: 2012-11-17 03:32 PST by Gion-Andri Cantieni [:gion-andri]
Modified: 2013-03-07 11:26 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Firefox changed to &BrandShortName in the files specified. (3.30 KB, patch)
2013-01-15 12:12 PST, Raymond Heldt
jaws: feedback+
Details | Diff | Splinter Review
Proposed Patch (5.08 KB, patch)
2013-02-14 15:50 PST, Raymond Heldt
jaws: review-
Details | Diff | Splinter Review
Proposed Patch (8.36 KB, patch)
2013-02-15 14:56 PST, Raymond Heldt
jaws: feedback+
Details | Diff | Splinter Review
Proposed Patch (Version 4) (13.17 KB, patch)
2013-02-21 09:27 PST, Raymond Heldt
jwalker: review+
Details | Diff | Splinter Review
Proposed Patch V5 (13.18 KB, patch)
2013-02-27 16:08 PST, Raymond Heldt
no flags Details | Diff | Splinter Review
Proposed Patch V6 (14.08 KB, patch)
2013-02-28 12:59 PST, Raymond Heldt
jwalker: review+
jaws: review+
Details | Diff | Splinter Review
v7 (13.99 KB, patch)
2013-03-04 08:00 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review

Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-11-19 12:20:41 PST
Yes, we should. Thanks for reporting it.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-21 02:54:20 PST
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Comment 3 Raymond Heldt 2013-01-15 12:12:23 PST
Created attachment 702454 [details] [diff] [review]
Firefox changed to &BrandShortName in the files specified.

For variable names that included the word "Firefox" in the middle of it, it still says "Firefox" so that we don't get an undefined variable; i.e. "restartFirefoxNocacheDesc" still reads "restartFirefoxNocacheDesc" and not "restart&BrandShortNameNocacheDesc"
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2013-01-15 12:37:58 PST
Comment on attachment 702454 [details] [diff] [review]
Firefox changed to &BrandShortName in the files specified.

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

For strings that have Firefox in the variable name, we should still change those, but as you found out we can't use brandShortName there. For those, you can just change 'Firefox' to 'Browser', but keep the casing as it was before.

::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
@@ +333,4 @@
>  # first opens the developer toolbar to explain the command line, and is shown
>  # each time it is opened until the user clicks the 'Got it!' button. This
>  # string is the opening paragraph of the intro text.
> +introTextOpening=The &BrandShortName command line is designed for developers. It focuses on speed of input over JavaScript syntax and a rich display over monospace output.

Were you able to test these changes? I can help you out with building and figuring out how to test these.

.properties files will not allow DTD entities to be placed within the string. You'll need to use special identifiers which can then be used by external localization functions to replace the identifier with a user-facing string.

For example, see the results for this search on MXR: http://mxr.mozilla.org/mozilla-central/find?text=social.remove.label&string=
Comment 5 Rob Campbell [:rc] (:robcee) 2013-02-12 11:21:48 PST
Jared's comments are good ones. This needs to be updated to use a formatting string and the method that uses it should be changed to getFormattedString.

needs an update!
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2013-02-14 15:16:33 PST
Comment on attachment 702454 [details] [diff] [review]
Firefox changed to &BrandShortName in the files specified.

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

::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
@@ +333,4 @@
>  # first opens the developer toolbar to explain the command line, and is shown
>  # each time it is opened until the user clicks the 'Got it!' button. This
>  # string is the opening paragraph of the intro text.
> +introTextOpening=The &BrandShortName command line is designed for developers. It focuses on speed of input over JavaScript syntax and a rich display over monospace output.

Raymond and I talked on IRC and he showed me that in browser/locales/en-US/installer/custom.properties $BrandShortName is used. I didn't know about this before, but if it works for you then that's cool with me. Note that you need to use a dollar sign here ($) and not an ampersand (&).

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +73,4 @@
>  # LOCALIZATION NOTE (screenshotChromeDesc) A very short string to describe
>  # the 'chrome' parameter to the 'screenshot' command, which is displayed in
>  # a dialog when the user is using this command.
> +screenshotChromeDesc=Capture &BrandShortName chrome window? (true/false)

These entities need to have a semicolon after them and a lower-case b, so &brandShortName;
Comment 7 Raymond Heldt 2013-02-14 15:50:10 PST
Created attachment 714146 [details] [diff] [review]
Proposed Patch

"Firefox" changed to "$BrandShortName" in strings and "Browser" as part of variable names.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2013-02-15 08:15:46 PST
Comment on attachment 714146 [details] [diff] [review]
Proposed Patch

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

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +140,3 @@
>  # describe the 'nocache' parameter to the 'restart' command, which is
>  # displayed in a dialog when the user is using this command.
> +restartBrowserNocacheDesc=Disables loading content from cache upon restart

If these names change then you also need to change the code that references them. Did you build with these changes and test it out?

Also, for all of the strings that required changes, the name will need to be updated so the localizers are aware that the string has changed and for them to include $BrandShortName.
Comment 9 Raymond Heldt 2013-02-15 14:56:53 PST
Created attachment 714621 [details] [diff] [review]
Proposed Patch

Majority of the bug is fixed in this patch. Rather than a hard-coded "Firefox", the name of whatever browser is being used (could still be "Firefox", could be "Nightly", could be whatever else the name is) is displayed where necessary to the user in the GCLI commands - with one exception: introTextOpening in gcli.properties still uses the hard-coded "Firefox" since the only other file that references it is gcli.jsm, which is auto-generated and not to be modified. Info on this would be appreciated.
Comment 10 Panos Astithas [:past] 2013-02-17 23:41:37 PST
The original sources for GCLI are located here:

https://github.com/joewalker/gcli

However, since it is meant to be used outside Firefox as well, I don't know if the localization setup we have here will be applicable in other contexts. I'm not sure how Joe wants to deal with this, so let's ask him directly.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-18 06:15:34 PST
(In reply to Panos Astithas [:past] from comment #10)
> The original sources for GCLI are located here:
> 
> https://github.com/joewalker/gcli
> 
> However, since it is meant to be used outside Firefox as well, I don't know
> if the localization setup we have here will be applicable in other contexts.
> I'm not sure how Joe wants to deal with this, so let's ask him directly.

Thanks for asking Raymond - I think that exhortation to not update the generated files is a bit heavy handed and unrealistic.

There are 2 options for you:
1. Ignore the message, update gcli.properties and send it to me for review, and I'll keep GCLI in sync
2. Check out the sources that Panos points you to, find the source of the offending string, and send me a pull request, and do step 1.

Thanks.
Comment 12 Raymond Heldt 2013-02-18 11:46:21 PST
(In reply to Joe Walker [:jwalker] from comment #11)
> (In reply to Panos Astithas [:past] from comment #10)
> > The original sources for GCLI are located here:
> > 
> > https://github.com/joewalker/gcli
> > 
> > However, since it is meant to be used outside Firefox as well, I don't know
> > if the localization setup we have here will be applicable in other contexts.
> > I'm not sure how Joe wants to deal with this, so let's ask him directly.
> 
> Thanks for asking Raymond - I think that exhortation to not update the
> generated files is a bit heavy handed and unrealistic.
> 
> There are 2 options for you:
> 1. Ignore the message, update gcli.properties and send it to me for review,
> and I'll keep GCLI in sync
> 2. Check out the sources that Panos points you to, find the source of the
> offending string, and send me a pull request, and do step 1.
> 
> Thanks.


Okay, well as far as doing step 1 goes, the only change that would be made to the file gcli.properties would just be switching the word "Firefox" to "%1$S" on line 336. That's the easy part. The thing I was having problems with was getting the string formatted elsewhere. If you look at my proposed patch, you'll see that CmdResize.jsm and BuiltInCommands.jsm are the files that actually fetch the browser name to format the strings in gclicommands.properties - it's just that neither of those files referenced introTextOpening from gcli.properties - that string was referenced ONLY by gcli.jsm

So, maybe I'm not understanding this correctly because it seems fairly trivial to accomplish step 1. Do you just want me to update gcli.properties (it would just be that one line modified, and I guess another line added in the comments above it explaining that the argument represents the name of the browser), send that to you for review, and then you'll update GCLI (including gcli.jsm) to format this particular string (introTextOpening) correctly? Sorry if I got that wrong.

Thanks.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2013-02-19 12:30:49 PST
Comment on attachment 714621 [details] [diff] [review]
Proposed Patch

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

Please include 8 lines of context on each side of the change in your next revision. From a glance this looks good, but you should request review from jwalker.
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-21 04:57:25 PST
(In reply to Raymond Heldt from comment #12)
> (In reply to Joe Walker [:jwalker] from comment #11)
> > (In reply to Panos Astithas [:past] from comment #10)
> > > The original sources for GCLI are located here:
> > > 
> > > https://github.com/joewalker/gcli
> > > 
> > > However, since it is meant to be used outside Firefox as well, I don't know
> > > if the localization setup we have here will be applicable in other contexts.
> > > I'm not sure how Joe wants to deal with this, so let's ask him directly.
> > 
> > Thanks for asking Raymond - I think that exhortation to not update the
> > generated files is a bit heavy handed and unrealistic.
> > 
> > There are 2 options for you:
> > 1. Ignore the message, update gcli.properties and send it to me for review,
> > and I'll keep GCLI in sync
> > 2. Check out the sources that Panos points you to, find the source of the
> > offending string, and send me a pull request, and do step 1.
> > 
> > Thanks.
> 
> 
> Okay, well as far as doing step 1 goes, the only change that would be made
> to the file gcli.properties would just be switching the word "Firefox" to
> "%1$S" on line 336. That's the easy part. The thing I was having problems
> with was getting the string formatted elsewhere. If you look at my proposed
> patch, you'll see that CmdResize.jsm and BuiltInCommands.jsm are the files
> that actually fetch the browser name to format the strings in
> gclicommands.properties - it's just that neither of those files referenced
> introTextOpening from gcli.properties - that string was referenced ONLY by
> gcli.jsm
> 
> So, maybe I'm not understanding this correctly because it seems fairly
> trivial to accomplish step 1. Do you just want me to update gcli.properties
> (it would just be that one line modified, and I guess another line added in
> the comments above it explaining that the argument represents the name of
> the browser), send that to you for review, and then you'll update GCLI
> (including gcli.jsm) to format this particular string (introTextOpening)
> correctly? Sorry if I got that wrong.

No - you got it right, I'd forgotten that this was read by gcli.jsm, sorry!

So can I suggest a hack:

# LOCALIZATION NOTE (introTextOpening): The 'intro text' opens when the user
# first opens the developer toolbar to explain the command line, and is shown
# each time it is opened until the user clicks the 'Got it!' button. This
# string is the opening paragraph of the intro text.
introTextOpening=This is a command line designed for developers. It focuses on speed of input over JavaScript syntax and a rich display over monospace output.

i.e. s/The Firefox command line is/This is a command line/

What do you think?
Comment 15 Raymond Heldt 2013-02-21 09:27:29 PST
Created attachment 716624 [details] [diff] [review]
Proposed Patch (Version 4)

This patch displays the browser name where necessary (not "Firefox" hard-coded), plus there was a workaround in displaying the browser name in introTextOpening from gcli.properties by just rewriting the string to say "This command line" instead of "The Firefox command line". Thanks to jwalker for that idea.

Also, 8 lines of context are included in the patch.
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-21 10:24:22 PST
Thanks Raymond. I'll get this landed for you.
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-22 06:35:17 PST
https://tbpl.mozilla.org/?tree=Try&rev=9f33fb88d95f
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-22 08:40:49 PST
https://hg.mozilla.org/integration/fx-team/rev/8f598aa5a7c2
Comment 19 Tim Taubert [:ttaubert] 2013-02-25 01:10:19 PST
https://hg.mozilla.org/mozilla-central/rev/8f598aa5a7c2
Comment 20 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-02-25 10:43:50 PST
Pike: Should the entity identifiers be changed? I only discovered the changes because of the warning that my translation lacks the placeholders in them.
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2013-02-25 10:50:04 PST
(In reply to Archaeopteryx [:aryx] from comment #20)
> Pike: Should the entity identifiers be changed? I only discovered the
> changes because of the warning that my translation lacks the placeholders in
> them.

Yes, they should have been changed and should still be changed so that localizers who haven't gotten around to translating yet will notice.

It appears that the following feedback was not addressed:

(In reply to Jared Wein [:jaws] from comment #8)
> Also, for all of the strings that required changes, the name will need to be
> updated so the localizers are aware that the string has changed and for them
> to include $BrandShortName.
Comment 22 Vlado Valastiak [:wladow] @ Mozilla.sk 2013-02-25 10:55:09 PST
This is bad. Please reopen, backout and recommit with properly altered entity identifiers. And please do not land similar L10n non-approved patches anymore. Thanks.
Comment 23 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-26 14:21:50 PST
Backout: https://hg.mozilla.org/integration/fx-team/rev/fb9a02b79254
Comment 24 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-26 14:45:24 PST
So Raymond, sorry I forgot to check the key changes.

The issue here is that localizers won't necessarily know to update the strings because their tools don't tell them about changes to existing strings, just about new strings. So in order to keep the localizations in sync, we have to change the keys when we make non-trivial changes to strings.

So rather than:
  -restartBrowserDesc=Restart %1$S
  +restartFirefoxDesc=Restart Firefox

We need to do:
  -restartBrowserDesc=Restart %1$S
  +restartFirefoxDesc2=Restart Firefox

And obviously make similar changes to the JS.

Sometimes people try to keep the names looking 'sensible' by doing a swap from restartBrowserDesc to restartBrowserTitle rather than restartBrowserDesc2 but in this case we're following a pattern where Desc means something specific as does restart etc.

Do you think you can make the changes? Or would you like someone else to?
Comment 25 Raymond Heldt 2013-02-26 15:23:59 PST
(In reply to Joe Walker [:jwalker] from comment #24)
> So Raymond, sorry I forgot to check the key changes.
> 
> The issue here is that localizers won't necessarily know to update the
> strings because their tools don't tell them about changes to existing
> strings, just about new strings. So in order to keep the localizations in
> sync, we have to change the keys when we make non-trivial changes to strings.
> 
> So rather than:
>   -restartBrowserDesc=Restart %1$S
>   +restartFirefoxDesc=Restart Firefox
> 
> We need to do:
>   -restartBrowserDesc=Restart %1$S
>   +restartFirefoxDesc2=Restart Firefox
> 
> And obviously make similar changes to the JS.
> 
> Sometimes people try to keep the names looking 'sensible' by doing a swap
> from restartBrowserDesc to restartBrowserTitle rather than
> restartBrowserDesc2 but in this case we're following a pattern where Desc
> means something specific as does restart etc.
> 
> Do you think you can make the changes? Or would you like someone else to?

Okay, I can get that taken care of. It seems simple enough.

But let me first just make sure I'm not going crazy here. This is what you said:

> So rather than:
>   -restartBrowserDesc=Restart %1$S
>   +restartFirefoxDesc=Restart Firefox
> 
> We need to do:
>   -restartBrowserDesc=Restart %1$S
>   +restartFirefoxDesc2=Restart Firefox

But if "-" means subtract and "+" means add, I would think it's the opposite of that. I took OUT the lines that used the term "Firefox" and added IN lines that used "Browser" and "%1S$".

So, is this what it's supposed to say?

> So rather than:
>   -restartFirefoxDesc=Restart Firefox
>   +restartBrowserDesc=Restart %1$S
> 
> We need to do:
>   -restartFirefoxDesc=Restart Firefox
>   +restartBrowserDesc2=Restart %1$S

Thanks.
Comment 26 Francesco Lodolo [:flod] (mostly out of office until Dec 19) 2013-02-26 22:13:37 PST
(In reply to Joe Walker [:jwalker] from comment #24)
> So rather than:
>   -restartBrowserDesc=Restart %1$S
>   +restartFirefoxDesc=Restart Firefox

Wrong example, the key changed here.

This is the right example (and there are other 3, if I didn't miss others)
-screenshotChromeDesc=Capture %1$S chrome window? (true/false)
+screenshotChromeDesc=Capture Firefox chrome window? (true/false)

BTW I opened bug 845279 to fix this since I was previously told that in devtools you don't reopen bugs :-\
Comment 27 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-02-26 23:25:06 PST
Thanks Flod, for making the point about key names:

So to be clear:
    -screenshotChromeDesc=Capture Firefox chrome window? (true/false)
    +screenshotChromeDesc=Capture %1$S chrome window? (true/false)

Becomes:
    -screenshotChromeDesc=Capture Firefox chrome window? (true/false)
    +screenshotChromeDesc2=Capture %1$S chrome window? (true/false)

(And I'll stop doing bugmail late at night!)
Comment 28 Raymond Heldt 2013-02-27 16:08:14 PST
Created attachment 719228 [details] [diff] [review]
Proposed Patch V5

Entity names changed where necessary. Specifically:

screenshotChromeDesc    -->  screenshotChromeDesc2
screenshotChromeManual  -->  screenshotChromeManual2
resizeModeManual        -->  resizeModeManual2

I thought there was more than just those 3, but after checking several times, I couldn't find any others that weren't already changed (like "Firefox" --> "Browser"). If I missed anything, please let me know.
Comment 29 Francesco Lodolo [:flod] (mostly out of office until Dec 19) 2013-02-27 22:31:30 PST
Missed this for sure

-introTextOpening=This command line is designed for developers. It focuses on speed of input over JavaScript syntax and a rich display over monospace output.
+introTextOpening=The Firefox command line is designed for developers. It focuses on speed of input over JavaScript syntax and a rich display over monospace output.
Comment 30 Raymond Heldt 2013-02-28 12:59:48 PST
Created attachment 719647 [details] [diff] [review]
Proposed Patch V6

(In reply to Francesco Lodolo [:flod] from comment #29)
> Missed this for sure
> 
> -introTextOpening=This command line is designed for developers. It focuses
> on speed of input over JavaScript syntax and a rich display over monospace
> output.
> +introTextOpening=The Firefox command line is designed for developers. It
> focuses on speed of input over JavaScript syntax and a rich display over
> monospace output.


Good catch. It's been changed to introTextOpening2 in both gcli.properties and gcli.jsm
Comment 31 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-01 02:21:27 PST
Comment on attachment 719647 [details] [diff] [review]
Proposed Patch V6

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

Looks good to me.
I think my r+ will also go for Jared, but I'll wait for a while before landing in case anyone else has comments.
Comment 32 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-04 07:54:23 PST
https://tbpl.mozilla.org/?tree=Try&rev=089f57640fc8
Comment 33 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-04 08:00:08 PST
Created attachment 720714 [details] [diff] [review]
v7

Rebased and added user header.
Comment 35 Panos Astithas [:past] 2013-03-06 23:29:05 PST
https://hg.mozilla.org/mozilla-central/rev/7eb5914cbb5d
Comment 36 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-03-07 03:08:21 PST
Thanks for the help, Raymond

Note You need to log in before you can comment on or make changes to this bug.