Closed Bug 1015041 Opened 6 years ago Closed 6 years ago

Migrate code for pseudo localization from build/l10n.js to runtime

Categories

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: yurenju, Assigned: stas)

References

Details

Attachments

(1 file, 6 obsolete files)

we will get pseudo localization featureafter 900182 landed, but it not be a part of optimization process (webapp-optimize.js) in gaia build system.

we should migrate it from l10n.js which is loaded in webapp-optimize.js to multilocale.js.
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(yurenju.mozilla)
Assignee: nobody → yurenju.mozilla
Status: NEW → ASSIGNED
move code from l10n.js to pseudo-l10n.js and add unit tests.

Stas, could you take a look to it? I'm not l10n expert so please let me know if I miss something.
Attachment #8443358 - Flags: feedback?(stas)
Comment on attachment 8443358 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/20786

Hi Yuren, thanks for taking a stab at this.  Before I have actual feedback for you, I'd like to provide some context and ask for your opinion.

The original l10n code lives in the l20n/l20n.js repository on the 'gaia' branch.  All changes land there, then we build with 'grunt' and copy the following files:

 - dist/runtime/l10n.js to shared/js/l10n.js
 - dist/buildtime/l10n.js to build/l10n.js
 - tests/lib to apps/sharedtests/test/unit/l10n/lib

For example, the pseudo locales logic lives here:

https://github.com/l20n/l20n.js/blob/a8901508d6a291dd1604bb59cab43f848ce6fad7/lib/l20n/pseudo.js

And tests are here:

https://github.com/l20n/l20n.js/blob/a8901508d6a291dd1604bb59cab43f848ce6fad7/tests/lib/pseudo_test.js
https://github.com/l20n/l20n.js/blob/a8901508d6a291dd1604bb59cab43f848ce6fad7/tests/lib/util_test.js

They are copied to:

https://github.com/mozilla-b2g/gaia/tree/ef280abd3891cf44a8b33e58f623c9bce8501209/apps/sharedtest/test/unit/l10n/lib

One challenge of making a change like the one which you propose is to make sure things continue to work both in Gaia and in the L20n.js repository, especially tests.

Another thing to note is that build/l10n.js is a special thin layer over shared/js/l10n.js overriding a few functions to make them work for the buildtime optimizations.  As such, it is already quite specific to the logic of webapp-optimize.

I have to do some more thinking about the idea of putting the pseudo logic in (or after) the multilocale step.  First, it felt like it belonged there.  But I'm now not sure if we should generate the .properties files for pseudolocales at all, at least not right now.

First of all, I'd like to make pseudolocales work with DEBUG=1 builds, which means that they will have to be generated dynamically on runtime (I filed bug 1028127 for this).

Secondly, I'm not fond of the idea of using a regexp to replace the value part of en-US properties files.  Using the AST as we do now is a cleaner approach and is also forward-compatible with our plan to migrate away from .properties entirely, and use .l20n instead (bug 1022859).

Here's another proposal:  move pseudolocales logic into shared/js/l10n.js (it's only a handful of functions) and don't generate .properties files for them at all.  This way:

 - for DEBUG=1 builds, pseudo-translations will be generated on the fly, whenever mozL10n.get is called,
 - for GAIA_CONCAT_LOCALES=0, we won't have the .properties files for pseudolocales, but, just like above, they will be generated on the fly,
 - for GAIA_CONCAT_LOCALES=1, we can use the existing optimization to generate the JSON files for pseudolocales to make things faster.

This has the additional advantage of bringing the pseudolocales technology to third-party developers using l10n.js, which will help them with localization testing just as it helps s today.

Yuren, Pike, what do you think?
Attachment #8443358 - Flags: feedback?(stas)
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(l10n)
Just a few things to add:

I hit the road end with where pseudolocales are currently located when working on bug 1022767 comment 12. It would be great if this change could allow me to extend build_stage's locale manifests.

