1002772 - Don't set document's lang and dir in Homescreen

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

Bug 914414 made the localization library take care of setting the lang and dir of the document, so we can remove that from the Homescreen app.

Comment 1

[Staś Małolepszy :stas]

Attachment: Remove land and dir setting

Pull request: https://github.com/mozilla-b2g/gaia/pull/18765

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8414057 [details] [diff] [review]
Remove land and dir setting

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

LGTM.

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8414057 [details] [diff] [review]
Remove land and dir setting

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

::: apps/homescreen/js/homescreen.js
@@ +8,3 @@
>    var iconGrid = document.getElementById('icongrid');
>  
> +  navigator.mozL10n.ready(GridManager.localize.bind(GridManager));

Alas, this doesn't work.  My bad, fortunately Travis caught it.

GridManager isn't defined when this line is run.  This used to work because in the old code, GridManager was referenced in the closure.  It was just lucky that mozL10n.ready takes a while to fire, and when it did, GridManager was already defined (at least most of the time).

I'll have a new patch in a few.

Comment 4

[Staś Małolepszy :stas]

Attachment: Fixed patch

In this patch, GridManager.localize() is called a bit later, in the callback to GridManager.init().  This ensures that GridManager is defined and all the required Icons have already been created.

Before mozL10n.ready() was fixed (bug 993188) it made slightly more sense to call it early on -- this way it registered a hadnler to the localized event instead of just invoking the callback and returning.  With the new behavior, the callback is guaranteed to be registered as a handler, and also will be invoked on the next turn of the event loop if the localization is ready.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8415342 [details] [diff] [review]
Fixed patch

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

r+ with one nit.

::: apps/homescreen/js/homescreen.js
@@ +32,4 @@
>      };
>  
>      GridManager.init(options, function gm_init() {
> +      navigator.mozL10n.ready(GridManager.localize.bind(GridManager));

s/GridManager/this/g

Comment 6

[Staś Małolepszy :stas]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #5)

> s/GridManager/this/g

The gm_init callback is called with the global object as 'this' [1] so I think using GridManager in that line is correct?

[1] https://github.com/mozilla-b2g/gaia/blob/800800b332d03580f4f8748e5046d71f00a3c7d9/apps/homescreen/js/grid.js#L1080

Comment 7

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #6)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #5)
> 
> > s/GridManager/this/g
> 
> The gm_init callback is called with the global object as 'this' [1] so I
> think using GridManager in that line is correct?
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> 800800b332d03580f4f8748e5046d71f00a3c7d9/apps/homescreen/js/grid.js#L1080

Vivien, could you explain the rationale behind the s/GridManager/this/g nit?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Staś Małolepszy :stas from comment #7)
> (In reply to Staś Małolepszy :stas from comment #6)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> > needinfo? please) from comment #5)
> > 
> > > s/GridManager/this/g
> > 
> > The gm_init callback is called with the global object as 'this' [1] so I
> > think using GridManager in that line is correct?
> > 
> > [1]
> > https://github.com/mozilla-b2g/gaia/blob/
> > 800800b332d03580f4f8748e5046d71f00a3c7d9/apps/homescreen/js/grid.js#L1080
> 
> Vivien, could you explain the rationale behind the s/GridManager/this/g nit?

Oh man. I thought we were into GridManager = {
 init: function() ...
}

Forgot about that.

Comment 9

[Staś Małolepszy :stas]

Cool, I just wanted to make sure.  I'll land the patch as-is.  Thanks!

Comment 10

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/481fadfc474ba8a0987cd6aeedcba1da7689d05d
Merged: https://github.com/mozilla-b2g/gaia/commit/a5ef08424b376bc9069a553406b48c393a3bf353