Closed
Bug 1204152
Opened 9 years ago
Closed 9 years ago
Migrate FTU to use l20n.js
Categories
(Firefox OS Graveyard :: Gaia::First Time Experience, defect)
Firefox OS Graveyard
Gaia::First Time Experience
ARM
Gonk (Firefox OS)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: sfoster, Assigned: zbraniecki)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Reporter | ||
Updated•9 years ago
|
Blocks: 2.5_LateCustomisation
Assignee | ||
Comment 1•9 years ago
|
||
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?
Attachment #8660221 -
Flags: feedback?(stas)
Comment 2•9 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1) > Created attachment 8660221 [details] > 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•9 years ago
|
||
Comment on attachment 8660221 [details]
pull request
This looks good, but let's wait for resolutions in other bugs.
Attachment #8660221 -
Flags: feedback?(stas) → feedback+
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8660221 [details] 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 |
Attachment #8660221 -
Flags: review?(stas)
Attachment #8660221 -
Flags: review?(sfoster)
Comment 5•9 years ago
|
||
Comment on attachment 8660221 [details] 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.
Attachment #8660221 -
Flags: review?(stas) → review-
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8660221 [details]
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 :)
Attachment #8660221 -
Flags: review?(sfoster)
Reporter | ||
Comment 7•9 years ago
|
||
Not blocking late customization anymore. Would still love to get this landed for 2.5, but its a lower priority.
No longer blocks: 2.5_LateCustomisation
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8660221 [details] 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!
Attachment #8660221 -
Flags: review- → review?(stas)
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
||
(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
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8660221 [details]
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?
Attachment #8660221 -
Flags: review?(sfoster)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8660221 [details]
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.
Attachment #8660221 -
Flags: review?(sfoster) → review+
Assignee | ||
Comment 13•9 years ago
|
||
> 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•9 years ago
|
||
Comment on attachment 8660221 [details]
pull request
Great work, Zibi. r=me. I'm happy to see a perf win!
Attachment #8660221 -
Flags: review?(stas) → review+
Assignee | ||
Comment 15•9 years ago
|
||
All right! Let's give it a try :) Thanks everyone! https://github.com/mozilla-b2g/gaia/commit/a5ee8789bc08e74cee82f7109a63bc6baa9c8d67
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•