Closed
Bug 1267320
Opened 8 years ago
Closed 8 years ago
Multiple spellings for telemetry extras
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
Firefox 49
People
(Reporter: liuche, Assigned: mfinkle, Mentored)
Details
(Whiteboard: [lang=java][good next bug])
Attachments
(2 files, 1 obsolete file)
23.36 KB,
image/png
|
Details | |
8.42 KB,
patch
|
mfinkle
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We're getting multiple spellings for telemetry extras, likely from string conversions from Enums in other locales. There should be a way to convert enums to only the en-US locale because we're using this for usage analysis and don't need locale separations. For example: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/LoginDoorHanger.java#86
Comment 1•8 years ago
|
||
http://developer.android.com/reference/java/lang/String.html#toLowerCase(java.util.Locale)
Assignee | ||
Comment 2•8 years ago
|
||
So I guess we need to decide: * Use the strings "as-is" which probably makes an ALLCAPS string * Use .toLowerCase(Locale.US) to create an English-friendly lowercase (the enums are all in English anyway) * Override the Enum's toString method and do the lowercase there, instead of in all uses.
Comment 3•8 years ago
|
||
Hello, I am a new contributor and i am willing to work on this bug. Please, Can you guide me to go forward with it.
Flags: needinfo?(liuche)
Comment 4•8 years ago
|
||
Hi, I would like to contribute in solving this bug. I am new to this, can you please guide me in how to proceed ahead?
Comment 5•8 years ago
|
||
This doesn't sound like a good first bug, since it requires some investigation.
Whiteboard: [lang=java][good first bug] → [lang=java][good next bug]
Assignee | ||
Comment 6•8 years ago
|
||
Here's a patch that uses the .toLowerCase(Locale.ENGLISH) approach
Assignee: nobody → mark.finkle
Attachment #8747758 -
Flags: review?(liuche)
Assignee | ||
Comment 7•8 years ago
|
||
Testing the patch, I see: Before patch, in en-US: D/Telemetry(32229): SendUIEvent: event = show.1 method = doorhanger timestamp = 8112936571 extras = login Before patch, in tr_TR: D/Telemetry( 1188): SendUIEvent: event = show.1 method = doorhanger timestamp = 8113023102 extras = logın After patch, in tr_TR: D/Telemetry( 2059): SendUIEvent: event = show.1 method = doorhanger timestamp = 8113106198 extras = login The 'i' in "login" looks as expected after the patch.
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8747758 [details] [diff] [review] doorhanger-telemetry-string-fix v0.1 Review of attachment 8747758 [details] [diff] [review]: ----------------------------------------------------------------- http://developer.android.com/reference/java/util/Locale.html#default_locale Android docs suggest using Locale.US, because it will be present on all devices. r+ with that change.
Attachment #8747758 -
Flags: review?(liuche) → review+
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(liuche)
Assignee | ||
Comment 9•8 years ago
|
||
Locale.ENGLISH -> Locale.US
Attachment #8747758 -
Attachment is obsolete: true
Attachment #8750022 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c1cb391d03b8
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c1cb391d03b8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8750022 [details] [diff] [review] doorhanger-telemetry-string-fix v0.2 Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: Multiple spellings for a single telemetry probe [Describe test coverage new/current, TreeHerder]: Running on Nightly [Risks and why]: Low risk. The change is limited to converting a telemetry probe text input to lowercase using US locale settings so the lower casing is done consistently across other locales. [String/UUID change made/needed]: none
Attachment #8750022 -
Flags: approval-mozilla-aurora?
Comment 13•8 years ago
|
||
Comment on attachment 8750022 [details] [diff] [review] doorhanger-telemetry-string-fix v0.2 Corrects lowercasing on telemetry, let's uplift.
Attachment #8750022 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•8 years ago
|
||
This would be good to verify. Chenxia can you confirm the fix worked either in nightly or once it lands on aurora?
Flags: needinfo?(liuche)
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4ab883b1ddd
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(liuche)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•