1115796 - Update l10n.js to use Langpacks API

Status: RESOLVED FIXED

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Once we have Gecko support for Langpacks, we need to update l10n.js to use the API for extending languages list and loading resources.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

The branch: https://github.com/zbraniecki/gaia/tree/1115796-add-langpacks-to-l10n

Requires Gecko changes from bug 1108096. Works with Settings app only for now.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

We may consider working on bug 1115807 in order to simplify this patch.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Updated my branch to work on top of changes from bug 1115807

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Stas,

I cleaned up the patch and would like to add tests.

Can you take a look at it and let me know if you like the approach? I'll remove the language list bits before asking for review of course.

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8546904 [details] [review]
pull request

Thanks, Zibi.  I left a few comments on github.  My two biggest points are:

  1. I'm not a fan of changing the arg type of ctx.registerLocales.  Maybe pass the additional languages as a third optional argument?

  2. I wonder if there's a way to avoid hardcoding gaiaVersion in 2.2 and give us time to work on a solution in 2.3.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #5)
> Comment on attachment 8546904 [details] [review]
> pull request
> 
> Thanks, Zibi.  I left a few comments on github.  My two biggest points are:
> 
>   1. I'm not a fan of changing the arg type of ctx.registerLocales.  Maybe
> pass the additional languages as a third optional argument?

Let's discuss it. I have no strong opinion here.

>   2. I wonder if there's a way to avoid hardcoding gaiaVersion in 2.2 and
> give us time to work on a solution in 2.3.

I expect us to have to come up with a non-hardcoded solution. Would love a pure runtime one, but I don't want mozSettings involved. If we can't get anything, as a last resort I'm ok with the <meta> argument.

Thanks Stas! I'll work on some simple unit tests for that and will ask for review next.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8546904 [details] [review]
pull request

Hey Stas,

Can you look at the patch again? I applied your feedback from the first round.

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8546904 [details] [review]
pull request

Looks great, thanks Zibi!  I left comments in the pr.  I'm not sure if I like the change to type of the availableLangs argument to ctx.registerLocales.  I think this might break third-party code using the 2.x version of l10n.js and I'd like to be careful here.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #8)
> Comment on attachment 8546904 [details] [review]
> pull request
> 
> Looks great, thanks Zibi!  I left comments in the pr.  I'm not sure if I
> like the change to type of the availableLangs argument to
> ctx.registerLocales.  I think this might break third-party code using the
> 2.x version of l10n.js and I'd like to be careful here.

Hmm... How would you like to resolve that?

In order to negotiate between extraLangs and availableLangs I need to have the list of lang versions from meta. If I don't change the signature of ctx.registerLocales I need to resolve that in bindings which means no third parameter for ctx.registerLocales and passing already resolved array there.

Would you prefer that?

Comment 10

[Staś Małolepszy :stas]

Sounds like a good approach which will help keep the langpack logic specific to the bindings.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8546904 [details] [review]
pull request

Ok, updated my patch to your latest feedback.

