Closed Bug 993189 Opened 6 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(1 file)

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.
Depends on: 993188
Blocks: 993188
No longer depends on: 993188
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.
Assignee: nobody → stas
Status: NEW → ASSIGNED
Attachment #8404765 - Flags: review?(ehung)
Attachment #8404765 - Flags: feedback?(gandalf)
Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

sweet! f+
Attachment #8404765 - Flags: feedback?(gandalf) → feedback+
Attachment #8404765 - Flags: superreview?(21)
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. :)
Attachment #8404765 - Flags: review?(ehung)
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.
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 on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

Evelyn: can you take a look at the updated patch?
Attachment #8404765 - Flags: review?(ehung)
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!
Attachment #8404765 - Flags: review?(ehung) → review?(arthur.chen)
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.
Attachment #8404765 - Flags: review?(arthur.chen)
Arthur: can we do this in a separate bug?
Yes, let's remove the tests from the patch and open a new bug for them.
I'd prefer to land with the test. I'll update the pull request later today and re-ask Arthur for a review.
Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

Arthur, can you take another look at the pull request, please?
Attachment #8404765 - Flags: review?(arthur.chen)
Comment on attachment 8404765 [details] [review]
Pull request: Add mozL10n.once()

r=me, thank you for the effort!
Attachment #8404765 - Flags: review?(arthur.chen) → review+
No longer blocks: 993188
You need to log in before you can comment on or make changes to this bug.