993189 - Settings currently relies on the broken mozL10n.ready() behavior

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

Depending on when exactly it is called, mozL10n.ready() will either execute the callback once, or register it as a listener to the localized event.  See bug 993188 for details.

It just so happens that loading the 'root' panel usually happens when mozL10n is done loading the resources.  The callback to _navigate executes once and everything works as expected.

If, however, loading l10n resources takes more time, the callback to _navigate might fail to execute and will be instead registered as listener to the 'localized' event, thus executing every time the language changes.  In practice, this means navigating back to 'root' every time the language is changed on the fly.

When bug 993188 lands, we should move Settings to using mozL10n.once(), which is better-suited for its use-case.

Comment 1

[Staś Małolepszy :stas]

Attachment: Pull request: Add mozL10n.once()

The pull request adds the mozL10n.once() method and makes Settings use it as needed.

I already tested this patch with the patch from the depending bug 993193, and all works correctly.

Comment 2

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

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

sweet! f+

Comment 3

[Evelyn Hung]

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

clear my review flag since I have a concern on settings.js, I had left a comment on Github, please check it out. I didn't test this patch to testify if my concern is correct, but I'd like to see it's been addressed. 

Thanks for fixing this l10n issue on Settings. :)

Comment 4

[Staś Małolepszy :stas]

Evelyn - great catch, thanks.  I must have been thinking about bug 993188 when I wrote this code.  I'll revert the change for now and update the pull request.

Comment 5

[Staś Małolepszy :stas]

I updated the pull request and added a Marionette test to make sure we don't regress this in the future.  It's my first time writing Marionette tests, so please be gentle :) and let me know if I can structure it better.  I'm happy to learn more about this!

Comment 6

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

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

Evelyn: can you take a look at the updated patch?

Comment 7

[Evelyn Hung]

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

The patch looks good, but I'd like to add one more eye on test cases.

Thanks for the updated patch!

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

The patch is good, thanks! I have only one comment on the marionette tests. I would suggest to move the tests to a new file for the root panel.

Comment 9

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

Arthur: can we do this in a separate bug?

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Yes, let's remove the tests from the patch and open a new bug for them.

Comment 11

[Staś Małolepszy :stas]

I'd prefer to land with the test. I'll update the pull request later today and re-ask Arthur for a review.

Comment 12

[Staś Małolepszy :stas]

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

Arthur, can you take another look at the pull request, please?

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

r=me, thank you for the effort!

Comment 14

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/409b7816f8713364c38b0331bb716bd2cc0aa0b5
Merged: https://github.com/mozilla-b2g/gaia/commit/b066f8068d7cb69030a90977cf37ded4442947e3

Thank you for the reviews!

Comment 15

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

Landed for l20n.js@gaia - https://github.com/l20n/l20n.js/commit/2468ebd223606032293b3daccbea1910e03ad771