Closed Bug 1203108 Opened 4 years ago Closed 4 years ago

Port SMS to l20n.js

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(1 file)

SMS uses l10n.js async API (setAttributes, translateFragment etc.) and as such is a good candidate for porting over to l20n.js.
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

This is still WIP because the test are failing.  Julien, do you have recommendations about how to stub promises in sinon?  The use-case here is that document.l10n.ready and document.l10n.languages are promises (not methods returning promises but actual promises) and I'm not sure how to stub their 'then' methods.
Attachment #8658714 - Flags: feedback?(felash)
\0/ Happy to see that l20n is coming! Btw, do we have any raptor comparison numbers for this PR?
Thanks, Oleg!  I ran raptor locally on my flame-kk ona clean profile with no messages.

Master:

[gaia] raptor test coldlaunch --app sms --output quiet --runs 30
| Metric                           | Mean     | Median   | Min      | Max      | StdDev | p95      |
| -------------------------------- | -------- | -------- | -------- | -------- | ------ | -------- |
| coldlaunch.navigationLoaded      | 1070.300 | 1052.500 | 981.000  | 1208.000 | 61.723 | 1176.000 |
| coldlaunch.willRenderThreads     | 1110.033 | 1098.500 | 1021.000 | 1248.000 | 62.056 | 1217.000 |
| coldlaunch.navigationInteractive | 1113.433 | 1103.500 | 1024.000 | 1250.000 | 61.809 | 1221.000 |
| coldlaunch.visuallyLoaded        | 1377.800 | 1373.000 | 1305.000 | 1498.000 | 40.449 | 1456.000 |
| coldlaunch.fullyLoaded           | 1450.067 | 1446.000 | 1371.000 | 1573.000 | 42.508 | 1525.000 |
| coldlaunch.pss                   | 19.900   | 19.800   | 19.300   | 20.900   | 0.386  | 20.900   |
| coldlaunch.rss                   | 35.163   | 35.100   | 34.600   | 36.000   | 0.308  | 36.000   |
| coldlaunch.uss                   | 15.830   | 15.800   | 15.300   | 16.900   | 0.383  | 16.900   |

L20n.js:

[gaia] raptor test coldlaunch --app sms --output quiet --runs 30
| Metric                           | Mean     | Median   | Min      | Max      | StdDev | p95      |
| -------------------------------- | -------- | -------- | -------- | -------- | ------ | -------- |
| coldlaunch.navigationLoaded      | 1064.200 | 1043.500 | 940.000  | 1233.000 | 63.690 | 1183.000 |
| coldlaunch.willRenderThreads     | 1104.400 | 1082.000 | 982.000  | 1274.000 | 63.508 | 1218.000 |
| coldlaunch.navigationInteractive | 1107.233 | 1087.500 | 984.000  | 1279.000 | 63.721 | 1221.000 |
| coldlaunch.visuallyLoaded        | 1382.667 | 1380.500 | 1310.000 | 1494.000 | 44.539 | 1472.000 |
| coldlaunch.fullyLoaded           | 1450.767 | 1449.500 | 1385.000 | 1560.000 | 43.921 | 1537.000 |
| coldlaunch.uss                   | 15.730   | 15.800   | 15.000   | 16.200   | 0.277  | 16.100   |
| coldlaunch.pss                   | 19.800   | 19.800   | 19.100   | 20.400   | 0.301  | 20.300   |
| coldlaunch.rss                   | 35.040   | 35.100   | 34.100   | 35.400   | 0.293  | 35.400   |

I'm still working on the pull request but I don't expect these number to change much.  However, we're working on a number of other initiatives which play nicely with l20n.js (e.g. bug 1191011) which might help improve the performance in the future.
Assignee: nobody → stas
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

We'll need to carefully review all uses of shared libraries, but otherwise the change is quite straightforward.

Is is a 2.5 goal for your team? Otherwise I think I'd prefer to defer to post-2.5 if that's possible.
Attachment #8658714 - Flags: feedback?(felash) → feedback+
(In reply to Julien Wajsberg [:julienw] (PTO Sept 23rd) from comment #5)
> Comment on attachment 8658714 [details] [review]
> [gaia] stasm:1203108-sms-l20n > mozilla-b2g:master
> 
> We'll need to carefully review all uses of shared libraries, but otherwise
> the change is quite straightforward.

Great feedback, thanks Julien!

> Is is a 2.5 goal for your team? Otherwise I think I'd prefer to defer to
> post-2.5 if that's possible.

It's not a goal but definitely a nice-to-have for us.  Let me work on this this week and fix all the tests and we can see how we feel about this.  But, if you prefer to wait until post-2.5, that's fine too.  The most important goal of porting apps to l20n.js is to help developers not write obsolete (i.e. mozL10n.get etc) code.  In case of SMS, I feel like you guys have been extremely cooperative and helpful so I'm not worried about it at all :)
I was thinking during my PTO about a possible migration point that could be less painful:

* l10n.js: expose the l10n object to document.l10n in addition to navigator.mozL10n.
=> makes it possible to update all code at once without changing the logic. Solves the necessity to check whether we have navigator.mozL10n or document.l10n.

* l20n.js: expose obsolete functions (like once()). Makes it possible to update apps without updating /shared libs (or the other way around :) ). So that we don't forget to remove them later on, make them call console.error with the new API.

