812762 - Use &brandShortName; instead of Firefox [gcli]

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Gion-Andri Cantieni [:gion-andri]]

Shouldn't we use &BrandShortName; instead of Firefox in these files?

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/gcli.properties 

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/gclicommands.properties

Comment 1

[Joe Walker [:jwalker]]

Yes, we should. Thanks for reporting it.

Comment 2

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 3

[Raymond Heldt]

Attachment: 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)]

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)]

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)]

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]

Attachment: Proposed Patch

"Firefox" changed to "$BrandShortName" in strings and "Browser" as part of variable names.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: 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 (he/him) [:past] (please ni?)]

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]]

(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]

(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)]

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]]

(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]

Attachment: 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]]

Thanks Raymond. I'll get this landed for you.

Comment 17

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=9f33fb88d95f

Comment 18

[Joe Walker [:jwalker]]

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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8f598aa5a7c2

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)]

(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]]

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]]

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

Comment 24

[Joe Walker [:jwalker]]

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]

(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]]

(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]]

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]

Attachment: 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]]

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]

Attachment: 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]]

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]]

https://tbpl.mozilla.org/?tree=Try&rev=089f57640fc8

Comment 33

[Joe Walker [:jwalker]]

Attachment: v7

Rebased and added user header.

Comment 34

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=7eb5914cbb5d
https://hg.mozilla.org/integration/fx-team/rev/7eb5914cbb5d

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/7eb5914cbb5d

Comment 36

[Joe Walker [:jwalker]]

Thanks for the help, Raymond