I'm not convinced that moving pseudo-locales code to runtime is a good idea. First of all, pseudo-locales is not "handful of functions", closer to ~170 lines of code [*]. Second, unless we do want to do runtime magic, pseudo-locales is a build time optimization which generates additional locale resource file. I think it's easy to understand and work with this way. If you'll want to generate it on fly, in memory, it will be less representative for how real l10n's work (FOUC's may not be detected etc.) and harder to grasp for devs.


[*] https://github.com/l20n/l20n.js/commit/f3e76d8e0da1ec1ee2839ffc408e6d33192f25dc#diff-28fd3f7f03f7db47d5f47c997dbca367
hi stas & gandalf,

I'm good with Stas's solution.

from my view for build system, I would like to ensure code would be putted to right place, so (1) pseudo-locales shouldn't be a part of optimization then we need to move it into multilcoale or extract to another build step just like my pr. or (2) if we move pseudo-locales to shared/js/l10n.js and preload pseudo locales in webapp-optimize.js for optimization, it also works to me.

and (1) also can solve issue for using pseudo-locale in DEBUG=1 mode.
Flags: needinfo?(yurenju.mozilla)
yeah, sorry, not build time *optimization*, just build time *operation*.
(I don't have much to add at this point)
Flags: needinfo?(l10n)
:gandalf & :stas, I need your opinions to move forward on this issue :)

should we move pseudo l10n to runtime or should we migarate it to pseudo-l10n module in build time?
Flags: needinfo?(stas)
Flags: needinfo?(gandalf)
My preference is to move pseudo l10n to runtime (shared/js/l10n.js).  I've been in a work week during past few days so fairly busy, but I can come up with a patch early next week.
Flags: needinfo?(stas)
I also would like to see it in runtime.
Flags: needinfo?(gandalf)
Attached patch Move pseudo l10n to runtime (obsolete) — Splinter Review
TBPL: https://tbpl.mozilla.org/?rev=17ab80a39b74&tree=Gaia-Try

The patch moves the pseudol10n logic into shared/js/l10n.js and moves its application from mozL10n.get to Locale.prototype.addAST.

Gandalf, do you have any better ideas how to optimize Locale.prototype.addAST than what I did in the patch?

Yuren, are you happy with this separation or would you like to move some part of this to multilocale.js?
Attachment #8450410 - Flags: review?(gandalf)
Attachment #8450410 - Flags: feedback?(yurenju.mozilla)
Comment on attachment 8450410 [details] [diff] [review]
Move pseudo l10n to runtime

Stas, I'm happy with this patch, thanks!
Attachment #8450410 - Flags: feedback?(yurenju.mozilla) → feedback+
Assignee: yurenju.mozilla → nobody
Component: Gaia::Build → Gaia::L10n
Summary: Migrate code for pseudo localization from l10n.js to multilocale.js → Migrate code for pseudo localization from build/l10n.js to runtime
Thanks, Yuren.  I'm taking the bug then.
Assignee: nobody → stas
Priority: -- → P2
Duplicate of this bug: 1028127
Comment on attachment 8450410 [details] [diff] [review]
Move pseudo l10n to runtime

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

Three notes:

 - the patch will need an update when I land bug 1022859
 - we should test perf impact of this change
 - you may consider factorizing the code into a function/class. This way the pseudo locale objects will not be even created in non-pseudo scenarios.

I'm ok with this patch regardless of the notes.
Attachment #8450410 - Flags: review?(gandalf) → review+
I realized that by making Locale always download files for the default locale, I'm not taking advantage of the buildtime optimization of parsing l10n files into JSONs for pseudolocales.

I could make it so for .json resLinks l10n.js downloads foo.qps-ploc.json, and for .properties resLinks it downloads foo.en-US.proeprties (or whatever the default lang is).  However, this assumes that the buildtime optimization will always produce JSONs for the pseudolocales, which may not be the case, especially in the future when I'd like to have a checkbox in the Settings' Developer panel which turns them on and off.

An alternative is to always generate pseudlocales on the fly, regardless of the buildtime configuration.  This, of course, will imply a small perf hit when the Locale's AST is generated.  I'll test and see how much is it, realistically.

Zibi, do you have any other ideas?
Flags: needinfo?(gandalf)
Here's the implementation of the idea that Gandalf and I discussed on IRC earlier today.  It removes all buildtime optimizations for pseudolocales.  Everything is done on runtime.

The plan is to get this landed, and then add a setting in the Developer menu in the Settings app which can toggle pseudolocales on and off.

With everything moved into the runtime, pseudolocalizations are 200-300ms slower according to make test-perf.  

I'll explore adding a Makefile flag which would turn the buildtime optimizations on to reduce this difference in a separate bug.

One test is failing in build/test/unit/webapp-optimize.test.js.  I'll figure out how to fix it on Monday.
Attachment #8443358 - Attachment is obsolete: true
Attachment #8450410 - Attachment is obsolete: true
Attachment #8454419 - Flags: review?(gandalf)
Flags: needinfo?(gandalf)
Comment on attachment 8454419 [details] [diff] [review]
Part 1. Don't build JSONs for pseudolocales

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

::: build/l10n.js
@@ +91,5 @@
> +    } else {
> +      for (id in ast) {
> +        this.entries[id] = ast[id];
> +        this.ast[id] = ast[id];
> +      }

Why is this code in build/l10n.js? My understanding is that we want to move away from doing anything for qps on buildtime.

::: build/webapp-optimize.js
@@ +150,5 @@
> +      continue;
> +    }
> +    if (!this.webapp.dictionary[lang]) {
> +      this.webapp.dictionary[lang] = {};
> +    }

