Last Comment Bug 993188 - Clean up mozL10n.ready
: Clean up mozL10n.ready
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::L10n (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Staś Małolepszy :stas
:
:
Mentors:
https://github.com/mozilla-b2g/gaia/b...
Depends on:
Blocks: 993193 999779 999132 999193 1000593 1000599 1000866 1000895 1002625 1002772
  Show dependency treegraph
 
Reported: 2014-04-07 17:03 PDT by Zibi Braniecki [:gandalf][:zibi]
Modified: 2014-08-02 12:56 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
affected
affected
affected
fixed


Attachments
patch (1000 bytes, patch)
2014-04-09 13:03 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
patch, v2 (908 bytes, patch)
2014-04-10 00:35 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Diff | Splinter Review
Deterministic mozL10n.ready (4.77 KB, patch)
2014-04-23 05:54 PDT, Staś Małolepszy :stas
no flags Details | Diff | Splinter Review
Deterministic mozL10n.ready, without mozL10n.{add,remove}EventListener (6.93 KB, patch)
2014-04-24 03:19 PDT, Staś Małolepszy :stas
stas: review+
Details | Diff | Splinter Review
LazyL10n bustage fix (4.79 KB, patch)
2014-04-25 07:18 PDT, Staś Małolepszy :stas
gandalf: review+
Details | Diff | Splinter Review
Rebased and squashed patch, renamed _inDOM to _baseLoaded (9.66 KB, patch)
2014-04-28 06:56 PDT, Staś Małolepszy :stas
etienne: feedback+
Details | Diff | Splinter Review

Description User image Zibi Braniecki [:gandalf][:zibi] 2014-04-07 17:03:14 PDT
Current implementation of mozL10n.ready is internally inconsistent and may cause race conditions.

In particular, there are two behaviors of the function:

1) It may fire the function asynchronously
2) It may register a callback on 'localized' event for the callee

Which behavior will be chosen depends on if localization resources has been loaded at the time it is fired.

The current uses of mozL10n.ready in Firefox OS apps is always with an aim for one of the two, usually with additional code to prevent the other case. This is scary.

We will split this code into two separate API functions:

1) mozL10n.ready - which will fire if the resources are available and then every time the document is retranslated (and the resources for new language are available)

2) mozL10n.once - which will fire one, as soon as localization resources are available.
Comment 1 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-09 13:03:04 PDT
Created attachment 8404178 [details] [diff] [review]
patch

This patch clears mozL10n.ready and adds mozL10n.once.

