Closed Bug 1188156 Opened 9 years ago Closed 9 years ago

Turn on locales available in our L10N and Pocket

Categories

(Firefox :: Pocket, defect)

Product:
Firefox
Component:
Pocket
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox41 --- verified
firefox42 --- fixed

People

(Reporter: clarkbw, Assigned: Dolske)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

Patch v.2
1.17 KB, patch
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
In bug 1161138 Pocket was switched over to the Mozilla L10N system.  This bug is for ensuring we turn on the remaining locales available to Pocket.  Many of these translations are already in the system, this change is targeting Firefox 42.

The current preference is: browser.pocket.enabledLocales = [en-US de es-ES ja ja-JP-mac ru]

Which we need to expand to include the following locales:

fr
pt-BR
zh-CN
en-GB
pl-PL
it
nl
zh-TW
es-MX
ko
cs
hu
pt-pl

Also, Pocket uses the code 'es-la' which I couldn't find in our locale list, hopefully the l10n crew can weigh in on which one that matches to.
This bug may not be necessary, bug 1168481 comment 11 indicated that instead of continuing to use the locale list we may want to open up to all locales even if Pocket does not support them.  I'll file a new bug for the pref flip and close then when done.
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #0)
> In bug 1161138 Pocket was switched over to the Mozilla L10N system.  This
> bug is for ensuring we turn on the remaining locales available to Pocket. 
> Many of these translations are already in the system, this change is
> targeting Firefox 42.
> 
> The current preference is: browser.pocket.enabledLocales = [en-US de es-ES
> ja ja-JP-mac ru]

Also en-GB and en-ZA (one of which is in your list below) from bug 1168481 -- didn't land these on Firefox 41+ because we we assuming we'd just disable the .enableLocaleList instead of updating the whitelist.

> pl-PL

Firefox just has "pl", not "pl-PL".

> pt-pl

Hmm, Firefox has a pt-PT, but not a pt-PL. I'm assuming the former is meant here, for "Portuguese (Portugal)".

> Also, Pocket uses the code 'es-la' which I couldn't find in our locale list,
> hopefully the l10n crew can weigh in on which one that matches to.

la as in "Latin America"?

We only have 4 es-* locales: es-ES (already shipped), es-ES (added here), es-AR, es-CL. Presumably the latter two [Spanish (Argentina) and Spanish (Chile)] are the possibilities to add here.
Attached patch Patch v.1 (obsolete) — — Splinter Review 
Updated locale list from comment 0 with corrections from comment 2. Did not include es-AR/es-CL.

Kinda seems like we should really just flip browser.pocket.useLocaleList, but either way is simple.
Assignee: nobody → dolske
Attachment #8640209 - Flags: review?(jaws)
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #0)

> Many of these translations are already in the system, this change is
> targeting Firefox 42.

Shouldn't this target 41? That's the release that bug 1161138 landed in (halfway through the Nightly cycle). We could double check to see if the relevant locales have actually done translations, but I'd expect them to do so, and so this should be fine to uplift to Aurora/41 in the next week or so (before it branches to beta).
Comment on attachment 8640209 [details] [diff] [review]
Patch v.1

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

::: browser/app/profile/firefox.js
@@ +1977,5 @@
>  pref("browser.pocket.api", "api.getpocket.com");
>  pref("browser.pocket.site", "getpocket.com");
>  pref("browser.pocket.oAuthConsumerKey", "40249-e88c401e1b1f2242d9e441c4");
>  pref("browser.pocket.useLocaleList", true);
> +pref("browser.pocket.enabledLocales", "cs de en-GB en-US en-ZA es-ES es-MX fr hu it ja ja-JP-mac ko nl pl-PL pt-BR pt-PT ru zh-CN zh-TW");

According to http://mxr.mozilla.org/mozilla-central/source/browser/locales/shipped-locales#60, there is no pl-PL locale. It is just listed as 'pl'.

r=me if you either confirm that pl-PL works or switch it to 'pl' and check that 'pl' works.
Attachment #8640209 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> According to
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/shipped-
> locales#60, there is no pl-PL locale. It is just listed as 'pl'.

And this was pointed out in comment #2 actually (just read it now). Any reason why this stayed in the patch or did this just get forgotten when you wrote the patch?
(In reply to Justin Dolske [:Dolske] from comment #4)
> (In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #0)
> 
> > Many of these translations are already in the system, this change is
> > targeting Firefox 42.
> 
> Shouldn't this target 41?

indeed.  maybe that was an off by one error?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)

> And this was pointed out in comment #2 actually (just read it now). Any
> reason why this stayed in the patch or did this just get forgotten when you
> wrote the patch?

Err, oops. I apparently just messed up when editing the list.
Attached patch Patch v.2 — — Splinter Review 
Fixed pl.
Attachment #8640209 - Attachment is obsolete: true
url:        https://hg.mozilla.org/integration/fx-team/rev/09573cae39d381301c36bda5b1563aca42453cf9
changeset:  09573cae39d381301c36bda5b1563aca42453cf9
user:       Justin Dolske <dolske@mozilla.com>
date:       Wed Jul 29 12:52:30 2015 -0700
description:
Bug 1188156 - Turn on locales available in our L10N and Pocket. r=jaws
Comment on attachment 8640674 [details] [diff] [review]
Patch v.2

Approval Request Comment
[Feature/regressing bug #]: Pocket
[User impact if declined]: Pocket not available in more locales.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: This basically is enabling Pocket in-product for the locales supported by the web service. Strings were added to the normal L10N flow in bug 1161138, so if this was any other Firefox feature we simply wouldn't have a whitelist at all.
[String/UUID change made/needed]: See comment 4. No string changes here, just  enabling locales.
Attachment #8640674 - Flags: approval-mozilla-aurora?
Comment on attachment 8640674 [details] [diff] [review]
Patch v.2

Approved. I checked with l10n to be sure and they have no concerns with uplifting this.
Attachment #8640674 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
I verified that Pocket is available in the locales added to 'browser.pocket.enabledLocales' across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit) using Firefox 41.0RC.
status-firefox41: fixed → verified
Verification on 41 RC should be enough here.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: