Closed Bug 1000852 Opened 8 years ago Closed 8 years ago

Clean up bootstrap mozL10n API use in Video

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(1 file, 1 obsolete file)

Video currently works around the non-deterministic mozL10n.ready behavior.  Bug 993188 has the fix and bug 993189 introduced mozL10n.once, which is better-suited for initialization tasks.

Also, bug 914414 made the localization library take care of setting the lang and dir of the document, so we can remove that too.
Attached patch Use mozL10n.once (obsolete) — Splinter Review
https://github.com/mozilla-b2g/gaia/pull/18641
Attachment #8411757 - Flags: review?(dflanagan)
Comment on attachment 8411757 [details] [diff] [review]
Use mozL10n.once

Canceling the review request until I figure out bug 1000806.
Attachment #8411757 - Flags: review?(dflanagan)
Depends on: 1000806
Comment on attachment 8411757 [details] [diff] [review]
Use mozL10n.once

Review of attachment 8411757 [details] [diff] [review]:
-----------------------------------------------------------------

As concluded in bug 1000806, as long as mozL10n.ready and .once are called in deferred scripts, it is safe to assume that the callback will have access to an interactive DOM.  This is the case here.  Re-asking r?.
Attachment #8411757 - Flags: review?(dflanagan)
Comment on attachment 8411757 [details] [diff] [review]
Use mozL10n.once

Review of attachment 8411757 [details] [diff] [review]:
-----------------------------------------------------------------

r- because there is code that needs to be run more than once.

::: apps/video/js/video.js
@@ +120,3 @@
>  
>    if (!isPhone) {
>      // reload the thumbnail list title field for tablet which is the app name.

This code apparently needs to run any time the language changes, so putting it inside of a once() call is not right.  Maybe we need the once() call for init() and a localized event handler for the dynamic change.
Attachment #8411757 - Flags: review?(dflanagan) → review-
(In reply to David Flanagan [:djf] from comment #5)
> > ::: apps/video/js/video.js
> @@ +120,3 @@
> >  
> >    if (!isPhone) {
> >      // reload the thumbnail list title field for tablet which is the app name.
> 
> This code apparently needs to run any time the language changes, so putting
> it inside of a once() call is not right.  Maybe we need the once() call for
> init() and a localized event handler for the dynamic change.

Great catch, thanks.  I followed your suggestion and put init in its own mozL10n.once.  For the thumbnail list title on tablets, I reversed the order of blocks:  the if (!isPhone) is first, and a new mozL10n.ready with the logic responsible for relocalizing the title is called inside.  This way the listener is not registered at all on phones.

Travis build: https://travis-ci.org/mozilla-b2g/gaia/builds/26892379
Attachment #8411757 - Attachment is obsolete: true
Attachment #8435253 - Flags: review?(dflanagan)
Comment on attachment 8435253 [details] [diff] [review]
Use mozL10n.once and mozL10n.ready

Review of attachment 8435253 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Its a nice touch to only conditionally call the ready() func for the tablet case.
Attachment #8435253 - Flags: review?(dflanagan) → review+
You need to log in before you can comment on or make changes to this bug.