993188 - Clean up mozL10n.ready

Status: RESOLVED FIXED

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: 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

[Staś Małolepszy :stas]

(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

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: 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

[Staś Małolepszy :stas]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

Bluetooth: 996272

Comment 6

[Staś Małolepszy :stas]

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

[Staś Małolepszy :stas]

Attachment: 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

[Staś Małolepszy :stas]

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

[Staś Małolepszy :stas]

LazyL10n tests are failing now https://travis-ci.org/mozilla-b2g/gaia/jobs/23589346#L2194 b/c they hardcode the 'localize' event.

Comment 10

[Staś Małolepszy :stas]

I pushed an update to the pull request which should fix the failing tests.  It also simplifies LazyL10n a bit.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

[Staś Małolepszy :stas]

(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

[Staś Małolepszy :stas]

(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

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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

[Staś Małolepszy :stas]

(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

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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

[Staś Małolepszy :stas]

(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

[Staś Małolepszy :stas]

(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

[Staś Małolepszy :stas]

Attachment: 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

[Staś Małolepszy :stas]

The build is green: https://travis-ci.org/mozilla-b2g/gaia/builds/23657930

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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

[Staś Małolepszy :stas]

(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

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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

[Staś Małolepszy :stas]

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

[Staś Małolepszy :stas]

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=120339d943e0

Comment 26

[Staś Małolepszy :stas]

Commit: https://github.com/mozilla-b2g/gaia/commit/a96aeea2798dd3242b80e83d3b61a6896c71de9e
Merged: https://github.com/mozilla-b2g/gaia/commit/c19be94c08fcdc86c71aacd22a48519aa4f03b7a

L20n.js: https://github.com/l20n/l20n.js/commit/9a404f1764215cd0a88c86ff37213f5f12e60f50

Comment 27

[Anthony Ricaud (:rik)]

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

[Anthony Ricaud (:rik)]

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

[Staś Małolepszy :stas]

Attachment: 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

[Staś Małolepszy :stas]

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

[Etienne Segonzac (:etienne)]

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

[Staś Małolepszy :stas]

Attachment: 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

[Etienne Segonzac (:etienne)]

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

[Staś Małolepszy :stas]

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