1117889 - Hardcoded English (en-US) string "General data\n" in about:telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

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

There is a harcoded caption in http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js#121

caption.appendChild(document.createTextNode("General data\n"));

This should likely be the same string like in
http://hg.mozilla.org/mozilla-central/file/default/toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd#l15

<!ENTITY aboutTelemetry.generalDataSection "
  General Data
">

but in aboutTelemetry.properties.

Comment 1

[Vladan Djeric (:vladan)]

This is from bug 1086252. Georg, do you want to fix this on Nightly now or maybe turn this into a mentor bug?

Comment 2

[Georg Fritzsche [:gfritzsche]]

I can mentor this.

We need to change the code in aboutTelemetry.js as aryx described.
To get the string for the name, we can use bundle.GetStringFromName().

This can be seen and verified by navigating to about:telemetry.

Comment 3

[Georg Fritzsche [:gfritzsche]]

mhoye for heads-up.

Comment 4

[Mike Hoye [:mhoye]]

Got it, thanks!

Comment 5

[Gabriel Machado [:goma]]

I'd like to work on this bug. 
If it's possible

Comment 6

[Georg Fritzsche [:gfritzsche]]

Sure, do you have Firefox built yet?
Be sure to check out the first steps here:
https://developer.mozilla.org/en-US/docs/Introduction

For actually fixing it:
* comment 0 describes the problem and proposes a fix
* for that we need to use bundle.GetStringFromName() instead of using a raw string
* navigate to about:telemetry to check things work properly
* upload a patch for feedback or review when you have something

If you have any questions, please ask.

Comment 7

[Gabriel Machado [:goma]]

Hello.
I have a questions.
The bundle object gets the strings from aboutTelemetry.properties file.
However, there is no string corresponding with "General Data" at this file.
Should I add another String at this file? Something like
generalDataTitle = General Data
and then use the bundle.GetStringFromName() method ??

Comment 8

[Francesco Lodolo [:flod]]

Yes, the string currently hard-coded needs to be added as a new string in the .properties file to get properly localized.

Comment 9

[Gabriel Machado [:goma]]

Attachment: bug-1117889-fix.patch

Comment 10

[Francesco Lodolo [:flod]]

Comment on attachment 8559136 [details] [diff] [review]
bug-1117889-fix.patch

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

Looks good as far as I can tell (not a reviewer though). 
Not sure if the new line is needed, given how many lines over 80 characters are in that file.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8559136 [details] [diff] [review]
bug-1117889-fix.patch

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

Thanks, this looks good. I only have two minor suggestions below.
Did you have a chance to test this?

::: toolkit/content/aboutTelemetry.js
@@ +119,5 @@
>      let table = document.createElement("table");
>  
>      let caption = document.createElement("caption");
> +    caption.appendChild(
> +            document.createTextNode(bundle.GetStringFromName("generalDataTitle") + "\n"));

How about we just do this instead:
let captionString = bundle.GetStringFromName...
caption.appendChild...

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +10,5 @@
>  generalDataHeadingName = Name
>  
>  generalDataHeadingValue = Value
>  
> +generalDataTitle = General Data

Nit: I would move this up before the HeadingName.

Comment 12

[Gabriel Machado [:goma]]

Attachment: bug-1117889-fix-v2.patch

Comment 13

[Gabriel Machado [:goma]]

gfritzsche:
I've tested it by running the new browser in my computer, but I haven't used any automatic test system.

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to Gabriel O. Machado [:goma] from comment #13)
> I've tested it by running the new browser in my computer, but I haven't used
> any automatic test system.

Ok, that is fine then. We don't have any automated test for the about:telemetry page itself.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8559187 [details] [diff] [review]
bug-1117889-fix-v2.patch

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

This looks good, thanks for the patch!
I think you should update the commit message though - it should be more about what you fixed, not so much about the detailed steps.
E.g. "Fix hard-coded general data caption in about:telemetry.".

Note that i quickly checked this locally and this is simple enough to not require the try run mentioned here:
https://developer.mozilla.org/en-US/docs/Introduction#Step_6_-_Actually_get_the_code_into_the_tree

Comment 16

[Gabriel Machado [:goma]]

Attachment: bug-1117889-fix-v3.patch

I've updated the commit message

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8559259 [details] [diff] [review]
bug-1117889-fix-v3.patch

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

Thank you, this looks fine now and is good to go.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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