1162283 - Add support for limited hard-coded localizations to Pocket

Status: RESOLVED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Justin Dolske [:Dolske]]

The in-product Pocket code is currently shipping hard-coded en-US strings, outside of the usual localizable places. We'd like to extend this to include Russian, Japanese, Spanish, and German. Also outside of the normal L10N flow, since this is an exceptional/special situation with a short timeframe.

Specifically, we currently have:

  browser/base/content/browser-pocket.dtd
  browser/base/content/browser-pocket.properties
  browser/components/pocket/panels/js/dictionary.js

(Note that these are not under the usual /browser/locales/en-US/... tree.)

The short-tem fix here, as I understand it, will be to do the following:

1) Add browser-pocket-$LOCALE.properties files for ru/jp/es/de.

2) Add run-time detection (similar to the browser.pocket.useLocaleList check in CustomizableWidgets.jsm) to pick the correct .properties file to use.

3) Figure out a solution for doing effectively the same thing for the .dtd strings. I suspect it will be easiest to move these strings to the .properties files, and do runtime DOM manipulation to jam them into the right places (menuitem labels in the context menu / bookmarks menu).

4) The dictionary.js file comes from Pocket, but we probably need to suggest how to use the solution in #2 here so they can do the same.

The actual strings for ru/jp/es/de are being localized in dependent bugs. We'll need to take care of landing the localized strings into the right files.

Comment 1

[Justin Dolske [:Dolske]]

Oh:

5) Add ru/jp/es/de to browser.pocket.enabledLocales, but we can do that in a followup bug if it's easier / works better for the timing of all this.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch with just strings

Quick patch to add the relevant strings from the dependent bugs. Still waiting on Spanish from bug 1161842, so I just copied in the English strings for the moment.

Comment 3

[Marco Mucci [:MarcoM]]

Hi Drew, can you provide a point value.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch with just strings

Now with Spanish -- all 4 locales complete.

Comment 5

[Justin Dolske [:Dolske]]

Submitted PR https://github.com/Pocket/Firefox/pull/51/ to Pocket for the strings in their code.

Comment 6

[Drew Willcoxon :adw]

Attachment: add locale chrome manifest entries

OK, this works.  Instead of changing the DOM, this sets up new chrome manifest locales just for Pocket and these languages and updates the chrome URIs to point to the files.  To test it, I made language packs for all four non-en-US languages [1], installed the resulting XPIs in my build of Firefox, added the locales to browser.pocket.enabledLocales, and changed locales in Firefox with Benjamin's Locale Switcher add-on [2].  I also did the same for a non-localized locale, fr, and observed that the en-US strings were used.

[1] https://developer.mozilla.org/en-US/docs/Creating_a_Language_Pack
[2] https://addons.mozilla.org/en-Us/firefox/addon/locale-switcher/

Comment 7

[Drew Willcoxon :adw]

Next time we need to do something like this I think it would be way easier and better to commit the specific localizations to their respective repos in http://hg.mozilla.org/l10n-central/, put the en-US localizations where they normally go, but add LOCALIZATION NOTES in the files that say don't actually localize the strings.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Drew Willcoxon :adw from comment #7)
> Next time we need to do something like this [...]

AIUI the reason we didn't do this is because it the L10N dashboards for all locales light up with warnings about untranslated strings, and causes pain for localizers. Ideally the tools would have a automagic way to say "we're really sorry, just ignore this file for version X", but apparently that's difficult.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 8603105 [details] [diff] [review]
add locale chrome manifest entries

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

Elegant -- I like it!

Would be "nice" if we could user chrome://browser/locale/... instead of chrome://pocket/locale... with this trick, so that restoring normal localization is as simple as moving the .dtd/.properties files and removing the jar.mn manifest. But also fine as-is.

Comment 10

[Justin Dolske [:Dolske]]

NI / FYI to Pike, just in case he knows of any obscure cases here that are going to cause significant breakage with product or tools. But let's go ahead an land optimistically.

Comment 11

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db0c55dec2a5

Comment 12

[Drew Willcoxon :adw]

That was with browser.pocket.enabled=false.  Here's =true:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e6a21bb40a

Comment 13

[Justin Dolske [:Dolske]]

Status update: Integration branches are closed due test-failures and backlog, and other infra problems make it look like landing straight to m-c are also unlikely this evening.

Comment 14

[Axel Hecht [:Pike]]

Thanks for checking, this is what I expected, and shouldn't surprise any tools.

Per email, the strings in dictionary.js are coming in outside of this bug, is there one that tracks that?

Comment 15

[Axel Hecht [:Pike]]

One thing I forgot, the package name is global, maybe just "pocket" is something other add-ons use? Not sure what'd happen in that case.

Comment 16

[Drew Willcoxon :adw]

Pike, the tryserver builds for Linux opt, Linux x64 opt, Windows XP opt, and Windows 8 x64 opt failed with the following error.  (See treeherder links in comment 11 and 12.)  Do you understand it?