Once we fix the Settings bug, we should be able to land this and start converting apps to use mozL10n.once.
Comment 2 User image Staś Małolepszy :stas 2014-04-09 16:28:31 PDT
(In reply to Zbigniew Braniecki [:gandalf] from comment #1)
> Created attachment 8404178 [details] [diff] [review]
> patch
> 
> This patch clears mozL10n.ready and adds mozL10n.once.
> 
> Once we fix the Settings bug, we should be able to land this and start
> converting apps to use mozL10n.once.

Hmm, I'm not sure in which order to fix this.  The fix to the Settings bug *is* to add mozL10n.once, so this part is common and I think will be taken of by bug 993189.

I'm also afraid of changing mozL10n without reviewing all other apps for their startup logic and workarounds.
Comment 3 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-10 00:35:57 PDT
Created attachment 8404499 [details] [diff] [review]
patch, v2

That makes sense. Add 'once' in that bug, and let's fix the behavior of ready here (since we may have to fix more use cases of ready before landing it).
Comment 4 User image Staś Małolepszy :stas 2014-04-11 03:31:03 PDT
Note to self:  we may want to use mozL10n.once in places like this:

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/test/unit/l10n/l10n_test.js#L77-L82

(How does this currently work?)
Comment 5 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-14 15:42:41 PDT
Bluetooth: 996272
Comment 6 User image Staś Małolepszy :stas 2014-04-23 05:52:07 PDT
Comment on attachment 8404499 [details] [diff] [review]
patch, v2

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

Canceling the review request;  I'll attach a similar patch in a second.
Comment 7 User image Staś Małolepszy :stas 2014-04-23 05:54:44 PDT
Created attachment 8410982 [details] [diff] [review]
Deterministic mozL10n.ready

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

This patch adds a deterministic mozL10n.ready method together with mozL10n.addEventListener and mozL10n.removeEventListener for edge cases when you want to have a fine-grained control over the exact moment of execution (e.g. only *after* the language changes).

It also migrates LazyL10n over to mozL10n.once.
Comment 8 User image Staś Małolepszy :stas 2014-04-23 06:07:05 PDT
I believe we should land this patch first and then take care of individual apps and fix their startup logic.

Right now we're still using the non-deterministic version of mozL10n.ready and it's just plain confusing to explain how it works.  For instance, in bug 999163 comment 4 you described the behavior of the deterministic mozL10n.ready() which is not yet implemented.

If my pull request is green, I suggest we go ahead and merge it, fixing mozL10n.ready, and then deal with any fallout in follow-up bugs.

It's a bit of a chicken and egg problem, and the way to break the stalemate is to accept the tiny risk of regression that comes with this pull request and commit to fixing all other apps promptly.
Comment 9 User image Staś Małolepszy :stas 2014-04-23 08:28:33 PDT
LazyL10n tests are failing now https://travis-ci.org/mozilla-b2g/gaia/jobs/23589346#L2194 b/c they hardcode the 'localize' event.
Comment 10 User image Staś Małolepszy :stas 2014-04-23 09:32:07 PDT
I pushed an update to the pull request which should fix the failing tests.  It also simplifies LazyL10n a bit.
Comment 11 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-23 09:51:15 PDT
Comment on attachment 8410982 [details] [diff] [review]
Deterministic mozL10n.ready

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

::: shared/js/l10n.js
@@ +1173,5 @@
> +      return navigator.mozL10n.ctx.addEventListener(type, callback);
> +    },
> +    removeEventListener: function removeEventListener(type, callback) {
> +      return navigator.mozL10n.ctx.removeEventListener(type, callback);
> +    },

On a second thought - I'm not sure if I like it.

It seems to duplicated window.addEventListener with the only difference being that it's not available until navigator.mozL10n is set.

