Closed Bug 1000895 Opened 6 years ago Closed 5 years ago

Clean up bootstrap mozL10n API use in System

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(1 file, 1 obsolete file)

System 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.
Depends on: 993188
I rebased your WIP on top of master and I found a bug.

You switched statusbar.js away from 'localized' event to mozL10n.once.

But Statusbar.init depends on DOM being available - https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L162

'localized' is fired once DOM is translated (and available), while .once can be fired before (when l10n resources are available, but DOM is not yet ready).

It's an interesting case that may be more common than we think...
Good catch, thanks Gandalf.  I'll review the other init methods and guard with DOMContentLoaded where needed.
Assignee: nobody → stas
Status: NEW → ASSIGNED
Depends on: 1000806
(In reply to Staś Małolepszy :stas from comment #3)
> Good catch, thanks Gandalf.  I'll review the other init methods and guard
> with DOMContentLoaded where needed.

Looks like this won't be needed after all.  I investigated this question in bug 1000806.  As long as mozL10n.ready and .once are called in deferred scripts, the callback is guaranteed to have access to an interactive DOM.  This is the case in the System app.
I rebased my WIP branch and triggered test builds:

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

I'll work on a final patch tomorrow.
Pull Request: https://github.com/mozilla-b2g/gaia/pull/18680
Travis: https://travis-ci.org/mozilla-b2g/gaia/builds/26771734 (green)

I changed window.addEventListener('localized') to mozL10n.once or mozL10n.ready, depending on the context.  We'll want to move away from using the localized event in the future and mozL10n.ready and mozL10n.once are safer to call.  (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.)

mozL10n sets the document's lang and dir automatically, so I removed code that repeated this logic from the System app.

I also found that the window manager broadcasts its own messages, like here:

js/app_window_manager.js (line 250)
  this.broadcastMessage('localized');

I'm not sure how they are used by other apps.  Etienne, is this fine to leave as-is?
Attachment #8435212 - Flags: review?(etienne)
Attachment #8435212 - Flags: feedback?(gandalf)
Comment on attachment 8435212 [details] [diff] [review]
Use mozL10n.ready and mozL10n.once

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

omg. That looks awesome. I counted 8 cases where you switch from retaining the whole chunk of code in memory (and refire it on each retranslation), to freeing it right after use.

That feels like a sweet victory!
Attachment #8435212 - Flags: feedback?(gandalf) → feedback+
Comment on attachment 8435212 [details] [diff] [review]
Use mozL10n.ready and mozL10n.once

I'm pretty sure it's okay (and the patch looks good!), but Alive will know for sure.
Attachment #8435212 - Flags: review?(etienne) → review?(alive)
Comment on attachment 8435212 [details] [diff] [review]
Use mozL10n.ready and mozL10n.once

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

I am afraid you need more work to get all unit tests pass.
Attachment #8435212 - Flags: review?(alive) → review+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> Comment on attachment 8435212 [details] [diff] [review]
> Use mozL10n.ready and mozL10n.once
> 
> Review of attachment 8435212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am afraid you need more work to get all unit tests pass.

Just saw the travis is green so ignore me.
(In reply to Staś Małolepszy :stas from comment #6)
> Created attachment 8435212 [details] [diff] [review]
> Use mozL10n.ready and mozL10n.once
> 
> Pull Request: https://github.com/mozilla-b2g/gaia/pull/18680
> Travis: https://travis-ci.org/mozilla-b2g/gaia/builds/26771734 (green)
> 
> I changed window.addEventListener('localized') to mozL10n.once or
> mozL10n.ready, depending on the context.  We'll want to move away from using
> the localized event in the future and mozL10n.ready and mozL10n.once are
> safer to call.  (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.)
> 
> mozL10n sets the document's lang and dir automatically, so I removed code
> that repeated this logic from the System app.
> 
> I also found that the window manager broadcasts its own messages, like here:
> 
> js/app_window_manager.js (line 250)
>   this.broadcastMessage('localized');
> 
> I'm not sure how they are used by other apps.  Etienne, is this fine to
> leave as-is?

For now this is used to update the app name (in task manager) according the locale + app's manifest.
Thanks for the review and the explanations, Alive!

Commit: https://github.com/mozilla-b2g/gaia/commit/7aec2c80be8979b588f2a8b35656703d9573551a
Merged: https://github.com/mozilla-b2g/gaia/commit/ec49d12262cad25cc0c0bda80dcfeaa619f4d3ce
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Thanks for backing this out, Ryan.  I'm investigating the problem.  It looks like some unit tests call mock mozL10n.once(init) too early, when the beforeAll hooks haven't set up the DOM for testing (which the init method needs).  I think the same problem existed before I landed my patch (hence bug 1016504), but it was less exposed, since most of the components would wait for the window's localized event instead of calling mozL10n.once directly.
Attached patch Bustage fixSplinter Review
Pull request:   https://github.com/mozilla-b2g/gaia/pull/20200
Travis (green): https://travis-ci.org/mozilla-b2g/gaia/builds/27091814
TBPL:           https://tbpl.mozilla.org/?tree=Gaia-Try&rev=addf5efe35c5

The reason why this broke the tests is the way the tests are written.

Most of the tests require the JS file they're supposed to test and call the init() method manually.  Previously, this worked well, because the l10n-related part of the initialization in the JS file looked like this:

  if (navigator.mozL10n.readyState == 'complete' ||
      navigator.mozL10n.readyState == 'interactive') {
    StatusBar.init();
  } else {
    window.addEventListener('localized', function statusbar_init() {
      window.removeEventListener('localized', statusbar_init);
      StatusBar.init();
    });
  }

MockL10n's readyState is undefined and the localized event was never fired, so the class or the instance remained uninitialized until init was explicitly called in the tests.

With my change from this bug, the above snippet becomes:

  navigator.mozL10n.once(StatusBar.init.bind(StatusBar));

Now this is problematic in tests because:

 a) some tests require the tested JS file before they assign navigator.mozL10n = MockL10n, i.e. when navigator.mozL10n is undefined (it's not a platform API but is provided by shared/js/l10n.js)

 b) even if mozL10n was already defined, in my first patch MockL10n.once used to just setTimeout(callback), so the init method was called at the time when the tested file was required;  this resulted in errors in cases when the init method needed access to DOM (like the one which made Ryan back out the original patch).

To prevent a), I now guard all mozL10n.once(init) calls with an if:

  if (navigator.mozL10n) {
    navigator.mozL10n.once(StatusBar.init.bind(StatusBar));
  }

