1204152 - Migrate FTU to use l20n.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Sam Foster [:sfoster] (he/him)]

There's a number of interesting possibilities opened up by using l20n api. We should migrate the FTU app to use it. 

I'm thinking of FTU add-ons injecting their own string bundles.

Comment 1

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

Attachment: pull request

First try.

I didn't port any tests, but I migrated whole app. Three thoughts after working on that:

1) Do we want to temporarily make navigator.mozL10n a proxy? That would make porting shared stuff easier and later we could add deprecation warnings.

2) We need QPS for FTU. Bug 1204067

3) We need to figure out how to load many entities at once. See bug 1204086.

Stas, what do you think?

Comment 2

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> Created attachment 8660221 [details] [review]
> pull request
> 
> First try.
> 
> I didn't port any tests, but I migrated whole app. Three thoughts after
> working on that:
> 
> 1) Do we want to temporarily make navigator.mozL10n a proxy? That would make
> porting shared stuff easier and later we could add deprecation warnings.

Sounds like a good idea.  How about we do it in a separate bug?  It would help me with bug 1203108, too.

> 2) We need QPS for FTU. Bug 1204067
> 
> 3) We need to figure out how to load many entities at once. See bug 1204086.
> 
> Stas, what do you think?

I commented in both of the above bugs.

Comment 3

[Staś Małolepszy :stas]

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

This looks good, but let's wait for resolutions in other bugs.

Comment 4

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

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