I tried to write some bindings tests, but since we don't have anything for bindings, adding the first test is a lot of work and possibly require changes to build infra and to bindings code, because now mozL10n is included before the test is run so I can't really build for bootstrap :(

I think we should add having bindings test to 3.0 plan and land this as-is.

Let me know what you think.

Comment 12

[Staś Małolepszy :stas]

Comment on attachment 8546904 [details] [review]
pull request

r=me, thanks Zibi!  I left one more comment about extendLocales/registerLocales/initLocale in the PR.  I think there's value in putting the logic of extending locales into a separate function, especially for testing.

(In reply to Zibi Braniecki [:gandalf] from comment #11)

> I tried to write some bindings tests, but since we don't have anything for
> bindings, adding the first test is a lot of work and possibly require
> changes to build infra and to bindings code, because now mozL10n is included
> before the test is run so I can't really build for bootstrap :(
> 
> I think we should add having bindings test to 3.0 plan and land this as-is.

I agree we should plan an effort to set up an infrastructure for running bindings tests in 3.0.

In this patch, I'd love it if you could add tests for the logic of extending locales and storing the information about their sources.  I'd prioritize Gaia unit tests over L20n unit tests, i.e. I think it should be fine to expose extendLocales/registerLocales onto mozL10n._getInternalAPI and put tests in apps/sharedtest/test/unit/l10n/bindings/, and at the same time not make them runnable via l20n's 'grunt test' (for now).

As long as our 2.x branch is tightly coupled with Gaia, we don't risk landing a regression which would be caught by those tests.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/92bae78cdb176076119df692bf2b30d433eaab0e
Merge: https://github.com/mozilla-b2g/gaia/commit/1cff36cf7b2335d7958b5406ea8a33a0cc05b3cd

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Follow up patch: https://github.com/mozilla-b2g/gaia/commit/2ca9784fd7054d072a1d8b46fa8f7faaf6c106b7

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request for 2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): feature
[User impact] if declined: no language packs
[Testing completed]: On device
[Risk to taking this patch] (and alternatives if risky): It doesn't alter anything if there are no langpacks installed. Worst case scenario - langpacks don't work, but no regressions.
[String changes made]: No strings required.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

L20n: https://github.com/l20n/l20n.js/commit/1c6de559fd1bdc94cd10e4359b9db8e494ccef47

Comment 17

[Julien Wajsberg [:julienw]]

Wondering why the bug is still open but you landed in master ?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/2c576dbbe6b5126c427c29609d55de2f6f4d90cf

Comment 19

[Julien Wajsberg [:julienw]]

I see a perf regression on datazilla for this commit. While datazilla is not really precise, did/can you measure how much time this new code is taking? For example merely accessing the mozApps property could be costly if it's not preloaded.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> I see a perf regression on datazilla for this commit. While datazilla is not
> really precise, did/can you measure how much time this new code is taking?

Yeah, I was watching datazilla closely after landing and it did look like there's a regression, but because datazilla was down for most of yesterday I could not pinpoint it and verify it above the noise level.

I did test the changesets used by datazilla:
 - pre: 5e98dc164b17fd6d
 - post: 1c91c05b18af61f3

I tested on Flame, with default locale, with RUNS=100 on Settings app.

My results did not show any regression. I have the results and can link them here if you want.

I then proceeded to test non-default locales (where the same getAdditionalLanguages call happens before document state 'complete') and confirmed a regression within 40ms window, which I suspect is similar to what you observe in datazilla.

> For example merely accessing the mozApps property could be costly if it's
> not preloaded.

Maybe. I'm currently testing the performance of non-defualt and trying to pinpoint the regression. Once again - it shows up only in non-default locale. I cannot reproduce it on default-locale (which is what datazilla is tracking).

I'll report back today with results.

Comment 21

[Julien Wajsberg [:julienw]]

Thanks; yeah it could be only noise... :)

Comment 22

[Julien Wajsberg [:julienw]]

Attachment: debug-l10n.diff

So I measured how much time it is to merely access the mozApps property: between 30 and 40ms. Yes, synchronously. The problem is likely that we lazy load the API at the first access.

The actual call to getAdditionalLanguages is less than 4ms, so I consider it's ok.

Fabrice, do you think we should preload this API as well, like mozSettings ?

Comment 23

[[:fabrice] Fabrice Desré]

Julien, here's the performance I see with your patch:

