1161138 - localization of pocket doesn't use gecko l10n system

Status: RESOLVED FIXED

(Firefox :: Pocket, defect, P2)

Description

[Axel Hecht [:Pike]]

To localize the in-product UI for pocket via mozilla l10n processes, we'll need that code to load localized texts from .properties and .dtd files.

Right now, it seems they're getting their strings from http://mxr.mozilla.org/mozilla-central/source/browser/components/pocket/panels/js/dictionary.js, looking at http://mxr.mozilla.org/mozilla-central/search?string=Translations&find=pocket.

We have only a few of our integration string coming from http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pocket.properties

Comment 1

[Axel Hecht [:Pike]]

Not sure if this is a requirement to land localizations in mozilla-central. It's not a strict requirement, but either way, this complicates matters.

We'll need to get this aspect triaged and included into the schedule of the next few days.

Comment 2

[Francesco Lodolo [:flod]]

Side note on those strings: "..." should really be "…", some weird capitalization ("Sign Up with email").

Comment 3

[Justin Dolske [:Dolske]]

Code wise this is fairly trivial to do -- move our hard-coded .properties/.dtd file to the usual localizable place, and convert Pocket's dictionary.js to a .properties file.

The part I'm unsure about, because it's a new situation, is what to do with locales that are not supported by the Pocket website. We can expose the in-product strings to all localizers, but presumably we shouldn't enable the Pocket button (via browser.pocket.enabledLocales) until we know the Pocket website has matching support. [Do we want to keep .enabledLocales forever, or just until some threshold of major locales are covered?]

Other than the fact that Pocket will be expanding their set of supported languages, I don't think there are any specific product requirements here.

So, strawman proposal:

* Move strings to the usual spots, so all localizers can do translations
* L10N should coordinate with localizer community, to let them know plans for website translation, and that in-product translation is effectively optional until the website plans to add support.

Comment 4

[Justin Dolske [:Dolske]]

Bryan: do we know what the next set of supported locales/translations will be, and when they need to be available?

This may influence how we time these changes. For example, if we need to add support for language X and Y in Firefox 39, I think we'd want to keep the existing hard-coded strings and simply extend them for X+Y. But for things in the 40/41/beyond timeframe, we can switch to the usual L10N system.

Comment 5

[Axel Hecht [:Pike]]

My expectation is that we'll move from a scheme of pocket driving our set of localizations to us driving their set of localizations.

Chad, Mark, what's our product vision here for Hindi? And to be more extreme, for Upper Sorbian?

Once I know what to work towards, I can suggest technical steps on how to get there.

Comment 6

[chris hofmann]

lets construct a plan to get off the hack, and get to the regular process as quickly as possible.

4 firefox users walk into a pub in london or paris.   
a parisian, a german, a brit, and an american.

two of them start talking about pocket, and the other two seem confused.
then all 4 wonder if the folks with pocket have been hacked. when they talk
to more firefox folks surrounding they get even more confused about
what should be part of firefox and what should not be there.

not the kind of experience we want.

Comment 7

[Axel Hecht [:Pike]]

Looping in Nate, could have been a better idea to start with you.

Nate, what's your vision for people coming to your with language preferences like Hindi or Upper Sorbian? Is there something we can help with in getting there?

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Bryan: do we know what the next set of supported locales/translations will
> be, and when they need to be available?
> 
> This may influence how we time these changes. For example, if we need to add
> support for language X and Y in Firefox 39, I think we'd want to keep the
> existing hard-coded strings and simply extend them for X+Y. But for things
> in the 40/41/beyond timeframe, we can switch to the usual L10N system.

We have a small number of languages we'd like to get into 39 which Pocket and Mozilla have agreed to, I believe a set of about 20 languages by end of year.  I'm not sure that Pocket will be supporting the smaller language distributions, but that is a question for them to tackle.

Comment 9

[Justin Dolske [:Dolske]]

I feel like this bug fell off the tracks and is wandering into the weeds. The scope of this bug is how to get the in-product localization back in line with how we localize the rest of Firefox, so Pocket is less of a special case. I think my proposal in comment 3 was pretty clear, so that's what I'm going to proceed with.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This switches the existing .dtd and .properties stuff to the normal L10N flow. I'll convert the dictionary.js stuff in a separate patch.