I think FTU is ready for the transition. The changes are really minimal as we keep the shared/pages/* using l10n.js for now.

There are two non-trivial changes:

1) I'm removing the code that makes FTU set the hour12 based on the timezone, since now we use automatic select until use manually selects an options (bug 1187539).

2) I'm migrating region UI selection to a more sophisticated algorithm. The new one creates empty DOMFragment, localizes it then sorts it using Intl.Collator.

The performance looks really good!

I tested on Flame-kk, master from yesterday:

master:
| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| l10nready             | 1550.355 | 1551   | 1332   | 1717   | 87.669  | 1694.300 |
| initialize            | 2460.774 | 2453   | 2283   | 2638   | 90.361  | 2628.750 |
| navigationLoaded      | 2568.161 | 2565   | 2388   | 2746   | 91.574  | 2739.850 |
| navigationInteractive | 3574.097 | 3565   | 3293   | 3783   | 118.674 | 3760.850 |
| contentInteractive    | 3574.516 | 3565   | 3293   | 3784   | 118.810 | 3761.800 |
| visuallyLoaded        | 3686.613 | 3696   | 3507   | 3862   | 96.322  | 3860     |
| fullyLoaded           | 3687.129 | 3696   | 3508   | 3866   | 96.665  | 3862.900 |
| pss                   | 19.991   | 19.996 | 19.901 | 20.170 | 0.061   | 20.090   |
| rss                   | 36.256   | 36.262 | 36.176 | 36.402 | 0.053   | 36.363   |
| uss                   | 16.131   | 16.129 | 16.051 | 16.266 | 0.050   | 16.243   |

patch:
| Metric                | Mean     | Median | Min    | Max    | StdDev | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------ | -------- |
| l10nready             | 1392.226 | 1375   | 1182   | 1568   | 94.557 | 1554.400 |
| initialize            | 2438.968 | 2415   | 2282   | 2619   | 90.493 | 2579.600 |
| navigationLoaded      | 2544     | 2523   | 2388   | 2722   | 90.630 | 2693.950 |
| navigationInteractive | 3566.774 | 3557   | 3408   | 3743   | 90.116 | 3732.800 |
| contentInteractive    | 3566.968 | 3557   | 3408   | 3744   | 90.161 | 3732.800 |
| visuallyLoaded        | 3659.484 | 3626   | 3505   | 3848   | 93.700 | 3797.600 |
| fullyLoaded           | 3659.968 | 3626   | 3505   | 3848   | 93.901 | 3797.900 |
| uss                   | 16.218   | 16.211 | 16.156 | 16.328 | 0.034  | 16.295   |
| pss                   | 20.090   | 20.078 | 20.027 | 20.378 | 0.065  | 20.200   |
| rss                   | 36.343   | 36.336 | 36.281 | 36.461 | 0.035  | 36.413   |

Comment 5

[Staś Małolepszy :stas]

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

The PR looks good but it's missing an upgrade path for pseudolanguages.  In bug 1210721 we removed l10n.qps and replaced it with l10n.pseudo whose methods are async.  Pseudolanguages had been lower priority in l20n because we carefully started the migration with apps which don't need them.

Let's plan how to upgrade the marionette tests in the FTU and let's also plan what to do with shared/js/languages_list.js during the transition to l20n.

Comment 6

[Sam Foster [:sfoster] (he/him)]

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

This has my tentative r+, but I'll re-check it when you re-submit the patch. I have to defer to your knowledge on what needs updating here. 

The getVersionInfo/init stuff is kinda fugly - not your fault. But, it looks functionally equivalent to what we have today - for better or worse (mostly worse :)

Comment 7

[Sam Foster [:sfoster] (he/him)]

Not blocking late customization anymore. Would still love to get this landed for 2.5, but its a lower priority.

Comment 8

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

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

Rerequesting feedback. I rebased the patch and moved all tests to use document.l10n.

Stas, can you review this? I don't think we will want to land it in 2.5, but I'd like to land it soon after branching if :sfoster will be ok with that.

One item that I'm not sure how to solve is https://github.com/mozilla-b2g/gaia/blob/60f40768667a2e2b92b5d1028e1cb4a9c54d691c/apps/ftu/test/marionette/lib/ftu.js#L36-L37 - do you have any recommendations on how to approach that in L20n API?

Thanks!

Comment 9

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

Perf results:

master:
| Metric                | Mean     | Median | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | ------ | ------ | ------ | ------ | --------- |
| l10nready             | 1533.226 | 1513   | 1460   | 1668   | 54.538 | 1552.425  |
| initialize            | 2378.484 | 2365   | 2293   | 2509   | 53.533 | 2397.329  |
| navigationLoaded      | 2504.065 | 2498   | 2416   | 2637   | 53.534 | 2522.910  |
| navigationInteractive | 3328.806 | 3332   | 3248   | 3479   | 56.933 | 3348.848  |
| contentInteractive    | 3329.032 | 3332   | 3248   | 3480   | 56.878 | 3349.055  |
| visuallyLoaded        | 3565.032 | 3558   | 3475   | 3697   | 54.372 | 3584.173  |
| fullyLoaded           | 3565.419 | 3558   | 3475   | 3697   | 54.371 | 3584.559  |
| uss                   | 16.252   | 16.215 | 16.141 | 16.668 | 0.134  | 16.299    |
| pss                   | 20.109   | 20.062 | 20.008 | 20.507 | 0.134  | 20.156    |
| rss                   | 36.457   | 36.418 | 36.344 | 36.871 | 0.133  | 36.504    |

l20n.js:
| Metric                | Mean     | Median | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | ------ | ------ | ------ | ------ | --------- |
| l10nready             | 1375.387 | 1379   | 1235   | 1452   | 39.728 | 1389.372  |
| initialize            | 2370.419 | 2364   | 2294   | 2463   | 37.681 | 2383.684  |
| navigationLoaded      | 2482.581 | 2481   | 2411   | 2566   | 37.146 | 2495.657  |
| navigationInteractive | 3311.806 | 3311   | 3225   | 3395   | 40.613 | 3326.103  |
| contentInteractive    | 3312.484 | 3311   | 3225   | 3395   | 40.601 | 3326.777  |
| visuallyLoaded        | 3545.548 | 3544   | 3472   | 3633   | 37.903 | 3558.891  |
| fullyLoaded           | 3546.387 | 3545   | 3472   | 3634   | 37.828 | 3559.704  |
| uss                   | 16.464   | 16.328 | 16.285 | 17.281 | 0.326  | 16.578    |
| rss                   | 36.678   | 36.543 | 36.500 | 37.496 | 0.326  | 36.793    |
| pss                   | 20.323   | 20.189 | 20.141 | 21.137 | 0.326  | 20.438    |

Comment 10

[Staś Małolepszy :stas]

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

> One item that I'm not sure how to solve is
> https://github.com/mozilla-b2g/gaia/blob/
> 60f40768667a2e2b92b5d1028e1cb4a9c54d691c/apps/ftu/test/marionette/lib/ftu.
> js#L36-L37 - do you have any recommendations on how to approach that in L20n
> API?

I think executeAsyncScript and document.l10n.ready.then() should do the job:

http://mozilla-b2g.github.io/marionette-js-client/api-docs/classes/Marionette.Client.html#method_executeAsyncScript

Comment 11

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

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

Applied :stas feedback.

The tree and perf results look promising:

ftu.gaiamobile.org     base: mean  1: mean  1: delta  1: p-value
---------------------  ----------  -------  --------  ----------
l10nready                    1579     1376      -203      * 0.00
initialize                   2408     2312       -96      * 0.00
navigationLoaded             2537     2423      -114      * 0.00
navigationInteractive        3595     3478      -117      * 0.00
contentInteractive           3595     3478      -116      * 0.00
visuallyLoaded               3676     3565      -111      * 0.00
fullyLoaded                  3676     3566      -110      * 0.00
pss                        20.820   20.924     0.104        0.08
rss                        37.124   37.242     0.118      * 0.05
uss                        16.978   17.085     0.107        0.07

Sam, can you review this and decide if you want to try to land it for 2.5?

Comment 12

[Sam Foster [:sfoster] (he/him)]

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

I've gone through this and tested as much as I can and it looks good to me. I've poked around the date/time screen in particular and havent run into any issues. The question with these patches is always not so much if the changes are good, but if they are the right changes (and all the necessary changes) and I have to trust you and Stas have covered this. 

Do we need to give other modules a heads-up on those /shared/ changes? 
Otherwise, provided the tests pass (and it appears they do) I'm cool with landing this.

Comment 13

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

> Do we need to give other modules a heads-up on those /shared/ changes? 

We don't. We have proxy on l20n.js to l10n.js and reverse proxy from l10n.js to l20n.js. In the next cycle I hope to move all apps to l20n.js and remove the proxies, but for now we can rather smoothly migrate ./shared code as we go and it will work in both - l10n.js and l20n.js apps.

I also tested FTU on the device with multiple locales and it seems to work well. I think it's worth giving a shot. If we run into regressions we can back it out fairly easily as I don't expect any conflicting changes to land on top of this anytime soon.

Comment 14

[Staś Małolepszy :stas]

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

Great work, Zibi. r=me.  I'm happy to see a perf win!

Comment 15

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

All right! Let's give it a try :)

Thanks everyone!
https://github.com/mozilla-b2g/gaia/commit/a5ee8789bc08e74cee82f7109a63bc6baa9c8d67