Closed Bug 1115796 Opened 5 years ago Closed 5 years ago

Update l10n.js to use Langpacks API

Categories

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

defect

Tracking

(b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S4 (23jan)
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

()

Details

Attachments

(3 files)

Once we have Gecko support for Langpacks, we need to update l10n.js to use the API for extending languages list and loading resources.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Depends on: 1108096
Priority: -- → P1
Target Milestone: --- → 2.2 S3 (9jan)
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.
We may consider working on bug 1115807 in order to simplify this patch.
Updated my branch to work on top of changes from bug 1115807
Depends on: 1115807
Attached file 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.
Attachment #8546904 - Flags: feedback?(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.
Attachment #8546904 - Flags: feedback?(stas) → feedback+
(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 on attachment 8546904 [details] [review]
pull request

Hey Stas,

Can you look at the patch again? I applied your feedback from the first round.
Attachment #8546904 - Flags: feedback+ → feedback?(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.
Attachment #8546904 - Flags: feedback?(stas) → feedback+
(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?
Flags: needinfo?(stas)
Sounds like a good approach which will help keep the langpack logic specific to the bindings.
Flags: needinfo?(stas)
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.
Attachment #8546904 - Flags: review?(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.
Attachment #8546904 - Flags: review?(stas) → review+
Attached file 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.
Attachment #8552185 - Flags: review+
Attachment #8552185 - Flags: approval-gaia-v2.2?(bbajaj)
Depends on: 1124141
Wondering why the bug is still open but you landed in master ?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8552185 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
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.
Flags: needinfo?(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.
Flags: needinfo?(gandalf)
Thanks; yeah it could be only noise... :)
Attached patch debug-l10n.diffSplinter Review
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 ?
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
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.
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?
NI me, I'll file a bug
Flags: needinfo?(felash)
Depends on: 1128505
Filed bug 1128505
Flags: needinfo?(felash)
Flags: needinfo?(fabrice)
You need to log in before you can comment on or make changes to this bug.