Is this related to pseudolocales?
(In reply to Zibi Braniecki [:gandalf] from comment #18)

> ::: build/l10n.js
> @@ +91,5 @@
> > +    } else {
> > +      for (id in ast) {
> > +        this.entries[id] = ast[id];
> > +        this.ast[id] = ast[id];
> > +      }
> 
> Why is this code in build/l10n.js? My understanding is that we want to move
> away from doing anything for qps on buildtime.

In the patch, I still support pretranslation for qps in order to allow GAIA_DEFAULT_LOCALE=qps-* to work correctly.  So I still need to modify the default locale into a pseudolocale somewhere in the build time logic.  We're already overriding addAST to provide this.ast, and I decided to put the pseudol10n logic there.

I considered turning off pretranslation for pseudolocales, but decided against it.  I didn't want to modify the isPretranslated logic, but maybe this could be a solution?

> 
> ::: build/webapp-optimize.js
> @@ +150,5 @@
> > +      continue;
> > +    }
> > +    if (!this.webapp.dictionary[lang]) {
> > +      this.webapp.dictionary[lang] = {};
> > +    }
> 
> Is this related to pseudolocales?

Yes, I don't want webapp-optimize to create empty <script> tags for qps, and the way it does it is to create one for every key in this.webapp.dictionary.
I tried turning pretranslation off for pseudolocales and it works remarkably well.  It makes me think we should consider turning it off for all locales :)  Here's a patch to be applied on top of the previous one.
Attachment #8455339 - Flags: review?(gandalf)
Attachment #8454419 - Attachment description: Don't do any buildtime optimizations for pseudol10n → Part 1. Don't build JSONs for pseudolocales
Here's one more patch to make this complete:  we need to build the inline JSON for the default locale to make qps work with the inline localization.  This causes a slight increase of the size of HTML right now, but the problem will go away when bug 994370 lands.
Attachment #8455352 - Flags: review?(gandalf)
Attached patch All 3 parts as one patch (obsolete) — Splinter Review
Comment on attachment 8455380 [details] [diff] [review]
All 3 parts as one patch

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

good work Stas!
Attachment #8455380 - Flags: review+
Thanks.  I'll cancel other review requests.  I rebased the patch and I'm still looking for a solution to fix the two broken webapp-optimize unit tests.
Attachment #8454419 - Flags: review?(gandalf)
Attachment #8455339 - Flags: review?(gandalf)
Attachment #8455352 - Flags: review?(gandalf)
This is the same patch with build unit tests fixed.  Yuren, can you review this for me, please?  I needed to change the place in webapp-optimize where webapp.dictionary members are created so that 'qps-ploc' and 'qps-plocm' don't end up as enumerable keys.

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=1a26a675af1e
https://travis-ci.org/mozilla-b2g/gaia/builds/30167241
Attachment #8454419 - Attachment is obsolete: true
Attachment #8455339 - Attachment is obsolete: true
Attachment #8455352 - Attachment is obsolete: true
Attachment #8455380 - Attachment is obsolete: true
Attachment #8458014 - Flags: review?(yurenju.mozilla)
Comment on attachment 8458014 [details] [diff] [review]
Same patch, with tests fixed

Stas, it's really hard for me to review this patch since I don't have enough background information for l10n, but for build script part, it looks good and works well even simpler for l10n part, so I would give f+ instead of r+ and this patch is ready to land.
Attachment #8458014 - Flags: review?(yurenju.mozilla) → feedback+
Thanks, Yuren!  Landed on master:

Commit: https://github.com/mozilla-b2g/gaia/commit/e17a47524c55893bd7e3ebc9d600d6e7d072f8f5
Merged: https://github.com/mozilla-b2g/gaia/commit/728824fcbb94e9ffe36d58111e34e5065735b1c2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I filed bug 1041565 to make pseudolocales toggleable in the Settings' Developer panel now that they are generated dynamically on runtime.
Blocks: 1011519
You need to log in before you can comment on or make changes to this bug.