1000866 - Clean up bootstrap mozL10n API use in SMS

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

SMS 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.ready and mozL10n.once

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

My understanding is that the check for document.documentElement.lang is supposed to test whether or not the localized event was already fired or not.  I think mozL10n.once fits this use-case better.

Also interesting is the fact that there are iframes that need to be localized.  I didn't change anything there but it might be a good idea to find a universal solution to this problem in mozL10n.

Comment 2

[Julien Wajsberg [:julienw]]

Hey Stas, I didn't find time to review this until now, but now I've finished most of my blocker duties and since I'm away in the end of the week I commit myself to review this before tomorrow evening.

Comment 3

[Staś Małolepszy :stas]

Thanks, Julien.

From all the Gaia apps that I've audited over the last few days, this patch is actually one of the more complex.  (Usually it was just a matter of replacing the old-and-buggy mozL10n.ready with the new-and-predictable mozL10n.once).

It would be useful to add a test to the patch which makes sure document is hidden and un-hidden when needed.  I'll be happy to work on that tomorrow.  Should this be a Marionette test?

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8411782 [details] [diff] [review]
Use mozL10n.ready and mozL10n.once

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

Couldn't do a full review.

It seems to work fine on the device but I'll check if we can add some unit tests and report back later today.

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Staś Małolepszy :stas from comment #3)
> Thanks, Julien.
> 
> From all the Gaia apps that I've audited over the last few days, this patch
> is actually one of the more complex.  (Usually it was just a matter of
> replacing the old-and-buggy mozL10n.ready with the new-and-predictable
> mozL10n.once).
> 
> It would be useful to add a test to the patch which makes sure document is
> hidden and un-hidden when needed.  I'll be happy to work on that tomorrow. 
> Should this be a Marionette test?

I'm not sure to follow the "document is hidden/un-hidden when needed" part :)

This is not that easy to add unit tests because startup.js does not have any currently, and in activity-handler it's quite messy at the moment...

We can say that the existing unit tests cover these cases, with the mock changes that you made.

Integration tests could be nice but again I don't know what to check; I'm afraid that we'd need to test locale strings, which change overtime, which would make a maintenance hell IMO.

I'd say, let's land this as-is, unless you have a better idea than me.

Comment 6

[Staś Małolepszy :stas]

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #5)

> I'm not sure to follow the "document is hidden/un-hidden when needed" part :)

I got confused.  I initially thought this was similar to bug 999195 comment 10 where Gandalf wrote a patch to test if the 'hidden' class was properly set/removed from document.body.  However, this has nothing to do with document.hidden and the visibilitychange API that the SMS app uses!  My bad.

> I'd say, let's land this as-is, unless you have a better idea than me.

Sounds good to me, I'll merge this today.  Thanks for the review!

Comment 7

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/4da1d1dd9d8cfff8c2fefd9afbbb9f2e6465a113
Merged: https://github.com/mozilla-b2g/gaia/commit/7348a03d300946adb4d4747ac01be9cf941f61fc