1229221 - Migrate Settings to l20n.js

Status: RESOLVED FIXED

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

Description

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

Settings is ready to be migrated and we should do this now.

I have a preliminary patch and promising perf test results.

Comment 1

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

Attachment: pull request

Comment 2

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

Here are results from flame-kk from today with RUNS=31:

master:
| Metric                | Mean     | Median   | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | -------- | ------ | ------ | ------ | --------- |
| contentInteractive    | 910.967  | 907      | 872    | 975    | 19.309 | 917.876   |
| navigationLoaded      | 1061.733 | 1056     | 978    | 1117   | 27.645 | 1071.626  |
| navigationInteractive | 1062     | 1056.500 | 978    | 1118   | 27.656 | 1071.897  |
| startupPathEnd        | 1732.933 | 1725     | 1674   | 1821   | 35.132 | 1745.505  |
| visuallyLoaded        | 2235     | 2236.500 | 2197   | 2290   | 20.986 | 2242.510  |
| fullyLoaded           | 4833.233 | 4831.500 | 4778   | 4930   | 39.186 | 4847.256  |
| uss                   | 18.321   | 18.338   | 18.090 | 18.520 | 0.092  | 18.354    |
| pss                   | 22.468   | 22.492   | 22.239 | 22.656 | 0.097  | 22.503    |
| rss                   | 37.884   | 37.908   | 37.652 | 38.082 | 0.097  | 37.919    |

l20n:
| Metric                | Mean     | Median  | Min    | Max    | StdDev | 95% Bound |
| --------------------- | -------- | ------- | ------ | ------ | ------ | --------- |
| contentInteractive    | 918.167  | 920     | 873    | 950    | 19.760 | 925.238   |
| navigationLoaded      | 935.233  | 937.500 | 888    | 966    | 20.210 | 942.465   |
| navigationInteractive | 935.467  | 937.500 | 889    | 966    | 20.236 | 942.708   |
| startupPathEnd        | 1679.900 | 1671    | 1617   | 1758   | 32.387 | 1691.489  |
| visuallyLoaded        | 2245.133 | 2244    | 2175   | 2289   | 24.437 | 2253.878  |
| fullyLoaded           | 4778.633 | 4776    | 4710   | 4846   | 28.493 | 4788.829  |
| rss                   | 37.159   | 37.199  | 36.711 | 37.840 | 0.292  | 37.264    |
| pss                   | 21.529   | 21.562  | 21.065 | 22.216 | 0.290  | 21.633    |
| uss                   | 17.186   | 17.221  | 16.727 | 17.867 | 0.289  | 17.290    |


The USS 95% Bound is over 1mb down, navigation 95% bound is around 100ms faster.
The only concerning piece is the higher stdev on mem. I'm not sure where it's coming from, but my first naive guess would be that it's because we release more memory earlier and it somehow interpolates with the measurements.
Because we still think that mem measurements are bimodal, I don't think we should be worried about the stdev.

Comment 3

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

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

Stas, I still have some tests to update and clean up, but I think that the patch is mostly ready for review.

Would you take a moment to just skim through it for any obvious points of concern?

I'd also like to ask you for an opinion on ManifestHelper: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/manifest_helper.js#L15

That's the one last remaining place which I didn't update to document.l10n, mostly because I don't know how. The getter is used by three apps - tv_apps, System and Settings, and scanning through https://github.com/mozilla-b2g/gaia/search?p=1&q=ManifestHelper&type=Code&utf8=%E2%9C%93 makes me worried about trying to turn it into an async call :/
Do you see any other way?

Comment 4

[Staś Małolepszy :stas]

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

Looks good except for listening to DOMLocalized, which we don't emit anymore.  We only have DOMRetranslated right now, and document.l10n.ready.


Re. the ManifestHelper, I'm stuck myself.  The getter logic is particularly unfirendly to our all-async paradigm.  To work around it we'd need to move the pseudol10n logic into the client, which I'm reluctant to do.  Perhaps we should consult the owners of the apps in questions and ask if they'd support rewriting the helper to async.

Comment 5

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

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

Evelyn, just putting this on your radar - still need to fix some tests and resolve the ManifestHelper story.

Comment 6

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

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

Ok, I think this is ready for review.

Comment 7

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

Stas, I decided to remove locale overriding from ManifestHelper. I tried to write a POC of how it should work, but it's an extremely deep dependency tree and getting name/short_name/description async would require tons of files to be deeply affected as in many cases they just carry the name from manifest to their own API's (see BrowserConfigHelper, AppHelper etc.).
It seems to me that the best way forward would be to disable locale overriding and either file a bug to write a new approach, or give up on Name/Desc pseudo.

I actually start to think that this may be a viable option - name/desc of an App is not a big part of what pseudo is for and I believe we can live without those overrides.
Where we do need them, maybe we should transform right before displaying instead of trying to transform data pulled out of manifest?

Either way, the patch is really big and I'm a bit afraid of amount of maintenance it'll require to prevent bitrotting. Let me know if you think my plan makes sense.

Comment 8

[Staś Małolepszy :stas]

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

> It seems to me that the best way forward would be to disable locale
> overriding and either file a bug to write a new approach, or give up on
> Name/Desc pseudo.
> 
> I actually start to think that this may be a viable option - name/desc of an
> App is not a big part of what pseudo is for and I believe we can live
> without those overrides.
> Where we do need them, maybe we should transform right before displaying
> instead of trying to transform data pulled out of manifest?

I realize how hard this is using the old sync approach.  I'm not opposed to exploring new approaches.  How about we remove this logic here and file a new bug for researching options for bringing it back?  One option would be to move the pseudolocale logic to the very end of the calling stack:  just before it's displayed in the homescreen, status bar and the task manager (and possibly other places).  Another idea might be to consider a more declarative approach:  perhaps a data-l10n-force-pseudo attribute for markup which we know isn't coming from l20n but should be localized.  This could also be useful for emulating things like localized messages coming from the server (or the carrier), although I'm slightly worried about the potential abuse which would lead to hard to detect l12y bugs.

Comment 9

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

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

Hi Fred, Evelyn pointed out that you'll be the best reviewer for me here :)

I'd like to move Settings to l20n.js on master. It's mostly search/replace navigator.mozL10n to document.l10n with a few meaningful changes.

l10n.js is deprecated and we want to move all apps to l20n in 2.6 cycle.

Comment 10

[[e-mail]]

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

Looks good, thanks!

Comment 11

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

Thanks for the super-fast review Fred! :)

Commit: https://github.com/mozilla-b2g/gaia/commit/4f3a98d60360d65889b475396b734338ef8bca8a

Comment 12

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

raptor.mozilla.org for flame-kk reports 130ms win on navigationLoaded/navigationInteractive and 1mb win on USS!

Sweet :)