What do you think ?
Flags: needinfo?(stas)
(In reply to Julien Wajsberg [:julienw] (PTO Sept 23rd) from comment #7)
> I was thinking during my PTO about a possible migration point that could be
> less painful:
> 
> * l10n.js: expose the l10n object to document.l10n in addition to
> navigator.mozL10n.
> => makes it possible to update all code at once without changing the logic.
> Solves the necessity to check whether we have navigator.mozL10n or
> document.l10n.

The way I see it, the trouble with the old API isn't navigator.mozL10n vs. document.l10n, but instead, it's the use of mozL10n.get().  As long as there are mozL10n.get()s in use, you can't switch to l20n.js.  Once you migrate to formatValue(s), the rest is mostly a matter of renaming.

Would exposing document.l10n in l10n.js help write shared code?  Or should we just keep using navigator.mozL10n there and migrate all of shared/ at a single point in time when all apps are using l20n?

> * l20n.js: expose obsolete functions (like once()). Makes it possible to
> update apps without updating /shared libs (or the other way around :) ). So
> that we don't forget to remove them later on, make them call console.error
> with the new API.

That's already taken care of in bug 1204660 :)  It exposes and shims obsolete mozL10n methods with an exception of the synchronous get().
Flags: needinfo?(stas)
Depends on: 1208369
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

Julien, I updated l20n.js to 3.1.0 (bug 1209370) which simplified the API and made this pr a bit easier.  I also realized that we don't need to stub document.l10n.ready anywhere b/c it's not needed anymore: all document.l10n methods internally wait for the right moment to perform their job.

I have a question about your preference regarding the asynchronous setup in apps/sms/views/shared/test/unit/localization_helper_test.js.  I can either make LocalizationHelper.init() return a promise and pass `done` as a callback, or schedule `done` in a next microtask with `Promise.resolve().then(done)`.  What's the best way to do this?
Attachment #8658714 - Flags: review?(felash)
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

I have few nits, I'd like to see the final working version before r+ing.

Can you put the changes in a separate commit ?

Also, your call whether we merge this before/after 2.5 branching. If we merge, we'll need your help in case we have performance issues and/or bugs with this.

I know that's already how you work :) but I prefer to state it explicitly in case anything goes wrong...
Attachment #8658714 - Flags: review?(felash)
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

I addressed your comments in a separate commit, as requested.  I'll rebase before merging.
Attachment #8658714 - Flags: review?(felash)
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

Please ping me when the tests are green with the updated mozL10n mock.
Attachment #8658714 - Flags: review?(felash)
Sorry for the noise.  It looks like I'll need to update the notification helper first; setting the dependency accordingly.
Depends on: 1214207
I did a new raptor run with the latest code and I don't see much difference compared to what you pasted in comment 4.
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

Julien, the build is green and I did some more perf testing to make sure there wouldn't be any regressions.  I think this is ready for your final review.

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=674f8815177e6de8854ec67c845e360967826bb6

I'll rebase the whole thing once again and squash the commits before landing.  I'm available of you have any questions and -- if we land this before the branching -- I'll make sure to keep an eye out on regressions.
Attachment #8658714 - Flags: review?(felash)
Comment on attachment 8658714 [details] [review]
[gaia] stasm:1203108-sms-l20n > mozilla-b2g:master

Please fix latest comment [1], squash, wait for a green build, and then r=me :)

Thanks for this great work !

[1] https://github.com/stasm/gaia/commit/674f8815177e6de8854ec67c845e360967826bb6#commitcomment-13930799
Attachment #8658714 - Flags: review?(felash) → review+
Thanks, Julien, for a thorough review!

Landed in master: https://github.com/mozilla-b2g/gaia/commit/41e13d5dd4788a69c28d2696cc60b2b7455e4369
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Of course, in theory, the performance change may be somehow related to bug 1211395... :) So it may be harder to evaluate the impact of this change alone.
Ahah yeah most of it is likely bug 1211395 :) We should have landed them at different days :p
Depends on: 1226197
You need to log in before you can comment on or make changes to this bug.