1000852 - Clean up bootstrap mozL10n API use in Video

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

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.

Comment 1

[Staś Małolepszy :stas]

Attachment: Use mozL10n.once

https://github.com/mozilla-b2g/gaia/pull/18641

Comment 2

[Staś Małolepszy :stas]

Comment on attachment 8411757 [details] [diff] [review]
Use mozL10n.once

Canceling the review request until I figure out bug 1000806.

Comment 3

[Staś Małolepszy :stas]

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?.

Comment 4

[Staś Małolepszy :stas]

Builds:

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=a07639912355
https://travis-ci.org/mozilla-b2g/gaia/builds/26756642

Comment 5

[David Flanagan [:djf]]

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.

Comment 6

[Staś Małolepszy :stas]

Attachment: Use mozL10n.once and mozL10n.ready

(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

Comment 7

[David Flanagan [:djf]]

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.

Comment 8

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/a9fe6b2b34070d75e453188c1a4e535d99c89ed8
Merged: https://github.com/mozilla-b2g/gaia/commit/3d5ee0d7b9c51f000fba077ae2829826d05e7b0c