(And just to be clear: the .enabledLocales pref isn't changing here.)

Comment 11

[Axel Hecht [:Pike]]

Dolske, we're on the same page, both in terms of being concerned and how to get back on track.

Bryan and I also talked about this on the firefox meeting today, and 41 is good enough for this work to land.

So in mozilla land, we're all aligned in one direction. I had poked you probably today, if things hadn't just fallen in place during the meeting.

Moving the needinfos over from our folks to Nate.

Nate, how can we go about enabling this feature across all Firefox localizations?

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.1, Part 2

Replace dictionary.js with browser-pocket.properties.

This was a little tricky, because the panel code is running as content, and thus can't access the stringbundle directly. So I just added an async message to slurp all the strings in the bundle and pass them over.

Other than removing dictionary.js itself, I kept as much of the existing structure as possible to minimize changes.

Hello and PDFjs use a more complex method to do this, which seems a bit like overkill for retrofitting into Pocket:

http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/web/l10n.js
http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/standalone/content/libs/l10n-gaia-02ca67948fe8.js#1379


Nate: does this seem like a reasonable change?

If you're looking to keep this code functional as a standalone addon, I think all you'll need to do is (1) modify chrome://browser/locale/browser-pocket.properties in main.js to point into your addon (eg chrome://pocket/locale/browser-pocket.properties), and (2) add the install.rdf and localized .properties files as described here: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localizing_an_extension#Adding_more_localizations

(It's been eons since I did this, so hopefully that's still how it works.)

Comment 13

[Justin Dolske [:Dolske]]

(In reply to Axel Hecht [:Pike] from comment #11)
> Dolske, we're on the same page, both in terms of being concerned and how to
> get back on track.

Great to hear. Also, can you reach out to the localizers for the existing Pocket locales, to help them import the hard-coded strings we previously shipped?

Comment 14

[Michael Wolf]

IMHO all Firefox locales should be included in Pocket because Pocket is an integral part of Firefox.

I am the translator for the extreme language Upper Sorbian and maybe even more extreme language Lower Sorbian. ;-)

Comment 15

[Nate Weiner]

Hey guys, sorry for the delay in responding here. Was slammed on email and missed this thread. 

There are good number of changes here to code that I didn't write so I'd like to get Nick and Michael to review this (likely Monday).

Dolske: Just for my context, what type of timing are we running on for trying to complete/ship this patch?

Comment 16

[Nate Weiner]

:dolske: 

Team looked this over, generally looks good. 

One thing to consider: 
Michael: "I could see one improvement in main.js. Currently every time the _initL10NMessageId comes the it loads the bundle and returns the strings. We could cache the strings and lookup if the strings are available we don’t have to load them from the bundle and return immediately otherwise load it."

One question:
Michael: "One question I would have is why he changed the place of the “panelIdFromURL" call, I assume because he need’s it earlier now because of sending the strings message, but just want to be sure he knows what he does."

Comment 17

[Justin Dolske [:Dolske]]

(In reply to Nate Weiner from comment #16)

> One thing to consider: 
> Michael: "I could see one improvement in main.js. Currently every time the
> _initL10NMessageId comes the it loads the bundle and returns the strings. We
> could cache the strings and lookup if the strings are available we don’t
> have to load them from the bundle and return immediately otherwise load it."

We could, but I'm not sure it would matter much. Especially given that we don't really do any caching for other places where we use a lot of strings, and just call GetStringFromName a bunch.

> One question:
> Michael: "One question I would have is why he changed the place of the
> “panelIdFromURL" call, I assume because he need’s it earlier now because of
> sending the strings message, but just want to be sure he knows what he does."

Yeah, I had to move this because it was otherwise only being set when the whole "overlay" was initialized, but I needed to get the strings before that happens (because messaging is async, but the overlay initialization is not)... The only place it's used is in addMessageListener / sendMessage, so moving it keeps it closer to where it's needed anyway.


(In reply to Nate Weiner from comment #15)

> Dolske: Just for my context, what type of timing are we running on for
> trying to complete/ship this patch?

No particular timing need, other than getting it landed in Firefox 41 before it uplifts at the end of the month.

Sounds like we're good to go now, though.

Comment 18

[Pulsebot]

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

Comment 19

[Justin Dolske [:Dolske]]

A couple of post-landing notes:

The enabled locales list (en-US de es-ES ja ja-JP-mac ru) remains the same, but until localizers copy over the previously-hardcoded strings into the Mozilla L10N system they will fallback to en-US strings. This is the normal case for newly added features on Nightly, but could be a little surprising to some users since this will look like a regression.

I don't think we're expecting to make any significant string changes between now and the Firefox 41 release, so if our L10N team can ask/help localizers to copy of the existing strings ASAP, that would minimize any user disruption on Nightly. (And this for sure should be completed before Beta/Release.)

Also, note that new locales will still need to be explicitly added to the browser.pocket.enabledLocales if we want to ship them. Localizers can test locally by simply flipping the browser.pocket.useLocaleList pref to false.

Comment 20

[Robert Kaiser]

(In reply to Justin Dolske [:Dolske] from comment #19)
> This is the normal
> case for newly added features on Nightly, but could be a little surprising
> to some users since this will look like a regression.

We shouldn't concern ourselves with that too much, it happens all the time that a few strings get slight changes along with a L10n ID change and are in US English on Nightly, maybe even Dev Edition, until the localizers catch up. Nightly users of localized builds are pretty low, let's leave the localizers to deal with how fast they catch up.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Filed bug 1188156 to turn on additional locales made available from the work in the bug.

Just to note that it appears a couple locales like en-GB and ja-* have yet to copy over their locale work from the internal files to the L10N system.  I can file another bug for that if useful.