Closed Bug 1164407 Opened 5 years ago Closed 5 years ago

Pocket not enabled on ja builds under Mac OS X

Categories

(Firefox :: Pocket, defect, P1)

38 Branch
All
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox38.0.5 + verified
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: petruta.rasa, Assigned: Dolske)

References

Details

(Whiteboard: [fixed by Bug 1164698])

Attachments

(2 files, 1 obsolete file)

Pocket is not enabled by default on 38.0.5 beta 1 - ja build under Mac OS X 10.9.5.

Japan mac builds are named ja-JP-mac while the .enabledLocales pref contains only the "ja" string:
browser.pocket.enabledLocales;en-US de es-ES ja ru

Modifying the pref and restarting the browser adds the Pocket icon to the navigation bar.
Do the strings inside the panel work? Looking at "addtags" for example, from http://mxr.mozilla.org/mozilla-beta/source/browser/components/pocket/panels/js/dictionary.js#92 ?
Attached image ja strings
Strings seems fine to me.
[Tracking Requested - why for this release]:

Thanks for testing.

Flagging for release to get ja-JP-mac added to the prefs.
Yuck.

We should add ja-JP-mac to the enabledLocales, fix the .properties file loading to handle this case (so it doesn't try to load "browser-pocket-ja-JP-mac.properties"), and make sure Pocket updates dictionary.js to handle this as well.

Isn't there a bug to get rid of this weird locale? It's a footgun. :(
Priority: -- → P1
(In reply to Petruta Rasa [QA] [:petruta] from comment #2)
> Created attachment 8605205 [details]
> ja strings
> 
> Strings seems fine to me.

Err. You should still be seeing en-US strings in a few places  (eg the bookmarks menu for View Pocket List and content menu for Save Page to Pocket).

Surprising that the Pocket panel is working... *looks*

Ah, seems Pocket's main.js :: getUILocale() is doing the expected form of detection, but pktApi.js still using window.navigator.language (which is presumably "ja" and note "ja-JP-mac", as I don't see any special handling for that.) While that happens to make Japanese work for this bug, it's broken in other cases because the differing language detection can cause make the Pocket panel use a language inconsistent from the rest of the browser chrome.
(In reply to Justin Dolske [:Dolske] from comment #4)
> Isn't there a bug to get rid of this weird locale? It's a footgun. :(

There's not, and until we have a product that's fully on top of l20n, or something equally powerful, we'll likely need to keep it around. The computer language in Japan is just different on Macs and everything else.
Attached patch Patch v.1 (obsolete) — Splinter Review
Assignee: nobody → dolske
Attachment #8605572 - Flags: review?(jaws)
Tracking the Pocket changes in bug 1164698, which we'll land at the same time.
Comment on attachment 8605572 [details] [diff] [review]
Patch v.1

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

Drive-by r+, looks good to me.
Attachment #8605572 - Flags: review+
Attached patch Patch v.2Splinter Review
Thanks for the driveby!

Updated the patch because it was actually broken, I cut'n'paste the last bit too quickly. (Needed to s/locale/browserLocale/ to get the variable name right.)
Attachment #8605572 - Attachment is obsolete: true
Attachment #8605572 - Flags: review?(jaws)
Comment on attachment 8605586 [details] [diff] [review]
Patch v.2

[Triage Comment]

a+ for all channels, required for Pocket launch in 38.0.5.
Attachment #8605586 - Flags: approval-mozilla-release+
Attachment #8605586 - Flags: approval-mozilla-beta+
Attachment #8605586 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/cba955e6adb2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Flags: qe-verify+
QA Contact: andrei.vaida
QA Contact: andrei.vaida → petruta.rasa
Whiteboard: [fixed by Bug 1164698]
Verified as fixed using Firefox 38.0.5 beta 2 build under Mac OS X 10.9.5: Pocket is installed by default on ja builds.
Removing qe-verify+ flag since this verification should suffice.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.