Closed Bug 1162283 Opened 4 years ago Closed 4 years ago

Add support for limited hard-coded localizations to Pocket

Categories

(Firefox :: Pocket, defect, P1)

defect
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: Dolske, Assigned: adw)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attached patch Patch with just strings (obsolete) — Splinter Review
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.
Hi Drew, can you provide a point value.
Iteration: --- → 40.3 - 11 May
Flags: qe-verify?
Flags: needinfo?(adw)
Flags: firefox-backlog+
Attached patch Patch with just strings (obsolete) — Splinter Review
Now with Spanish -- all 4 locales complete.
Attachment #8602880 - Attachment is obsolete: true
Blocks: Pocket
Priority: -- → P1
Submitted PR https://github.com/Pocket/Firefox/pull/51/ to Pocket for the strings in their code.
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/
Flags: needinfo?(adw)
Attachment #8603105 - Flags: review?(dolske)
Points: --- → 5
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.
(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 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.
Attachment #8603105 - Flags: review?(dolske) → review+
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.
Flags: needinfo?(l10n)
That was with browser.pocket.enabled=false.  Here's =true:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e6a21bb40a
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.
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?
Flags: needinfo?(l10n)
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.
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'
Flags: needinfo?(l10n)
That's packaging, which I have to confess, I don't understand.

glandium, gps, rstrong are good candidates.
Flags: needinfo?(l10n)
gps, please see comment 16.
Flags: needinfo?(gps)
glandium, please see comment 16.
Flags: needinfo?(mh+mozilla)
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
Attached patch change the DOM (obsolete) — Splinter Review
Attachment #8602973 - Attachment is obsolete: true
Attachment #8603105 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gps)
Attachment #8603599 - Flags: review?(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.
Attachment #8603599 - Flags: review?(dolske) → review+
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.
Attachment #8603599 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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 on attachment 8603650 [details] [diff] [review]
change the DOM v2

[Triage Comment]

Required for Pocket / 38.0.5 release.
Attachment #8603650 - Flags: approval-mozilla-release+
Attachment #8603650 - Flags: approval-mozilla-aurora+
QA Contact: andrei.vaida
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.