To prevent b), I no-opped MockL10n.once, because the init methods are called manually in tests anyway, except one: the newsletter manager.  This is a short-term solution intended to fix the bustage.  We should find a better one in bug 1004973.  For instance, I find the following pattern from https://github.com/mozilla-b2g/gaia/blob/089d170f986d2916e3551935c74bf4c865f45cfd/apps/system/test/unit/activity_window_test.js quite elegant:

  setup(function(done) {
    stubById = this.sinon.stub(document, 'getElementById');
    stubById.returns(document.createElement('div'));
    // ...
    requireApp('system/js/activity_window.js', done);
  });

activity_window.js is required in a before[All] hook and it can safely access the DOM thanks to the stub.  I'd like other tests to use this pattern if Alive agrees, but changing them in this patch would be a big undertaking.  I'll file a follow-up for this.

Alive, can you review this patch for me, please?
Attachment #8435212 - Attachment is obsolete: true
Attachment #8436801 - Flags: review?(alive)
(In reply to Staś Małolepszy :stas from comment #15)
> activity_window.js is required in a before[All] hook and it can safely
> access the DOM thanks to the stub.  I'd like other tests to use this pattern
> if Alive agrees, but changing them in this patch would be a big undertaking.
> I'll file a follow-up for this.

Bug 1022558.
Comment on attachment 8436801 [details] [diff] [review]
Bustage fix

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

this works for me. thanks.
Attachment #8436801 - Flags: review?(alive) → review+
Thanks, Alive!

Commit: https://github.com/mozilla-b2g/gaia/commit/0340f5b330aba8691c17f436a5dca90b8a8ccae2
Merged: https://github.com/mozilla-b2g/gaia/commit/7429e9032f9d06286713059b05db33a630cb0a60
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.