Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) Reporter
	Description • 10 years ago

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.

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) Reporter
	Comment 1 • 10 years ago

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.

	Staś Małolepszy :stas Assignee
	Comment 2 • 10 years ago

Comment on attachment 8443358 [details]
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?

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 3 • 10 years ago

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

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) Reporter
	Comment 4 • 10 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 5 • 10 years ago

yeah, sorry, not build time *optimization*, just build time *operation*.

	Axel Hecht [:Pike]
	Comment 6 • 10 years ago

(I don't have much to add at this point)

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) Reporter
	Comment 7 • 10 years ago

: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?

	Staś Małolepszy :stas Assignee
	Comment 8 • 10 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 9 • 10 years ago

I also would like to see it in runtime.

	Staś Małolepszy :stas Assignee
	Comment 10 • 10 years ago

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?

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) Reporter
	Comment 11 • 10 years ago

Comment on attachment 8450410 [details] [diff] [review]
Move pseudo l10n to runtime

Stas, I'm happy with this patch, thanks!

	Staś Małolepszy :stas Assignee
	Comment 12 • 10 years ago

Thanks, Yuren.  I'm taking the bug then.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 14 • 10 years ago

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.

	Staś Małolepszy :stas Assignee
	Comment 15 • 10 years ago

I rebased the patch.  TBPL: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=86833e38fd1c

	Staś Małolepszy :stas Assignee
	Comment 16 • 10 years ago

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?

	Staś Małolepszy :stas Assignee
	Comment 17 • 10 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 18 • 10 years ago

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?

	Staś Małolepszy :stas Assignee
	Comment 19 • 10 years ago

(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.

	Staś Małolepszy :stas Assignee
	Comment 20 • 10 years ago

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.

	Staś Małolepszy :stas Assignee
	Comment 21 • 10 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 23 • 10 years ago

Comment on attachment 8455380 [details] [diff] [review]
All 3 parts as one patch

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

good work Stas!

	Staś Małolepszy :stas Assignee
	Comment 24 • 10 years ago

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.

	Staś Małolepszy :stas Assignee
	Comment 25 • 10 years ago

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

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode) Reporter
	Comment 26 • 10 years ago

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.

	Staś Małolepszy :stas Assignee
	Comment 27 • 10 years ago

Thanks, Yuren!  Landed on master:

Commit: https://github.com/mozilla-b2g/gaia/commit/e17a47524c55893bd7e3ebc9d600d6e7d072f8f5
Merged: https://github.com/mozilla-b2g/gaia/commit/728824fcbb94e9ffe36d58111e34e5065735b1c2

	Staś Małolepszy :stas Assignee
	Comment 28 • 10 years ago

L20n.js: https://github.com/l20n/l20n.js/commit/c2c7abdaf25644175f29433bdb33df869fa079ff

	Staś Małolepszy :stas Assignee
	Comment 29 • 10 years ago

I filed bug 1041565 to make pseudolocales toggleable in the Settings' Developer panel now that they are generated dynamically on runtime.