I know that in the future we want to move the event to document, but we'll have to switch all uses of localized anyway, so I'd rather file a bug and do this soon (early in the window), instead of exposing another API methods on mozL10n.
Comment 12 User image Staś Małolepszy :stas 2014-04-23 12:48:21 PDT
(In reply to Zbigniew Braniecki [:gandalf] from comment #11)

> I know that in the future we want to move the event to document, but we'll
> have to switch all uses of localized anyway, so I'd rather file a bug and do
> this soon (early in the window), instead of exposing another API methods on
> mozL10n.

I don't have a strong preference, but I want to make sure that we are fine with the consequences.

 - Do we want to recommend window.addEventListener('localized', callback) for the edge-cases?
 - Can we officially discourage accessing mozL10n.ctx.addEventListener which may be tempting for those edge-cases? mozL10n.ctx is a private API and developers should never use it.  Should we rename it to mozL10n._ctx to make this more clear?

Also, it feels a bit clunky for me to mix mozL10n.{ready,once} and window.addEventListener.  In general, you should always use mozL10n.{ready,once}.  The edge-cases where you'd be tempted to explicitly deal with listeners instead of just using mozL10n.ready, are when the ctx is already ready and you want to do something on the next language change (like in tests).  If that's not what you want, you *should* use mozL10n.ready because it avoids the risk of causing intermittent race conditions.  So my thinking is, if you want to use the listener manually, you're probably doing it when the context is ready.  Compare the following two snippets:

  navigator.mozL10n.ready(function() {
    window.addEventListener('localized', doSomethingOnLangChange);
  });

  navigator.mozL10n.ready(function() {
    navigator.mozL10n.addEventListener('ready', doSomethingOnLangChange);
  });

My personal preference is for the latter since it uses two methods of the same object.  If you feel this is just cosmetic and the former is just as good, let's remove mozL10n.{add,remove}EventListener.
Comment 13 User image Staś Małolepszy :stas 2014-04-23 13:44:54 PDT
(In reply to Staś Małolepszy :stas from comment #10)
> I pushed an update to the pull request which should fix the failing tests. 
> It also simplifies LazyL10n a bit.

The build is green: https://travis-ci.org/mozilla-b2g/gaia/builds/23605945
Comment 14 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-24 01:06:08 PDT
(In reply to Staś Małolepszy :stas from comment #12)
> My personal preference is for the latter since it uses two methods of the
> same object.  If you feel this is just cosmetic and the former is just as
> good, let's remove mozL10n.{add,remove}EventListener.

Why do we emit localized then at all? Should we stop?

I see a value in keeping an event that is available beyond l10n.js functioning, so either on window or document and indicates that the localization has been completed.

Right now it is window.onlocalized, I'd like it to be document.onlocalized, but as long as there's no additional value brought by mozL10n.on*, I'd be against exposing that.

mozL10n.ctx is definitely private and should not be used.

> So my thinking is, if you want to use the listener manually, you're probably doing it when the context is ready.

That's possible, but since we're talking about edge cases, I'd prefer to assume that I don't know them all.
And I see no benefit of mozL10n.listener's over listener on window/document.

What's more, we're reporting status of the document, not library (or context), so I'd like to report it on the document, similarly like you have document.ondomcontentloaded and if you want to check the state you use document.readyState. Here we have document.ondocumentlocalized and status at document.mozL10n.readyState.
Comment 15 User image Staś Małolepszy :stas 2014-04-24 01:31:36 PDT
(In reply to Zbigniew Braniecki [:gandalf] from comment #14)
> Why do we emit localized then at all? Should we stop?

I think that in the current form, it's confusing to have the event fire on the window element.  'Localized' is a property of the content, not the viewport.

Both window and mozL10n are bad for this;  I think we should try to stop using 'localized' for now, instead relying on mozL10n.ready and mozL10n.once exclusively.  For edge cases (currently, only tests), mozL10n.addEventListener would be available, but otherwise is discouraged.

Maybe we should move this into another bug, and land this one without mozL10n.{add,remove}EventListener?   We can fix that other bug after we clean up all apps, which will make possible migration from using window easier.
Comment 16 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-24 01:37:15 PDT
(In reply to Staś Małolepszy :stas from comment #15)
> (In reply to Zbigniew Braniecki [:gandalf] from comment #14)
> > Why do we emit localized then at all? Should we stop?
> 
> I think that in the current form, it's confusing to have the event fire on
> the window element.  'Localized' is a property of the content, not the
> viewport.

Yes, we agree here.

> Both window and mozL10n are bad for this;  I think we should try to stop
> using 'localized' for now, instead relying on mozL10n.ready and mozL10n.once
> exclusively. 

We still agree.

> For edge cases (currently, only tests),
> mozL10n.addEventListener would be available, but otherwise is discouraged.

And here I disagree. You intend to expose *another* event listener method on top of the existing (and discouraged) one.

> Maybe we should move this into another bug, and land this one without
> mozL10n.{add,remove}EventListener?   We can fix that other bug after we
> clean up all apps, which will make possible migration from using window
> easier.

... and we agree again.

I prefer to stick to the existing API (window.onlocalized) and file a separate bug to replace it with document.ondocumentlocalized (taken from L20n 1.0 API), or anything else that you consider tha ultimate public event :)
Comment 17 User image Staś Małolepszy :stas 2014-04-24 01:47:45 PDT
(In reply to Zbigniew Braniecki [:gandalf] from comment #16)

> I prefer to stick to the existing API (window.onlocalized) and file a
> separate bug to replace it with document.ondocumentlocalized (taken from
> L20n 1.0 API), or anything else that you consider tha ultimate public event
> :)

That sounds like a good way to move forward.  I updated the pull request and removed mozL10n.{add,remove}EventListener.  I'll file a new bug and wait for the build to go green before I merge.
Comment 18 User image Staś Małolepszy :stas 2014-04-24 03:12:04 PDT
(In reply to Staś Małolepszy :stas from comment #17)

> That sounds like a good way to move forward.  I updated the pull request and
> removed mozL10n.{add,remove}EventListener.  I'll file a new bug and wait for
> the build to go green before I merge.

Bug 1000806.
Comment 19 User image Staś Małolepszy :stas 2014-04-24 03:19:32 PDT
Created attachment 8411702 [details] [diff] [review]
Deterministic mozL10n.ready, without mozL10n.{add,remove}EventListener

Carry over r+ from the previous patch.  Still waiting to see green on Travis.

Gandalf, are bug 996272 and bug 996291 hard dependencies?  It looks like they don't rely on the broken mozL10n.ready behavior and that landing this bug first should be safe.
Comment 20 User image Staś Małolepszy :stas 2014-04-24 03:29:23 PDT
The build is green: https://travis-ci.org/mozilla-b2g/gaia/builds/23657930
Comment 21 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-24 03:47:13 PDT
(In reply to Staś Małolepszy :stas from comment #19)
> Gandalf, are bug 996272 and bug 996291 hard dependencies?  It looks like
> they don't rely on the broken mozL10n.ready behavior and that landing this
> bug first should be safe.

They are not. In comment 2 you indicated that you don't want to land this before we review all use cases in all apps, so I started reviewing and filing bugs. Now that we have bug 1000593, they're not needed here.
Comment 22 User image Staś Małolepszy :stas 2014-04-24 04:11:41 PDT
(In reply to Zbigniew Braniecki [:gandalf] from comment #21)

> They are not. In comment 2 you indicated that you don't want to land this
> before we review all use cases in all apps, so I started reviewing and
> filing bugs. Now that we have bug 1000593, they're not needed here.

In comment 8 I suggested going forward with the landing now.  Are you OK with this?
Comment 23 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-24 04:14:05 PDT
(In reply to Staś Małolepszy :stas from comment #22)
> (In reply to Zbigniew Braniecki [:gandalf] from comment #21)
> 
> > They are not. In comment 2 you indicated that you don't want to land this
> > before we review all use cases in all apps, so I started reviewing and
> > filing bugs. Now that we have bug 1000593, they're not needed here.
> 
> In comment 8 I suggested going forward with the landing now.  Are you OK
> with this?

yes pls :)
Comment 24 User image Staś Małolepszy :stas 2014-04-24 09:09:54 PDT
Just to make sure, I also pushed to try around 6 hours ago: https://tbpl.mozilla.org/?tree=Try&rev=120339d943e0 and I'm still waiting for the results.

I also have a draft of an email to dev-gaia that I'd like to send tonight after we land this.
Comment 25 User image Staś Małolepszy :stas 2014-04-25 01:46:40 PDT
Try is green: https://tbpl.mozilla.org/?tree=Try&rev=120339d943e0
Comment 27 User image Anthony Ricaud (:rik) 2014-04-25 04:36:45 PDT
Sorry I had to back this out because it is breaking the callscreen app:
TypeError: navigator.mozL10n is undefined" {file: "app://communications.gaiamobile.org/shared/js/lazy_l10n.js" line: 26}

https://github.com/mozilla-b2g/gaia/commit/c0442eca3c5cd2c8071b98889ff49e9eb7df9a20
Comment 28 User image Anthony Ricaud (:rik) 2014-04-25 04:38:41 PDT
I think our unit tests didn't break because you haven't changed the l10n mocks. When you do an API change, you should update the mock. We have several mocks for l10n (that's bad), maybe it's time to merge them?
Comment 29 User image Staś Małolepszy :stas 2014-04-25 07:18:52 PDT
Created attachment 8412610 [details] [diff] [review]
LazyL10n bustage fix

Pull request with the original patch and this bustage fix: https://github.com/mozilla-b2g/gaia/pull/18684.  Should I squash the commits before I land or is it better to leave them separated to reflect the history of the bug?

Travis build: https://travis-ci.org/mozilla-b2g/gaia/builds/23756382

The original patch broken LazyL10n by setting LazyL10n._inDOM too early.  When subsequent LazyL10n.get() calls were made, LazyL10n tried to take an early shortcut by making a reference to navigator.mozL10n without loading shared/js/l10n.js again.  However, if l10n.js hasn't finished loading by that time, navigator.mozL10n was undefined.

The bustage fix moves this._inDOM = true into the LazyLoader.load's callback.  I also refactored the tests because I realized that they weren't testing the right thing.  By using a mock mozL10n object for all the tests it was impossible to test this specific behavior when navigator.mozL10n might not be yet available.

Rik, thanks again for backing out the original broken patch.  I verified that Comms/Dialer and Callscreen work fine with this bustage fix.  Can you review this patch for me, please?
Comment 30 User image Staś Małolepszy :stas 2014-04-25 11:46:22 PDT
Comment on attachment 8412610 [details] [diff] [review]
LazyL10n bustage fix

Hi Etienne - I'm looking for someone who could take a quick look at the updated tests of LazyL10n.  Would you like to help me out?  Thanks.
Comment 31 User image Etienne Segonzac (:etienne) 2014-04-28 05:41:55 PDT
Comment on attachment 8412610 [details] [diff] [review]
LazyL10n bustage fix

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

Can you rebase the patch?
Looks weird not to set _inDOM to true in the same tick that we call loader.load but maybe I'm just not looking at it correctly because it needs a rebase.
Comment 32 User image Staś Małolepszy :stas 2014-04-28 06:56:21 PDT
Created attachment 8413749 [details] [diff] [review]
Rebased and squashed patch, renamed _inDOM to _baseLoaded

Thanks, Etienne, for taking a look.  Here's the complete patch, rebased and commits squashed.

You're right - it looks weird because the name of the property is no longer appropriate.  I renamed _inDOM to _baseLoaded and _loaded to _allLoaded.

By setting _baseLoaded to true in loader.load's callback I make sure subsequent LazyL10n.get() calls don't assume navigator.mozL10n is available while in actuality l10n.js is still being fetched.
Comment 33 User image Etienne Segonzac (:etienne) 2014-04-29 02:13:02 PDT
Comment on attachment 8413749 [details] [diff] [review]
Rebased and squashed patch, renamed _inDOM to _baseLoaded

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

Thanks, lgtm.

::: shared/js/lazy_l10n.js
@@ +11,5 @@
>    var loader = LazyLoader;
>  
>    window.LazyL10n = {
> +    _baseLoaded: false,
> +    _allLoaded: false,

nit: |_ready|

@@ +19,5 @@
>          callback(navigator.mozL10n.get);
>          return;
>        }
>  
> +      var loadDate = this._loadDate.bind(this, callback);

nit: |loadDateAndFinalize|

@@ +28,5 @@
>        }
>  
>        // Add the l10n JS files to the DOM and wait for them to load.
> +      loader.load(['/shared/js/l10n.js'], function baseLoaded() {
> +        if (!this._baseLoaded) {

nit: we don't really need the if
Comment 34 User image Staś Małolepszy :stas 2014-04-29 05:06:35 PDT
Thank you, Etienne!  I addressed your feedback comments and landed.

Commit: https://github.com/mozilla-b2g/gaia/commit/66a87d7fe434d7401401ac09c29cb064bb2510ea
Merged: https://github.com/mozilla-b2g/gaia/commit/1892ba3a857f7e9cd1d2a0cf1c87481f3dcaca2c

Note You need to log in before you can comment on or make changes to this bug.