The default bug view has changed. See this FAQ.

Status

Firefox OS
Gaia::L10n
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: stas)

Tracking

(Blocks: 2 bugs)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.3 affected, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 fixed)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
(Reporter)

Updated

3 years ago
Depends on: 914414
(Assignee)

Updated

3 years ago
Blocks: 993189
(Reporter)

Updated

3 years ago
Blocks: 993193
(Reporter)

Updated

3 years ago
Assignee: nobody → gandalf
(Reporter)

Comment 1

3 years ago
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.
Attachment #8404178 - Flags: review?(stas)
(Assignee)

Comment 2

3 years ago
(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.
(Reporter)

Comment 3

3 years ago
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).
Attachment #8404178 - Attachment is obsolete: true
Attachment #8404178 - Flags: review?(stas)
Attachment #8404499 - Flags: review?(stas)
(Reporter)

Updated

3 years ago
No longer blocks: 993189
Depends on: 993189
(Reporter)

Updated

3 years ago
See Also: → bug 882592
(Assignee)

Comment 4

3 years ago
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?)
(Reporter)

Comment 5

3 years ago
Bluetooth: 996272
Depends on: 996272
(Reporter)

Updated

3 years ago
Depends on: 996291
(Reporter)

Updated

3 years ago
Blocks: 999132
(Reporter)

Updated

3 years ago
Depends on: 999138
(Reporter)

Updated

3 years ago
Depends on: 999155
(Reporter)

Updated

3 years ago
Depends on: 999163
(Reporter)

Updated

3 years ago
Blocks: 999138
No longer depends on: 999138
(Reporter)

Updated

3 years ago
Blocks: 999193
(Reporter)

Updated

3 years ago
Depends on: 999195
(Assignee)

Updated

3 years ago
Blocks: 999779
(Assignee)

Comment 6

3 years ago
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.
Attachment #8404499 - Flags: review?(stas)
(Assignee)

Comment 7

3 years ago
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.
Attachment #8404499 - Attachment is obsolete: true
Attachment #8410982 - Flags: review?(gandalf)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

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

Comment 10

3 years ago
I pushed an update to the pull request which should fix the failing tests.  It also simplifies LazyL10n a bit.
(Reporter)

Updated

3 years ago
Attachment #8410982 - Flags: review?(gandalf) → review+
(Reporter)

Comment 11

3 years ago
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.
Attachment #8410982 - Flags: review+ → review?(gandalf)
(Assignee)

Comment 12

3 years ago
(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.
(Assignee)

Comment 13

3 years ago
(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
(Assignee)

Updated

3 years ago
Blocks: 1000593
(Assignee)

Updated

3 years ago
Blocks: 1000599
(Reporter)

Comment 14

3 years ago
(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.
(Assignee)

Comment 15

3 years ago
(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.
(Reporter)

Comment 16

3 years ago
(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 :)
(Assignee)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
(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.
(Assignee)

Comment 19

3 years ago
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.
Assignee: gandalf → stas
Attachment #8410982 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8410982 - Flags: review?(gandalf)
Attachment #8411702 - Flags: review+
(Assignee)

Comment 20

3 years ago
The build is green: https://travis-ci.org/mozilla-b2g/gaia/builds/23657930
(Reporter)

Comment 21

3 years ago
(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.
No longer depends on: 996272, 996291, 914414, 993189, 999155, 999163, 999195
(Assignee)

Comment 22

3 years ago
(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?
(Reporter)

Comment 23

3 years ago
(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 :)
(Assignee)

Updated

3 years ago
Blocks: 1000866
(Assignee)

Updated

3 years ago
Blocks: 1000895
(Assignee)

Comment 24

3 years ago
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.
(Assignee)

Comment 25

3 years ago
Try is green: https://tbpl.mozilla.org/?tree=Try&rev=120339d943e0
(Assignee)

Comment 26

3 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(Assignee)

Comment 29

3 years ago
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?
Attachment #8412610 - Flags: review?(anthony)
(Assignee)

Updated

3 years ago
Attachment #8412610 - Flags: review?(gandalf)
(Assignee)

Comment 30

3 years ago
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.
Attachment #8412610 - Flags: feedback?(etienne)
(Reporter)

Updated

3 years ago
Attachment #8412610 - Flags: review?(gandalf) → review+
(Reporter)

Updated

3 years ago
No longer blocks: 999138
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.
Attachment #8412610 - Flags: feedback?(etienne)
(Assignee)

Comment 32

3 years ago
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.
Attachment #8413749 - Flags: feedback?(etienne)
(Assignee)

Updated

3 years ago
Blocks: 1002625
(Assignee)

Updated

3 years ago
Blocks: 1002772
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
Attachment #8413749 - Flags: feedback?(etienne) → feedback+
(Assignee)

Comment 34

3 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Attachment #8412610 - Flags: review?(anthony)
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
You need to log in before you can comment on or make changes to this bug.