> 20:56:20     INFO -  INFO:root:processing /builds/slave/try-lx-00000000000000000000000/build/src/browser/branding/nightly/locales/jar.mn
> 20:56:20     INFO -  INFO:root:adding 'browser' entry to root chrome manifest appid={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> 20:56:20     INFO -  gmake[6]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/branding/nightly/locales'
> 20:56:20     INFO -  gmake[6]: Entering directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/branding/nightly/locales'
> 20:56:20     INFO -  gmake[6]: Nothing to be done for `tools'.
> 20:56:20     INFO -  gmake[6]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/branding/nightly/locales'
> 20:56:20     INFO -  gmake[5]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/branding/nightly/locales'
> 20:56:20     INFO -  /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/_virtualenv/bin/python /builds/slave/try-lx-00000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/dist/l10n-stage/firefox ../../dist/xpi-stage/locale-x-test \
> 20:56:20     INFO -  	 \
> 20:56:22     INFO -  Error: Multiple locales aren't supported
> 20:56:22     INFO -  Error: Locale doesn't have a manifest entry for 'pocket'
> 20:56:22     INFO -  Error: Locale doesn't have a manifest entry for 'pocket'
> 20:56:22     INFO -  Error: Locale doesn't have a manifest entry for 'pocket'
> 20:56:22     INFO -  Error: Locale doesn't have a manifest entry for 'pocket'
> 20:56:22     INFO -  Error: Locale doesn't have a manifest entry for 'pocket'
> 20:56:23     INFO -  Traceback (most recent call last):
> 20:56:23     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py", line 60, in <module>
> 20:56:23     INFO -      main()
> 20:56:23     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py", line 56, in main
> 20:56:23     INFO -      non_resources=args.non_resource, non_chrome=NON_CHROME)
> 20:56:23     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/python/mozbuild/mozpack/packager/l10n.py", line 245, in repack
> 20:56:23     INFO -      _repack(app_finder, l10n_finder, copier, formatter, non_chrome)
> 20:56:23     INFO -    File "/tools/python27/lib/python2.7/contextlib.py", line 24, in __exit__
> 20:56:23     INFO -      self.gen.next()
> 20:56:23     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 129, in accumulate
> 20:56:23     INFO -      raise AccumulatedErrors()
> 20:56:23     INFO -  mozpack.errors.AccumulatedErrors
> 20:56:23     INFO -  gmake[4]: *** [repackage-zip] Error 1
> 20:56:23     INFO -  gmake[4]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/locales'
> 20:56:23     INFO -  gmake[3]: *** [repackage-zip-x-test] Error 2
> 20:56:23     INFO -  gmake[3]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/locales'
> 20:56:23     INFO -  gmake[2]: *** [l10n-check] Error 2
> 20:56:23     INFO -  gmake[2]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/browser/locales'
> 20:56:23     INFO -  gmake[1]: *** [l10n-check] Error 2
> 20:56:23     INFO -  gmake[1]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox'
> 20:56:23     INFO -  gmake: *** [automation/l10n-check] Error 2
> 20:56:23     INFO -  gmake: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox'

Comment 17

[Axel Hecht [:Pike]]

That's packaging, which I have to confess, I don't understand.

glandium, gps, rstrong are good candidates.

Comment 18

[Drew Willcoxon :adw]

gps, please see comment 16.

Comment 19

[Drew Willcoxon :adw]

glandium, please see comment 16.

Comment 20

[Drew Willcoxon :adw]

app.locales contains: ru de en-US es-ES ja
l10n.locales contains: ru

_repack expects app.locales to have only one item.

I can reproduce locally when I run this step from https://developer.mozilla.org/en-US/docs/Creating_a_Language_Pack:

make installers-ru LOCALE_MERGEDIR=/Users/adw/fx/obj-debug/browser/locales/mergedir

Comment 21

[Drew Willcoxon :adw]

Attachment: change the DOM

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 8603599 [details] [diff] [review]
change the DOM

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

Can you add a comment to the catch{} cases in browser-places.js and nsContenxtMenu.js noting that we fallback to the already-in-place english strings from the .dtd? It may be a little non-obvious.

Comment 23

[Drew Willcoxon :adw]

Attachment: change the DOM v2

Addresses your comment and also removes the strings in browser-pocket.properties (en-US) that were copied over from browser-pocket.dtd since they're not needed in the en-US case, since the DTD in the markup is used then.

Comment 24

[Drew Willcoxon :adw]

browser.pocket.enabled=false:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e322a22deab

browser.pocket.enabled=true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7432ceae5889

Comment 25

[Pulsebot]

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

Comment 26

[Justin Dolske [:Dolske]]

FTR: we shifted plans here, to the patch that landed, due to schedule pressure -- wanted to send beta1 to build this weekend with all the L10N stuff working.

Comment 27

[Justin Dolske [:Dolske]]

Comment on attachment 8603650 [details] [diff] [review]
change the DOM v2

[Triage Comment]

Required for Pocket / 38.0.5 release.

Comment 28

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/79814b970d85
https://hg.mozilla.org/releases/mozilla-release/rev/e7c47480555d