Launching the settings app:
01-26 14:21:46.190 D/Settings( 2092): Content JS TIME: mozApps 
01-26 14:21:46.190 D/Settings( 2092):     at B (app://settings.gaiamobile.org/gaia_build_index.js:296:8719)
01-26 14:21:46.200 D/Settings( 2092): Content JS TIMEEND: mozApps 
01-26 14:21:46.200 D/Settings( 2092):     at B (app://settings.gaiamobile.org/gaia_build_index.js:296:8804)
01-26 14:21:46.200 D/Settings( 2092): Content JS TIME: getAdditionalLanguages 
01-26 14:21:46.200 D/Settings( 2092):     at B (app://settings.gaiamobile.org/gaia_build_index.js:296:8831)
01-26 14:21:46.200 D/Settings( 2092): Content JS TIMEEND: getAdditionalLanguages

Launching the clock app:
01-26 14:24:13.830 D/Clock   ( 2294): Content JS TIME: mozApps 
01-26 14:24:13.830 D/Clock   ( 2294):     at initResources (app://clock.gaiamobile.org/js/startup.js:2260:4)
01-26 14:24:13.830 D/Clock   ( 2294): Content JS TIMEEND: mozApps 
01-26 14:24:13.830 D/Clock   ( 2294):     at initResources (app://clock.gaiamobile.org/js/startup.js:2262:6)
01-26 14:24:13.830 D/Clock   ( 2294): Content JS TIME: getAdditionalLanguages 
01-26 14:24:13.830 D/Clock   ( 2294):     at initResources (app://clock.gaiamobile.org/js/startup.js:2263:6)
01-26 14:24:13.830 D/Clock   ( 2294): Content JS TIMEEND: getAdditionalLanguages

Launching the contacts app:
01-26 14:26:03.640 D/Communications( 2479): Content JS TIME: mozApps 
01-26 14:26:03.640 D/Communications( 2479):     at initResources (app://communications.gaiamobile.org/contacts/gaia_build_defer_index.js:188:0)
01-26 14:26:03.650 D/Communications( 2479): Content JS TIMEEND: mozApps 
01-26 14:26:03.650 D/Communications( 2479):     at initResources (app://communications.gaiamobile.org/contacts/gaia_build_defer_index.js:188:88)
01-26 14:26:03.650 D/Communications( 2479): Content JS TIME: getAdditionalLanguages 
01-26 14:26:03.650 D/Communications( 2479):     at initResources (app://communications.gaiamobile.org/contacts/gaia_build_defer_index.js:188:115)
01-26 14:26:03.650 D/Communications( 2479): Content JS TIMEEND: getAdditionalLanguages 

As you can see, worst case is 10ms to get mozApps, not the 30/40 ms you are seeing. That's on current m-c.

Comment 24

[Julien Wajsberg [:julienw]]

I think you can't trust the timestamp you have in the logcat, it's not reporting you the time when the log is generated, but "something else". console.time doesn't work outside of the devtools (bug 1087325).
I get my times by loading the app with WebIDE, where console.time/timeEnd work.

You can get ~ the same measurements using performance.now() and logging the diff directly from the app.

Note that I tried with the SMS app only, I can try with other apps if you want.

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

I tested SMS app with and without getAdditionalLanguages using make test-perf.

The result for moz-app-loaded and moz-app-visually-complete (and others) indicate that navigator.mozApps.getAdditionalLanguages costs ~40ms.

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hah, interesting. Seems like Julien is on track. Using [0] as a base I measured three stages:

a) no getAdditionalLanguages at all - finishBootstrap.call(this, meta);
b) if (navigator.mozApps) {finishBootstrap.call(this, meta);}
c) navigator.mozApps.getAdditionalLanguages().then(finishBootstrap.bind(this, meta));

Results for SMS app:

sms (means in ms)          (a)    (b)     (c) 
-------------------------  -----  -----  -----
moz-chrome-dom-loaded       950    981    996
moz-chrome-interactive      980   1010   1025
moz-app-visually-complete  1041   1073   1085
moz-content-interactive    1389   1409   1429
moz-app-loaded             1098   1127   1140

[0] https://github.com/mozilla-b2g/gaia/blob/d2c5db05c480c0c9c6009e16d25765783119d12a/shared/js/l10n.js#L1658-L1664

Does it sound a like a good argument to preload it?

Comment 27

[Julien Wajsberg [:julienw]]

NI me, I'll file a bug

Comment 28

[Julien Wajsberg [:julienw]]

Filed bug 1128505