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

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aryx, Assigned: goma, Mentored)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

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.
This is from bug 1086252. Georg, do you want to fix this on Nightly now or maybe turn this into a mentor bug?
Flags: needinfo?(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.
Mentor: gfritzsche
Flags: needinfo?(gfritzsche)
Whiteboard: [lang=js]
mhoye for heads-up.
Flags: needinfo?(mhoye)

Comment 4

4 years ago
Got it, thanks!
Flags: needinfo?(mhoye)
(Assignee)

Comment 5

4 years ago
I'd like to work on this bug. 
If it's possible
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.
(Assignee)

Comment 7

4 years ago
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 ??
Flags: needinfo?(gfritzsche)
Yes, the string currently hard-coded needs to be added as a new string in the .properties file to get properly localized.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 9

4 years ago
Created attachment 8559136 [details] [diff] [review]
bug-1117889-fix.patch
Flags: needinfo?(gfritzsche)
Attachment #8559136 - Flags: review?(gfritzsche)
Attachment #8559136 - Flags: review?(francesco.lodolo)
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.
Attachment #8559136 - Flags: review?(francesco.lodolo) → feedback+
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.
Attachment #8559136 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 12

4 years ago
Created attachment 8559187 [details] [diff] [review]
bug-1117889-fix-v2.patch
Attachment #8559136 - Attachment is obsolete: true
Attachment #8559187 - Flags: review?(gfritzsche)
(Assignee)

Comment 13

4 years ago
gfritzsche:
I've tested it by running the new browser in my computer, but I haven't used any automatic test system.
Flags: needinfo?(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 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
Attachment #8559187 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8559259 [details] [diff] [review]
bug-1117889-fix-v3.patch

I've updated the commit message
Attachment #8559187 - Attachment is obsolete: true
Attachment #8559259 - Flags: review?(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.
Attachment #8559259 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f0ff5147c6e1
Assignee: nobody → gbrmachado
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f0ff5147c6e1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.