Define document.l10n through webidl for UI pages

RESOLVED FIXED in Firefox 64

Status

()

P2
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: mossop, Assigned: gandalf)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(5 attachments, 26 obsolete attachments)

46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
mossop
: review+
Details | Review
(Reporter)

Description

8 months ago
For those UI documents implemented by the browser (chrome, about), defining document.l10n through webidl allows us to implement localization support for untrusted content pages.
(Reporter)

Updated

8 months ago
Assignee: nobody → dtownsend
(Assignee)

Updated

8 months ago
Priority: -- → P2
Comment hidden (mozreview-request)
Depends on: 1455680
(Reporter)

Comment 2

8 months ago
Ok so this appears to work. We define document.l10n through webidl and the interface to that in DocumentL10n. I picked out the methods that looked like we want to expose them. A JS component is added that essentially replaces l10n.js, we initialise it when the document element is inserted and return it from document.l10n. It is responsible for finding the resources, creating DOMLocalization and ferrying API calls to it.

There are a few issues that still need to be worked out:

The change to XULDocument.cpp will be unnecessary once bug 1455680 lands.

mozDocumentL10n.js gets initialized early in the document load and just waits for DOMContentLoaded to init the DOMLocalization. That's probably later than we want. Options include initializing it from different places in the various content sinks (start of body for html for example), or making DOMLocalization capable of updating when new <link>'s are discovered and listening for DOMLinkAdded on the document.

DocumentSupportsL10n needs to be filled out, we currently attach to every document.

That's clearly wrong because some documents have no window and so a bunch of errors are logged right now.

It might be possible to just make DOMLocalization the thing we initialize and return from document.l10n, it would require some API changes though.
Comment hidden (mozreview-request)
Per IRC discussion, we'd like to move a way from [JSImplemention] in webidl. So concretely, that means:
(1) Making document.l10n a small C++-implemented shim.
(2) Adding the appropriate methods to mozIDocumentL10n.
(3) Having the shim instantiate a mozIDocumentL10n object in C++ and then forward the calls along.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

8 months ago
Comment on attachment 8969714 [details]
Bug 1455649: Implement document.l10n in webidl.

Ok here is my first pass. It's a bit rough and at the moment uses an intermediate JS component to forward calls on to DOMLocalization, I think that is easy enough to remove.

What do you think of this so far Bobby? My main pain points when implementing was getting the C++ even compiling with a stub (eventually turned out I needed to clobber for some reason) and I have to declare the functions that go all the way through to JS in both webidl and xpidl.
Attachment #8969714 - Flags: feedback?(bobbyholley)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8969714 [details]
Bug 1455649: Implement document.l10n in webidl.

https://reviewboard.mozilla.org/r/238512/#review245918

The wrapper bits look reasonable modulo the comments. Thanks!

::: intl/l10n/DocumentL10n.cpp:13
(Diff revision 3)
> +#include "mozilla/dom/DocumentL10nBinding.h"
> +#include "nsQueryObject.h"
> +#include "mozilla/dom/Promise.h"
> +#include "nsISupports.h"
> +
> +class mozL10nKey final : public mozIL10nKey

As mentioned below, I'm not sure this class is actually necessary.

::: intl/l10n/DocumentL10n.cpp:53
(Diff revision 3)
> +NS_IMPL_CYCLE_COLLECTION_CLASS(DocumentL10n)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DocumentL10n)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocument)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DocumentL10n)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocument)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(DocumentL10n)
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(DocumentL10n)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(DocumentL10n)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(DocumentL10n)
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END

Unless you're doing anything tricky here that I'm missing, I think there's a 1-liner macro to do all of this.

Also, don't you need to traverse mDOMLocalization as well? I don't entirely remember how this stuff works with XPCWrappedJS, but naively I'd think you'd need to.

::: intl/l10n/DocumentL10n.cpp:162
(Diff revision 3)
> +  for (auto& key : aKeys) {
> +    JS::RootedValue args(cx);
> +    if (key.mArgs) {
> +      args = JS::ObjectValue(*key.mArgs);
> +    } else {
> +      args = JS::UndefinedValue();
> +    }
> +
> +    nsCOMPtr<mozIL10nKey> jsKey = new mozL10nKey(cx, key.mId, args);
> +    keys.AppendElement(jsKey);
> +  }

Why do we need to round-trip through the mozIL10nKey stuff? Can't we just pass the l10n keys back to JS directly via ToJSVal?

::: intl/l10n/DocumentL10n.cpp:181
(Diff revision 3)
> +  RefPtr<Promise> promise = do_QueryObject(jsPromise);
> +  if (!promise) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }

This goop will all go away with bug 1455217, but it may not be worth blocking on it.

::: intl/l10n/mozDocumentL10n.js:14
(Diff revision 3)
> +  return Array.from(elem.querySelectorAll('link[rel="localization"]')).map(
> +    el => el.getAttribute("href")
> +  );
> +}
> +
> +function mozDocumentL10n() {

Seems like we might want to call the JS shim stuff mozDocumentL10nHelper or somesuch to distinguish it from the C++ class.

::: intl/l10n/mozDocumentL10n.js:36
(Diff revision 3)
> +    if (!this.localization) {
> +      throw Components.Exception("Not yet initialized.", Cr.NS_ERROR_NOT_INITIALIZED);
> +    }

Can't we assume it's been initialized? Same for the rest.
Attachment #8969714 - Flags: feedback?(bobbyholley) → feedback+
(Reporter)

Comment 9

8 months ago
(In reply to Bobby Holley (:bholley) from comment #8)
> Comment on attachment 8969714 [details]
> Bug 1455649: Add scaffolding for document.l10n implemented through webidl.
> 
> https://reviewboard.mozilla.org/r/238512/#review245918
> 
> The wrapper bits look reasonable modulo the comments. Thanks!
> 
> ::: intl/l10n/DocumentL10n.cpp:13
> (Diff revision 3)
> > +#include "mozilla/dom/DocumentL10nBinding.h"
> > +#include "nsQueryObject.h"
> > +#include "mozilla/dom/Promise.h"
> > +#include "nsISupports.h"
> > +
> > +class mozL10nKey final : public mozIL10nKey
> 
> As mentioned below, I'm not sure this class is actually necessary.
> 
> ::: intl/l10n/DocumentL10n.cpp:53
> (Diff revision 3)
> > +NS_IMPL_CYCLE_COLLECTION_CLASS(DocumentL10n)
> > +
> > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DocumentL10n)
> > +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocument)
> > +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
> > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> > +
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DocumentL10n)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocument)
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> > +
> > +NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(DocumentL10n)
> > +
> > +NS_IMPL_CYCLE_COLLECTING_ADDREF(DocumentL10n)
> > +NS_IMPL_CYCLE_COLLECTING_RELEASE(DocumentL10n)
> > +
> > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(DocumentL10n)
> > +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> > +NS_INTERFACE_MAP_END
> 
> Unless you're doing anything tricky here that I'm missing, I think there's a
> 1-liner macro to do all of this.

Yeah.

> Also, don't you need to traverse mDOMLocalization as well? I don't entirely
> remember how this stuff works with XPCWrappedJS, but naively I'd think you'd
> need to.

Maybe, because DOMLocalization is holding a reference to the DOM?

> ::: intl/l10n/DocumentL10n.cpp:162
> (Diff revision 3)
> > +  for (auto& key : aKeys) {
> > +    JS::RootedValue args(cx);
> > +    if (key.mArgs) {
> > +      args = JS::ObjectValue(*key.mArgs);
> > +    } else {
> > +      args = JS::UndefinedValue();
> > +    }
> > +
> > +    nsCOMPtr<mozIL10nKey> jsKey = new mozL10nKey(cx, key.mId, args);
> > +    keys.AppendElement(jsKey);
> > +  }
> 
> Why do we need to round-trip through the mozIL10nKey stuff? Can't we just
> pass the l10n keys back to JS directly via ToJSVal?

Oh right, yeah!

> ::: intl/l10n/mozDocumentL10n.js:14
> (Diff revision 3)
> > +  return Array.from(elem.querySelectorAll('link[rel="localization"]')).map(
> > +    el => el.getAttribute("href")
> > +  );
> > +}
> > +
> > +function mozDocumentL10n() {
> 
> Seems like we might want to call the JS shim stuff mozDocumentL10nHelper or
> somesuch to distinguish it from the C++ class.

Yeah, naming is a bit of a mess here.

> ::: intl/l10n/mozDocumentL10n.js:36
> (Diff revision 3)
> > +    if (!this.localization) {
> > +      throw Components.Exception("Not yet initialized.", Cr.NS_ERROR_NOT_INITIALIZED);
> > +    }
> 
> Can't we assume it's been initialized? Same for the rest.

We don't initialize until after DOMContentLoaded and scripts can run before then. I have a plan to fix that though.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1411707
(Assignee)

Comment 13

8 months ago
That looks great!

I'm reviewing the functionality now. There's a number of APIs that should be public and exposed on `document.l10n`:

 - formatMessages

 - getAttributes
 - connectRoot
 - disconnectRoot
 - translateRoots
 - pauseObserving
 - resumeObserving
 - translateFragment
 - translateElements
(Assignee)

Comment 14

8 months ago
I didn't run talos yet, but I did compile the code and launch Preferences with the profiler. There's a visible FOUC.

When analyzing the profile: https://perfht.ml/2JJ6b6Y

you can see that while bulk of work happens in reaction to MozBeforeInitialXULLayout around 1.6s, there is a `generateContext/ctxPromise<` call executed after 2.2s which I think is when the localization is applied.

I'm not sure why there's such a long empty space between when the initial localization happens and the final part.

Mossop - can you reproduce it?

STR:

1) Launch Firefox (en-US) with the patches
2) Open Preferences

Current result:

FOUC

Expected result:

All of the l10n completed in MozBeforeInitialXULLayout (because I/O is completed much earlier)


Another issue is that if you look at the first blue blip - addResource is called, and it triggers translateRoots. I think that addResource should not trigger translateRoots until MozBeforeInitialXULLayout happens, but we should figure out how and when to trigger `document.l10n.ctxs.touchNext` - which triggers I/O. The earlier we do this the earlier the I/O will complete.
(Assignee)

Comment 15

8 months ago
I couldn't get the talos test for about_preferences_basic to complete on try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa562af397b629d5f7e5b9c6cbc14022c44c28d4

Not sure why because they complete locally.
(Reporter)

Comment 16

8 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> I didn't run talos yet, but I did compile the code and launch Preferences
> with the profiler. There's a visible FOUC.
> 
> When analyzing the profile: https://perfht.ml/2JJ6b6Y
> 
> you can see that while bulk of work happens in reaction to
> MozBeforeInitialXULLayout around 1.6s, there is a
> `generateContext/ctxPromise<` call executed after 2.2s which I think is when
> the localization is applied.
> 
> I'm not sure why there's such a long empty space between when the initial
> localization happens and the final part.
> 
> Mossop - can you reproduce it?
> 
> STR:
> 
> 1) Launch Firefox (en-US) with the patches
> 2) Open Preferences
> 
> Current result:
> 
> FOUC
> 
> Expected result:
> 
> All of the l10n completed in MozBeforeInitialXULLayout (because I/O is
> completed much earlier)
> 
> 
> Another issue is that if you look at the first blue blip - addResource is
> called, and it triggers translateRoots. I think that addResource should not
> trigger translateRoots until MozBeforeInitialXULLayout happens

Until MozBeforeInitialXULLayout happens the roots should be empty shouldn't they? So translateRoots should be a no-op at that point.

> we should figure out how and when to trigger `document.l10n.ctxs.touchNext` -
> which triggers I/O. The earlier we do this the earlier the I/O will complete.

Ah yeah missed this part. So, for html it is straightforward, we could do it as soon as we hit the end of the <head> tag. That would be a reasonable sign that we're done discovering <link> elements. For XUL we don't have anything, unless we choose to also use <html:head> in XUL or some other useful container that can trigger an event at the end to hold localization links.

One alternative is using a deferred timer, waiting until we haven't seen a <link> in a few ms or something. That would require careful tweaking though.

Another alternative is using something other than <link> tags in XUL and instead something like a processing instruction. That takes us further from html so not sure how I feel about that.

Thoughts?
(Assignee)

Comment 17

8 months ago
> Another alternative is using something other than <link> tags in XUL and instead something like a processing instruction. That takes us further from html so not sure how I feel about that.
> 
> Thoughts?

Could we define a container for strings and specify that we wait for only one container before we kick off I/O?

Sth like:

```
<window>
  <l10n-resources>
    <link rel="localization" href="..."/>
    <link rel="localization" href="..."/>
  </l10n-resources>
</window>
```

We could then throw if we encounter the second container for now.
(Reporter)

Comment 18

8 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> > Another alternative is using something other than <link> tags in XUL and instead something like a processing instruction. That takes us further from html so not sure how I feel about that.
> > 
> > Thoughts?
> 
> Could we define a container for strings and specify that we wait for only
> one container before we kick off I/O?
> 
> Sth like:
> 
> ```
> <window>
>   <l10n-resources>
>     <link rel="localization" href="..."/>
>     <link rel="localization" href="..."/>
>   </l10n-resources>
> </window>
> ```
> 
> We could then throw if we encounter the second container for now.

We could, that's basically the same as using <html:head> just with a different name in XUL.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

7 months ago
Attachment #8971022 - Attachment is obsolete: true
(Reporter)

Updated

7 months ago
Attachment #8971717 - Flags: review?(gandalf)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

7 months ago
Attachment #8973862 - Flags: review?(gandalf)
Comment on attachment 8969714 [details]
Bug 1455649: Implement document.l10n in webidl.

https://reviewboard.mozilla.org/r/238512/#review248286

::: dom/base/nsDocument.cpp:3430
(Diff revision 6)
> +  rv = uri->SchemeIs("about", &isAbout);
> +  if (NS_FAILED(rv) || (!isAbout)) {
> +    return false;
> +  }

Note that this won't work for resource://payments/paymentRequest.xhtml?debug=1. Will we need to register an about: URI for it? I was trying to avoid it if possible since it just adds more indirection.

We would also like the ability to continue developing over HTTP so that we don't need browser restarts to iterate but it's not clear whether we could fallback to l10n.js in that case and have it still work by referencing .ftl from the HTTP pages.
(Assignee)

Comment 25

7 months ago
Soo... this somehow improves talos preferences performance by 20%!

 - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=84c81274c4520049f43731407f0f833ea0c8a436&newProject=try&newRevision=104655486084&framework=1

I don't fully understand how its possible since we don't remove any code, we do more work (FFI), and we kick off the same tasks at roughly same moments.

It's very cool to see an improvement, but keep in mind it may be an indication that this somehow fine tune for Talos better rather than for real life. I'll test this using hi-fps external camera today to verify.
(Reporter)

Comment 26

7 months ago
mozreview-review-reply
Comment on attachment 8969714 [details]
Bug 1455649: Implement document.l10n in webidl.

https://reviewboard.mozilla.org/r/238512/#review248286

> Note that this won't work for resource://payments/paymentRequest.xhtml?debug=1. Will we need to register an about: URI for it? I was trying to avoid it if possible since it just adds more indirection.
> 
> We would also like the ability to continue developing over HTTP so that we don't need browser restarts to iterate but it's not clear whether we could fallback to l10n.js in that case and have it still work by referencing .ftl from the HTTP pages.

I think we can support resource: too, you'll just have to register your substitution with the additional flag URI_DANGEROUS_TO_LOAD, I assume that is ok for your UI?

As for fallback, I imagine if the web l10n.js checks for document.l10n existence before defining its own it should work out ok. Gandalf may know better though.
(Reporter)

Comment 27

7 months ago
mozreview-review
Comment on attachment 8969714 [details]
Bug 1455649: Implement document.l10n in webidl.

https://reviewboard.mozilla.org/r/238512/#review248300

::: browser/components/preferences/connection.xul:29
(Diff revision 6)
> +    <html:link rel="localization" href="browser/preferences/connection.ftl"/>
> +  </linkset>
>  
>    <!-- Used for extension-controlled lockdown message -->
> -  <link rel="localization" href="browser/preferences/preferences.ftl"/>
> -  <link rel="localization" href="branding/brand.ftl"/>
> +  <html:link rel="localization" href="browser/preferences/preferences.ftl"/>
> +  <html:link rel="localization" href="branding/brand.ftl"/>

Fixed this locally.

::: dom/base/nsDocument.cpp:1538
(Diff revision 6)
>    mPreloadPictureFoundSource.SetIsVoid(true);
>    // For determining if this is a flash document which should be
>    // blocked based on its principal.
>    mPrincipalFlashClassifier = new PrincipalFlashClassifier();
> +
> +  mDocumentL10n = new DocumentL10n(this);

I'd really prefer not to do this here, but when we first have the principal for the document so we know the document supports it. Not sure when the principal is available though. This would allow skipping a lot of the principal checks too.

::: dom/base/nsDocument.cpp:3413
(Diff revision 6)
> +};
> +
> +void
> +nsIDocument::LoadDocumentL10n()
> +{
> +  nsContentUtils::AddScriptRunner(new LoadLocalizationsRunnable(this));

DocumentL10n should probably take care of this complication.

::: dom/base/nsDocument.cpp:3491
(Diff revision 6)
> +    return;
> +  }
> +
> +  nsString href;
> +  aLinkElement->GetAttr(kNameSpaceID_None, nsGkAtoms::href, href);
> +  nsContentUtils::AddScriptRunner(new HandleLocalizationId(this, href));

DocumentL10n should probably take care of this complication.

::: dom/webidl/Document.webidl:506
(Diff revision 6)
>  partial interface Document {
>    [Throws, Func="nsDocument::CallerIsTrustedAboutPage"] readonly attribute AboutCapabilities aboutCapabilities;
>  };
>  
> +partial interface Document {
> +  [Throws, Func="nsDocument::DocumentSupportsL10n"] readonly attribute DocumentL10n l10n;

This might not need to throw.

::: dom/xml/nsXMLContentSink.cpp:550
(Diff revision 6)
>          nodeInfo->NameAtom() == nsGkAtoms::textarea ||
>          nodeInfo->NameAtom() == nsGkAtoms::video ||
>          nodeInfo->NameAtom() == nsGkAtoms::audio ||
>          nodeInfo->NameAtom() == nsGkAtoms::object))
>        || nodeInfo->NameAtom() == nsGkAtoms::title
> +      || nodeInfo->Equals(nsGkAtoms::linkset, kNameSpaceID_XUL)

Not sure if this is needed.

::: intl/l10n/mozDocumentL10nHelper.js:42
(Diff revision 6)
> +    if (this.initialized) {
> +      this.localization.addResourceIds([resourceId]);
> +    } else {
> +      this.pendingResources.push(resourceId);
> +    }

Doing this in DocumentL10n would give us a perf win.
Comment hidden (mozreview-request)
(In reply to Dave Townsend [:mossop] from comment #26)
> Comment on attachment 8969714 [details]
> Bug 1455649: Implement document.l10n in webidl.
> 
> https://reviewboard.mozilla.org/r/238512/#review248286
> 
> > Note that this won't work for resource://payments/paymentRequest.xhtml?debug=1. Will we need to register an about: URI for it? I was trying to avoid it if possible since it just adds more indirection.
> > 
> > We would also like the ability to continue developing over HTTP so that we don't need browser restarts to iterate but it's not clear whether we could fallback to l10n.js in that case and have it still work by referencing .ftl from the HTTP pages.
> 
> I think we can support resource: too, you'll just have to register your
> substitution with the additional flag URI_DANGEROUS_TO_LOAD, I assume that
> is ok for your UI?

That's probably fine. Do you know off-hand how to do that from jar.mn? Or would we have to do it programmatically?

> As for fallback, I imagine if the web l10n.js checks for document.l10n
> existence before defining its own it should work out ok. Gandalf may know
> better though.

Yeah, I was also thinking I could fix l10n.js to detect if it's already defined. Looks like we're on the same page.
(Reporter)

Comment 30

7 months ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #29)
> (In reply to Dave Townsend [:mossop] from comment #26)
> > Comment on attachment 8969714 [details]
> > Bug 1455649: Implement document.l10n in webidl.
> > 
> > https://reviewboard.mozilla.org/r/238512/#review248286
> > 
> > > Note that this won't work for resource://payments/paymentRequest.xhtml?debug=1. Will we need to register an about: URI for it? I was trying to avoid it if possible since it just adds more indirection.
> > > 
> > > We would also like the ability to continue developing over HTTP so that we don't need browser restarts to iterate but it's not clear whether we could fallback to l10n.js in that case and have it still work by referencing .ftl from the HTTP pages.
> > 
> > I think we can support resource: too, you'll just have to register your
> > substitution with the additional flag URI_DANGEROUS_TO_LOAD, I assume that
> > is ok for your UI?
> 
> That's probably fine. Do you know off-hand how to do that from jar.mn? Or
> would we have to do it programmatically?

Programmatically you call setSubstitutionWithFlags. I donm't know if you can do that from jar.mn.
(Assignee)

Comment 31

7 months ago
Put a PR for Fluent changes in https://github.com/projectfluent/fluent.js/pull/195 - I'll file a bug to update FluentDOM once we land them and put it as a depedency on this one.
Blocks: 1460982
(Assignee)

Comment 33

7 months ago
Ok, another great update - here's the same but with the whole main browser menubar migrated to Fluent (120-130 messages) which gives us tpaint/ts_paint/sessionrestore numbers: https://pike.github.io/talos-compare/?revision=f7a7f51df5e9&revision=21ce2c05a50e&revision=a748ed2a0da7&revision=fbc6332bf924

tl;dr - zero measurable impact. This is awesome.
(Assignee)

Updated

7 months ago
Depends on: 1461048
(Assignee)

Comment 34

7 months ago
The good news is that I now have all the knowledge to test both via Talos and locally via screen recording and frame-by-frame.

The other good news is that I think I solved the mystery of this amazing performance boost.

The bad news is in the opt build from the talos run we don't really load Preferences at all. There's an error:

```
NS_ERROR_FAILURE: main.js:1160
buildContentProcessCountMenuList
chrome://browser/content/preferences/in-content/main.js:1160:7
init
chrome://browser/content/preferences/in-content/main.js:330:5
init
chrome://browser/content/preferences/in-content/preferences.js:43:7
init_category_if_required
chrome://browser/content/preferences/in-content/preferences.js:36:3
gotoPref
chrome://browser/content/preferences/in-content/preferences.js:160:5
init_all
chrome://browser/content/preferences/in-content/preferences.js:84:3
```

which comes from this line https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/main.js#1160

The problem is that my local build works, so I'm not sure why talos doesn't but it made me throw things and talos and measure for a couple days before I downloaded one of the builds from https://treeherder.mozilla.org/#/jobs?repo=try&revision=a748ed2a0da7ca6de0780bbae3d607de1bd9b091 and tried to run it. :(

Dave, any ideas?
Flags: needinfo?(dtownsend)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

7 months ago
Here are new numbers from a working talos - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f7a7f51df5e9&newProject=try&newRevision=565d17a7b30d753c81bb0b004aeb90ca3227f4d3&framework=1

Conclusion - no real impact on performance at this point. Will work on the patch now.
(Assignee)

Updated

7 months ago
Flags: needinfo?(dtownsend)
Comment on attachment 8969714 [details]
Bug 1455649: Implement document.l10n in webidl.

https://reviewboard.mozilla.org/r/238512/#review251132

::: intl/l10n/mozDocumentL10nHelper.js:24
(Diff revision 8)
> +
> +    if (!isLoading) {
> +      this.translateDocument();
> +    } else {
> +      let event = document.contentType === "application/vnd.mozilla.xul+xml" ?
> +                  "MozBeforeInitialXULLayout" : "readystate";

I was trying to test this with an XHTML document and noticed my document wasn't getting translated. I think "readystate" should be "readystatechange".
(In reply to Dave Townsend [:mossop] from comment #30)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #29)
> > (In reply to Dave Townsend [:mossop] from comment #26)
> > > Comment on attachment 8969714 [details]
> > > Bug 1455649: Implement document.l10n in webidl.
> > > 
> > > https://reviewboard.mozilla.org/r/238512/#review248286
> > > 
> > > > Note that this won't work for resource://payments/paymentRequest.xhtml?debug=1. Will we need to register an about: URI for it? I was trying to avoid it if possible since it just adds more indirection.
> > > > 
> > > > We would also like the ability to continue developing over HTTP so that we don't need browser restarts to iterate but it's not clear whether we could fallback to l10n.js in that case and have it still work by referencing .ftl from the HTTP pages.
> > > 
> > > I think we can support resource: too, you'll just have to register your
> > > substitution with the additional flag URI_DANGEROUS_TO_LOAD, I assume that
> > > is ok for your UI?
> > 
> > That's probably fine. Do you know off-hand how to do that from jar.mn? Or
> > would we have to do it programmatically?
> 
> Programmatically you call setSubstitutionWithFlags. I donm't know if you can
> do that from jar.mn.

Hmm… are you sure those substitution flags are the same as the channel flags? https://dxr.mozilla.org/mozilla-central/rev/54063deb2f1caf8c4acf6461d3ba779805835c96/netwerk/protocol/res/nsISubstitutingProtocolHandler.idl#20 makes me think not and it didn't seem to work for me (for my resource: page in the content process).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

7 months ago
Hi Dave,

I'm sorry for how long it took me - this is a non-trivial async code that spans across modules that I have not had a lot of experience with.

I think I'm pass the crux now. Here's the current 3-part patchset which implements the mozIDocumentLocalization and DocumentL10n APIs.

It does translate Preferences and I added some basic tests, although they don't yet verify that we resolve prior to firstPaint.
I also kept a lot of debugging printf/console.logs for you.

I'd like to request a feedback round on MozC++ and DOMC++ pattern uses and general logic and I hope that printfs will help you reason about what's the order of operations at startup and code triggers.

Once this is done, I'll clean up the printfs, add some inline docs, complete the API surface and request full review.
Flags: needinfo?(dtownsend)
(Assignee)

Comment 44

7 months ago
mozreview-review
Comment on attachment 8979775 [details]
Bug 1455649 - Implement DocumentL10n WebIDL.

https://reviewboard.mozilla.org/r/245922/#review251990

::: dom/base/nsDocument.cpp:3450
(Diff revision 1)
> +mozilla::Maybe<RefPtr<DocumentL10n>>
> +nsIDocument::GetLocalization(bool initialize)
> +{
> +  if (mDocumentL10n) {
> +    return Some(mDocumentL10n);
> +  } else if (initialize) {

I assume this should also guard against `PrincipleAllowsL10n`.

::: intl/l10n/DocumentL10n.cpp:109
(Diff revision 1)
> +  /*   nsContentUtils::AddScriptRunner( */
> +  /*       NewRunnableMethod<const nsAString&>("mozIDocumentLocalization::AddResourceId", */
> +  /*         mDocumentLocalization, */
> +  /*         &mozIDocumentLocalization::AddResourceId, */
> +  /*         aResourceId)); */
> +  /* } */

This doesn't seem to work. If I uncomment those lines it'll compile but won't fire the `mozIDocumentLocalization::AddResourceId`.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

7 months ago
Attachment #8969714 - Attachment is obsolete: true
Flags: needinfo?(dtownsend)
(Reporter)

Updated

7 months ago
Attachment #8971717 - Attachment is obsolete: true
Attachment #8971717 - Flags: review?(gandalf)
(Reporter)

Updated

7 months ago
Attachment #8973862 - Attachment is obsolete: true
Attachment #8973862 - Flags: review?(gandalf)
(Reporter)

Comment 50

7 months ago
mozreview-review
Comment on attachment 8979774 [details]
Bug 1455649 - Implement DocumentLocalization XPIDL API.

https://reviewboard.mozilla.org/r/245920/#review252294

::: intl/l10n/mozDocumentLocalization.js:20
(Diff revision 2)
> +
> +    this.localization = deferredL10nContainerParsed.promise.then(() => {
> +      const l10n = new DOMLocalization(this.document.defaultView, this.resourceIds);
> +      l10n.ctxs.touchNext(2);
> +      this.resolveLocalization = null;
> +      return l10n;

I think I would delete this.resourceIds at this point too.

::: intl/l10n/mozIDocumentLocalization.idl:12
(Diff revision 2)
> +
> +webidl Document;
> +webidl Element;
> +
> +[scriptable, uuid(7c468500-541f-4fe0-98c9-92a53b63ec8d)]
> +interface mozIDocumentLocalization : nsISupports

This needs a bunch of comments, also please clearly delineate the internal bits and the parts that are the DOMLocalization bits.

::: intl/l10n/mozIDocumentLocalization.idl:31
(Diff revision 2)
> +
> +  Promise formatMessages([array, size_is(aLength)] in jsval aKeys, in unsigned long aLength);
> +  Promise formatValues([array, size_is(aLength)] in jsval aKeys, in unsigned long aLength);
> +  Promise formatValue(in DOMString aId, [optional] in jsval aArgs);
> +
> +  Promise getReady();

Why not make this an attribute?
(Reporter)

Comment 51

7 months ago
mozreview-review
Comment on attachment 8979775 [details]
Bug 1455649 - Implement DocumentL10n WebIDL.

https://reviewboard.mozilla.org/r/245922/#review252304

::: dom/base/nsDocument.cpp:2238
(Diff revision 3)
> +  if (NS_FAILED(rv) || !uri) {
> +    return false;
> +  }
> +
> +  bool isAbout;
> +  rv = uri->SchemeIs("about", &isAbout);

I forgot to add that we want to support resource: here too.

::: dom/base/nsDocument.cpp:3436
(Diff revision 3)
> +    return;
> +  }
> +
> +  Element* head = GetHeadElement();
> +  if (parent != head && !parent->NodeInfo()->Equals(nsGkAtoms::linkset, kNameSpaceID_XUL)) {
> +    // TODO log a warning

I don't think we need a warning here, we'll have logged a warning for this link element previously.

I wonder what happens if you move a link element in the document and then remove it ....

::: dom/base/nsDocument.cpp:3450
(Diff revision 3)
> +mozilla::Maybe<RefPtr<DocumentL10n>>
> +nsIDocument::GetLocalization(bool initialize)
> +{
> +  if (mDocumentL10n) {
> +    return Some(mDocumentL10n);
> +  } else if (initialize && PrincipalAllowsL10n(NodePrincipal())) {

This means doing an additional principal check for every link element in every document that isn't allowed to access the API. I think we should avoid that. I think that's why I ended up trying to always define mDocumentL10n if the doc supports it.

::: intl/l10n/test/chrome.ini:25
(Diff revision 3)
> +[document_l10n/xul/test_lazy_resource_injection.xul]
> +[document_l10n/xul/test_no_loc.xul]
> +[document_l10n/xul/test_no_loc_api_calls.xul]
> +
> +[document_l10n/html/test_dom_loc.html]
> +[document_l10n/html/test_no_loc.html]

These all run in the main process I believe. We need tests for the content process behaviour.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 57

7 months ago
Comment on attachment 8979775 [details]
Bug 1455649 - Implement DocumentL10n WebIDL.

Boris - this is the core part of the new API which I'd like to get your high levle feedback on first.

It's unfortunately a fairly complicated API because it does a fair amount of back-and-forth between JS and C++.

On the JS side we have the core Fluent (localization library), L10nRegistry (our equivalent of ChromeRegistry for L10n which operates on async iterators of localization contexts) and most of FluentDOM (which adds operations on DOM).

On the C++ side we have some parts of FluentDOM which we moved there to avoid having to create reflections of nodes of JS (Node.localize), and now we're adding the Document level API which ties into nsIDocument - document.l10n.

I imagine that in the future we'll keep moving more of the FluentDOM to C++ (or Rust), but for now, we'd like to land this to unblock XBL removal work.

What this API is meant to do primarely is to allow for lazily initialization of DocumentL10n in result of one of two actions 
 - either a localization link is registered
 - or a caller asks for `document.l10n` from JS

Either of those triggers DocumentL10n and in result DocumentLocalization object initialization, and we additionally keep two flags that allow us to recognize which state we're in.

The result is that a user can call `document.l10n.formatValue` for example, at any point, but it'll only get resolved when we fetch the I/O for registered localization resources, or we reach the point where we know that resources were not added to the document (so we resolve the l10n contexts as empty).

That involves some C++/JS boilerplate and some deferred Promise logic.

I'd like to ask for your high level overview feedback on the API and direction on how to work with it further to get it into a reviewable state.

To test the API you can do one of the three things:
1) Launch Preferences
2) Launch the browser, open browser console and type "document.l10n"
3) Launch "./mach test ./intl/l10n/test/document_l10n/"

All three should work with the current code, although some of the code is commented out with questions, and there's some printf/console.log sprinkled around to help verify the operations executed and their order.

Let me know if you have any questions or if there's anything I can do to make the feedback round easier.
Attachment #8979775 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 58

7 months ago
Comment on attachment 8979774 [details]
Bug 1455649 - Implement DocumentLocalization XPIDL API.

Brian - similarly, as a future reviewer of this, I'd like to ask you to take a look at the JS parts (and C++ if you fancy) and provide me a high-level feedback on this approach :)
Attachment #8979774 - Flags: feedback?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8979775 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 62

7 months ago
Comment on attachment 8979774 [details]
Bug 1455649 - Implement DocumentLocalization XPIDL API.

Brian - I fixed the minimized testcase scenario you provided me, and added a test to verify that it solves it.
Attachment #8979774 - Flags: feedback?(bgrinstead)

Comment 63

7 months ago
mozreview-review
Comment on attachment 8979774 [details]
Bug 1455649 - Implement DocumentLocalization XPIDL API.

https://reviewboard.mozilla.org/r/245920/#review253760

::: intl/l10n/mozDocumentLocalization.js:18
(Diff revision 4)
> +    const deferredL10nContainerParsed = PromiseUtils.defer();
> +    const deferredDOMParsed = PromiseUtils.defer();
> +
> +    this._localization = deferredL10nContainerParsed.promise.then(() => {
> +      const l10n = new DOMLocalization(this._document.defaultView, Array.from(this._resourceIds));
> +      l10n.ctxs.touchNext(2);

What is this line doing, especially the arg '2'? Could use a comment

::: intl/l10n/mozDocumentLocalization.js:27
(Diff revision 4)
> +
> +    this._resolveLocalization = deferredL10nContainerParsed.resolve;
> +
> +    this.ready = deferredDOMParsed.promise.then(async () => {
> +      if (this._resourceIds.size > 0) {
> +        const l10n = await this._localization;

I know we have some complex state to manage here, and I haven't spent long enough to map it all out, but I think the initialization states here could be simplified.

For instance, my impression is that nulling out `_resolveLocalization` could lead to an error in onL10nResourceContainerParsed. If addResourceId can be called before  onL10nResourceContainerParsed happens (as I gather from the `if (this._resolveLocalization)` condition), then won't onL10nResourceContainerParsed throw in that case?

There's also a copy/pasted block inside the ready callback and in addResourceId that would be nice to avoid if possible.

I'm not sure what the right solution here is - I can take a closer look in another round. Maybe if we could wait to set up these promises until the init function when we know more about the document state it would be simpler?

::: intl/l10n/mozIDocumentLocalization.idl:14
(Diff revision 4)
> +webidl Element;
> +
> +/**
> + * An API for managing localization state of a single document using Fluent.
> + *
> + * The API is called by the DocumentL10n API and internally uses

This class seems to do two main things:

1) Manage initialization state
2) Construct and then send calls over to DOMLocalization

Curious if you've considered exposing DOMLocalization to C++ and then creating it from the DocumentL10n class instead of using XPCOM/JS for this? Is the issue that the initialization state management would be worse to deal with in C++, or is there another reason to have the new JS class here?

::: intl/l10n/mozIDocumentLocalization.idl:60
(Diff revision 4)
> +   * registered.
> +   */
> +  readonly attribute Promise ready;
> +
> +  /**
> +   * Below methods are exposing `DOMLocalization` API for the document.

Where are these methods actually exposed? I see setAttributes/getAttributes which are pretty much copied from DOMLocalization. But I don't see where translateFragment, translateElements, formatMessages, formatValues, or formatValue are implemented.

Further, addResourceId and removeResourceId don't seem to have a corresponding function in DOMLocalization - there they are called addResourceIds and removeResourceIds.
(Assignee)

Comment 64

7 months ago
mozreview-review-reply
Comment on attachment 8979774 [details]
Bug 1455649 - Implement DocumentLocalization XPIDL API.

https://reviewboard.mozilla.org/r/245920/#review253760

> What is this line doing, especially the arg '2'? Could use a comment

It is documented here: https://searchfox.org/mozilla-central/source/intl/l10n/Localization.jsm#73 - but yeah, I'll add more docs as I mature the code before review round.

The idea is that since we know we will need at least one locale, we eagerly trigger resolution of the first element of out of the cached iterator. Since at the moment we want to maximize the chance that we don't FOUC, we trigger for the locale and its fallback, which for now is always en-US.

Once this settles, we'll want to start memorizing which setups required how many fallbacks and fetch that number eagerly (see bug 1462839).

> I know we have some complex state to manage here, and I haven't spent long enough to map it all out, but I think the initialization states here could be simplified.
> 
> For instance, my impression is that nulling out `_resolveLocalization` could lead to an error in onL10nResourceContainerParsed. If addResourceId can be called before  onL10nResourceContainerParsed happens (as I gather from the `if (this._resolveLocalization)` condition), then won't onL10nResourceContainerParsed throw in that case?
> 
> There's also a copy/pasted block inside the ready callback and in addResourceId that would be nice to avoid if possible.
> 
> I'm not sure what the right solution here is - I can take a closer look in another round. Maybe if we could wait to set up these promises until the init function when we know more about the document state it would be simpler?

> If addResourceId can be called before  onL10nResourceContainerParsed happens (as I gather from the if (this._resolveLocalization) condition), then won't onL10nResourceContainerParsed throw in that case?

No. Basically, all `addResourceId` called before `onL10nResourceContainerParsed` are collected and send to `DocumentLocalization` when its initialized.
All later `addResourceId` are sent individually to already existing object.

> This class seems to do two main things:
> 
> 1) Manage initialization state
> 2) Construct and then send calls over to DOMLocalization
> 
> Curious if you've considered exposing DOMLocalization to C++ and then creating it from the DocumentL10n class instead of using XPCOM/JS for this? Is the issue that the initialization state management would be worse to deal with in C++, or is there another reason to have the new JS class here?

It does a bit more. `DocumentLocalization` also manages document's state and will in the next iteration of the patch do things like setting document's direction based on the resolved locale.

What I did consider is making `DocumentLocalization` extend `DOMLocalization`. The difference is that `DOMLocalization` uses the constructor to set its state, while `DocumentLocalization` has to use `init()`. I'm not sure which way to resolve it.

> Where are these methods actually exposed? I see setAttributes/getAttributes which are pretty much copied from DOMLocalization. But I don't see where translateFragment, translateElements, formatMessages, formatValues, or formatValue are implemented.
> 
> Further, addResourceId and removeResourceId don't seem to have a corresponding function in DOMLocalization - there they are called addResourceIds and removeResourceIds.

Hmm... `formatValue` and `formatValues` are defined on the class. The rest will be added in the next iteration since they will look exactly the same, unless we decide to make `DocumentLocalization` extend `DOMLocalization`. I wanted to try to keep the files smaller for now to make it easier to comperhand the flow and reason about the big picture.

It was also the minimal viable patch that makes Preferneces start without errors :)

Once I get feedback from you and Boris, I'll apply it and extend the patches to cover more.

Comment 65

7 months ago
mozreview-review
Comment on attachment 8979775 [details]
Bug 1455649 - Implement DocumentL10n WebIDL.

https://reviewboard.mozilla.org/r/245922/#review254526

::: dom/base/nsDocument.cpp:2240
(Diff revision 6)
> +    return false;
> +  }
> +
> +  bool isAbout;
> +  rv = uri->SchemeIs("about", &isAbout);
> +  if (NS_FAILED(rv) || (!isAbout)) {

This function could use a comment about what policy it's trying to implement.  What it implements is that only system things and some (but not all) about: URIs allow l10n.  Is that the intent?

::: dom/base/nsDocument.cpp:3200
(Diff revision 6)
>      }
>    }
>    mNodeInfoManager->SetDocumentPrincipal(aNewPrincipal);
>  
> +  if (aNewPrincipal && PrincipalAllowsL10n(aNewPrincipal)) {
> +    mPrincipalAllowsL10n = true;

Why bother caching this boolean?  So far it's used in only one place, and that place doesn't run very often....

::: dom/base/nsDocument.cpp:3401
(Diff revision 6)
>  }
>  
>  bool
> +nsDocument::DocumentSupportsL10n(JSContext* aCx, JSObject* aObject)
> +{
> +  return PrincipalAllowsL10n(nsContentUtils::SubjectPrincipal(aCx));

So this will expose the l10n api on all web pages to chrome script.  Is that the intent?

::: dom/base/nsDocument.cpp:3408
(Diff revision 6)
> +
> +void
> +nsIDocument::LocalizationLinkAdded(Element* aLinkElement)
> +{
> +  auto l10n = GetLocalization(true);
> +  if (!l10n) {

How can it be false?

::: dom/base/nsDocument.cpp:3418
(Diff revision 6)
> +  if (!parent) {
> +    return;
> +  }
> +
> +  Element* head = GetHeadElement();
> +  if (parent != head && !parent->NodeInfo()->Equals(nsGkAtoms::linkset, kNameSpaceID_XUL)) {

That's a really complicated way to write !parent->IsXULElement(nsGkAtoms::linkset)

::: dom/base/nsDocument.cpp:3431
(Diff revision 6)
> +}
> +
> +void
> +nsIDocument::LocalizationLinkRemoved(Element* aLinkElement)
> +{
> +  auto l10n = GetLocalization(true);

Why true here?

::: dom/base/nsDocument.cpp:3442
(Diff revision 6)
> +  if (!parent) {
> +    return;
> +  }
> +
> +  Element* head = GetHeadElement();
> +  if (parent != head && !parent->NodeInfo()->Equals(nsGkAtoms::linkset, kNameSpaceID_XUL)) {

Again, IsXULElement

::: dom/base/nsDocument.cpp:3448
(Diff revision 6)
> +    // TODO log a warning
> +    return;
> +  }
> +
> +  nsString href;
> +  aLinkElement->GetAttr(kNameSpaceID_None, nsGkAtoms::href, href);

Seems like it would be nice to factor out all this repeated "get the href or bail out" code into a helper so we only have one opy of it.

::: dom/base/nsDocument.cpp:3452
(Diff revision 6)
> +  nsString href;
> +  aLinkElement->GetAttr(kNameSpaceID_None, nsGkAtoms::href, href);
> +  (*l10n)->RemoveResourceId(href);
> +}
> +
> +mozilla::Maybe<RefPtr<DocumentL10n>>

Why not just return DocumentL10n*?  In practice, the only two return values from this function are Some(nonnull) and Nothing(), right?  And callers don't obviously need a strong ref.

::: dom/base/nsDocument.cpp:3490
(Diff revision 6)
> +    (*l10n)->OnDOMParsed();
> +  }
> +  mDOMParsed = true;
> +}
> +
> +already_AddRefed<DocumentL10n>

Why does this need to return an addrefed pointer?

::: dom/base/nsDocument.cpp:3494
(Diff revision 6)
> +
> +already_AddRefed<DocumentL10n>
> +nsIDocument::GetL10n(ErrorResult& aRv)
> +{
> +  auto l10nOrig = GetLocalization(true);
> +  if (l10nOrig.isNothing()) {

How can this happen?

::: dom/base/nsDocument.cpp:5257
(Diff revision 6)
>  
>    // DOM manipulation after content loaded should not care if the element
>    // came from the preloader.
>    mPreloadedPreconnects.Clear();
>  
> +  // DOM has been parsed by now, we can start localization

Is there a reason this is placed at this particular spot in this function?  Worth documenting.

::: dom/base/nsIDocument.h:3145
(Diff revision 6)
>  
>    already_AddRefed<mozilla::dom::AboutCapabilities> GetAboutCapabilities(
>      ErrorResult& aRv);
> +  void LocalizationLinkAdded(Element* aLinkElement);
> +  void LocalizationLinkRemoved(Element* aLinkElement);
> +  mozilla::Maybe<RefPtr<mozilla::dom::DocumentL10n>> GetLocalization(bool initialize);

All this stuff obviously needs documentation.

::: dom/html/HTMLBodyElement.cpp:305
(Diff revision 6)
>                                                   aCompileEventHandlers);
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // We now have a body, if l10n resources were to be loaded, it would happen
> +  // already.
> +  OwnerDoc()->OnL10nResourceContainerParsed();

What does that notification end up doing?  This is _not_ a safe time to call into script, for example.

::: dom/html/HTMLLinkElement.cpp:155
(Diff revision 6)
>      NewRunnableMethod("dom::HTMLLinkElement::BindToTree", this, update));
>  
> +  if (aDocument) {
> +    nsAutoString rel;
> +    GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel);
> +    if (rel.EqualsLiteral("localization"))

So this can't be combined with other rel values?

::: dom/html/HTMLLinkElement.cpp:156
(Diff revision 6)
>  
> +  if (aDocument) {
> +    nsAutoString rel;
> +    GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel);
> +    if (rel.EqualsLiteral("localization"))
> +      aDocument->LocalizationLinkAdded(this);

Again, what does that notification end up doing?

::: dom/html/HTMLLinkElement.cpp:203
(Diff revision 6)
>  
> +  if (oldDoc) {
> +    nsAutoString rel;
> +    GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel);
> +    if (rel.EqualsLiteral("localization"))
> +      oldDoc->LocalizationLinkRemoved(this);

And here.

This seems a bit odd to me: what actions happen when the link is removed?  Should they also happen when the rel changes from "localization" to something else?

What about changes from something else to "localization"?

::: dom/webidl/Document.webidl:505
(Diff revision 6)
>  // Allows about: pages to query aboutCapabilities
>  partial interface Document {
>    [Throws, Func="nsDocument::CallerIsTrustedAboutPage"] readonly attribute AboutCapabilities aboutCapabilities;
>  };
>  
> +partial interface Document {

Needs documentation.

::: dom/webidl/DocumentL10n.webidl:10
(Diff revision 6)
> + */
> +
> +dictionary L10nKey {
> +  required DOMString id;
> +
> +  object? args = null;

_Really_ needs documentation.

::: dom/webidl/DocumentL10n.webidl:17
(Diff revision 6)
> +
> +[NoInterfaceObject]
> +interface DocumentL10n {
> +  [Throws] void setAttributes(Element aElement, DOMString aId, optional object aArgs);
> +
> +  [Throws] Promise<DOMString> formatValue(DOMString aId, optional object aArgs);

Is this always a new Promise?  If so, [NewObject] is better than [Throws] if it's just throwing for the Promise creation failure.

::: dom/webidl/DocumentL10n.webidl:19
(Diff revision 6)
> +interface DocumentL10n {
> +  [Throws] void setAttributes(Element aElement, DOMString aId, optional object aArgs);
> +
> +  [Throws] Promise<DOMString> formatValue(DOMString aId, optional object aArgs);
> +
> +  [Throws] Promise<sequence<DOMString>> formatValues(sequence<L10nKey> aKeys);

Likewise.

::: intl/l10n/DocumentL10n.h:26
(Diff revision 6)
> +
> +class Element;
> +class Promise;
> +struct L10nKey;
> +
> +class DocumentL10n final : public nsISupports,

Does this actually need to inherit from nsISupports?

::: intl/l10n/DocumentL10n.cpp:35
(Diff revision 6)
> + * or while document is still being parsed, or much later.
> + *
> + * For that reason we initialize the `DocumentLocalization` with information
> + * about those two states, and if either of them is `false` at the time
> + * of initialization, the corresponding method is expected to be fired
> + * by the parsed later.

parser?

::: intl/l10n/DocumentL10n.cpp:39
(Diff revision 6)
> + * of initialization, the corresponding method is expected to be fired
> + * by the parsed later.
> + */
> +DocumentL10n::DocumentL10n(nsIDocument* aDocument, bool aL10nResourceContainerParsed, bool aDOMParsed)
> +  : mDocument(aDocument)
> +  , mDocumentLocalization(nullptr)

Why is this initializer needed?

::: intl/l10n/DocumentL10n.cpp:71
(Diff revision 6)
> +DocumentL10n::OnDOMParsed()
> +{
> +  printf("DocumentL10n::OnDOMParsed\n");
> +  if (!mDOMParsed) {
> +    if (nsContentUtils::IsSafeToRunScript()) {
> +      mDocumentLocalization->OnDOMParsed();

This would be a null-crash if that createInstance in the constructor failed, no?

::: intl/l10n/DocumentL10n.cpp:141
(Diff revision 6)
> +}
> +
> +void
> +DocumentL10n::SetAttributes(JSContext* cx, Element& aElement, const nsAString& aId, const Optional<JS::Handle<JSObject*>>& aArgs, ErrorResult& aRv)
> +{
> +  JS::RootedValue args(cx);

DOM style is JS::Rooted<JS::Value>

::: intl/l10n/DocumentL10n.cpp:159
(Diff revision 6)
> +}
> +
> +already_AddRefed<Promise>
> +DocumentL10n::FormatValue(JSContext* cx, const nsAString& aId, const Optional<JS::Handle<JSObject*>>& aArgs, ErrorResult& aRv)
> +{
> +  JS::RootedValue args(cx);

Again.

::: intl/l10n/DocumentL10n.cpp:180
(Diff revision 6)
> +}
> +
> +already_AddRefed<Promise>
> +DocumentL10n::FormatValues(JSContext* cx, const Sequence<L10nKey>& aKeys, ErrorResult& aRv)
> +{
> +  nsTArray<JS::HandleValue> jsKeys;

No, this is not OK.  You need to root these guys.  This will fail the GC hazard analysis.
Attachment #8979775 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #64)
> > Curious if you've considered exposing DOMLocalization to C++ and then creating it from the DocumentL10n class instead of using XPCOM/JS for this? Is the issue that the initialization state management would be worse to deal with in C++, or is there another reason to have the new JS class here?
> 
> It does a bit more. `DocumentLocalization` also manages document's state and
> will in the next iteration of the patch do things like setting document's
> direction based on the resolved locale.
> 
> What I did consider is making `DocumentLocalization` extend
> `DOMLocalization`. The difference is that `DOMLocalization` uses the
> constructor to set its state, while `DocumentLocalization` has to use
> `init()`. I'm not sure which way to resolve it.

It looks like DOMLocalization is currently only ever initialized once (when setting `document.l10n`). Won't that code (l10n.js) go away after the WebIDL conversion? If so, could we fold this new logic directly into DOMLocalization (possibly renaming to DocumentLocalization in the process), and have it do setup on init() instead of constructor?
(Assignee)

Comment 67

7 months ago
> It looks like DOMLocalization is currently only ever initialized once (when setting `document.l10n`). Won't that code (l10n.js) go away after the WebIDL conversion?

Not really. DOMLocalization can be initialized for any `Localization` object. We start small - with one Localization per Document (which is a DOMLocalization), but we want to stay flexible and keep the ability to initialize additional DOMLocalization objects separately.

For example:

```
<window>
  <linkset name="main"></linkset>
  <linkset name="menu"></linkset>
</window>
```
and its equivalent in HTML would allow us to have two Localization objects in a single document.

We can add `init` to DOMLocalization and make `DocumentLocalization` extend it tho. I'll look into this.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #64)
> > If addResourceId can be called before  onL10nResourceContainerParsed happens (as I gather from the if (this._resolveLocalization) condition), then won't onL10nResourceContainerParsed throw in that case?
> 
> No. Basically, all `addResourceId` called before
> `onL10nResourceContainerParsed` are collected and send to
> `DocumentLocalization` when its initialized.
> All later `addResourceId` are sent individually to already existing object.

Why is there the `if (this._resolveLocalization)` condition in `addResourceId` then? Wouldn't that never happen since _resolveLocalization is nulled out at onL10nResourceContainerParsed.
(In reply to Brian Grinstead [:bgrins] from comment #68)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #64)
> > > If addResourceId can be called before  onL10nResourceContainerParsed happens (as I gather from the if (this._resolveLocalization) condition), then won't onL10nResourceContainerParsed throw in that case?
> > 
> > No. Basically, all `addResourceId` called before
> > `onL10nResourceContainerParsed` are collected and send to
> > `DocumentLocalization` when its initialized.
> > All later `addResourceId` are sent individually to already existing object.
> 
> Why is there the `if (this._resolveLocalization)` condition in
> `addResourceId` then? Wouldn't that never happen since _resolveLocalization
> is nulled out at onL10nResourceContainerParsed.

Nevermind, I think see what you meant now. DocumentL10n::AddResourceId can call addResourceId before onL10nResourceContainerParsed is triggered - in which case the JS collects resources to process later once onL10nResourceContainerParsed happens.

Comment 70

7 months ago
mozreview-review
Comment on attachment 8979775 [details]
Bug 1455649 - Implement DocumentL10n WebIDL.

https://reviewboard.mozilla.org/r/245922/#review254542

::: intl/l10n/test/document_l10n/xul/test_lazy_webcomponent.xul:34
(Diff revision 6)
> +      return waitForMutation(element, opts, cb);
> +    }
> +
> +    SimpleTest.waitForExplicitFinish();
> +
> +    function appendL10nResource(id) {

Can this helper be added to document.l10n? I'm guessing this'll need to be used in many different places (including Custom Elemets). We could tack it onto MozXULElement if you don't forsee other consumers.
Comment on attachment 8979774 [details]
Bug 1455649 - Implement DocumentLocalization XPIDL API.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #67)
> > It looks like DOMLocalization is currently only ever initialized once (when setting `document.l10n`). Won't that code (l10n.js) go away after the WebIDL conversion?
> 
> Not really. DOMLocalization can be initialized for any `Localization`
> object. We start small - with one Localization per Document (which is a
> DOMLocalization), but we want to stay flexible and keep the ability to
> initialize additional DOMLocalization objects separately.
> 
> For example:
> 
> ```
> <window>
>   <linkset name="main"></linkset>
>   <linkset name="menu"></linkset>
> </window>
> ```
> and its equivalent in HTML would allow us to have two Localization objects
> in a single document.
> 
> We can add `init` to DOMLocalization and make `DocumentLocalization` extend
> it tho. I'll look into this.

OK, sounds good. f+ overall, my main concerns are around duplicated code between the two classes and keeping track of the various startup states, but those both seem addressable.
Attachment #8979774 - Flags: feedback?(bgrinstead) → feedback+
Blocks: 1469339
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8979774 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8979775 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8979776 - Attachment is obsolete: true
(Assignee)

Comment 81

6 months ago
The latest iteration of the patchset! I believe we're very close now. It implements (almost) all of the API and binds into Fluent the way we decided to go.

There are still open questions left, but I also think it's a good moment to get a last overview-level feedback before I start applying Boris' code feedback, adding docs and tests.

The changes are not that huge, but I basically removed the need for the the `mozDocumentLocalization` proxy, reducing `mozDOMLocalization` to a shell, that may also go away. I also made a huge progress in lifecycle of the DOMLocalization instance plugging it better into how DOM operates, and finally I now understand the code, which is important for my ability to maintain it once it lands.

Requests:

 - Stas - can you take a look at parts 1,2 and 9 and verify if this looks more-or-less sane to you? I'm happy to separate those patches out and provide them to git fluent repo as well, but first I'm trying to ensure that we, and talos, agree on the high level approach.

 - Mossop - can you take a look at parts 1-9 and verify that this looks like the right direction? Also, there are three `XXX(Mossop)` requests that I'd like to ask for you help with implementation. I know what should happen, I believe I wrote the right code, and the compiler/runtime disagrees. Lastly, I'd like your module owner confirmation that this work is aligned with bug 1425104.

 - Pike - can you take a look at part 3 and verify that it looks sane? Once again, high level direction - plugging FTLResource and accepting contexts with some FTLResources missing (as per yours and Flod's feedback).


I'm not asking for feedback from Boris yet, because I still didn't apply all of his feedback from the last round. I'll start that now, which is why I'm not setting f? flags that MozReview would happily strip on the next update. I'll use `ni?` instead.
Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Flags: needinfo?(dtownsend)

Comment 82

6 months ago
mozreview-review
Comment on attachment 8987642 [details]
Bug 1455649 - DocumentL10n, part 2 - Update FluentDOM to work with DocumentL10n.

https://reviewboard.mozilla.org/r/252876/#review259426


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/DOMLocalization.jsm:520
(Diff revision 1)
>          throw new Error("Cannot add a root that overlaps with existing root.");
>        }
>      }
>  
> +    if (this.windowElement) {
> +      if (this.windowElement !== newRoot.ownerDocument.defaultView) {

Error: Use .ownerglobal instead of .ownerdocument.defaultview [eslint: mozilla/use-ownerGlobal]

::: intl/l10n/DOMLocalization.jsm:524
(Diff revision 1)
> +    if (this.windowElement) {
> +      if (this.windowElement !== newRoot.ownerDocument.defaultView) {
> +        throw new Error("This DOMLocalization already has a root attached for a different window.");
> +      }
> +    } else {
> +      this.windowElement = newRoot.ownerDocument.defaultView;

Error: Use .ownerglobal instead of .ownerdocument.defaultview [eslint: mozilla/use-ownerGlobal]

::: intl/l10n/Localization.jsm:119
(Diff revision 1)
>      this.generateMessages = generateMessages;
> +    if (resourceIds) {
> -    this.ctxs =
> +      this.ctxs =
> -      new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> +        new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> -  }
> +    }
> +    //XXX(Mossop): This crashes the browser :(

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: intl/l10n/Localization.jsm:120
(Diff revision 1)
> +    if (resourceIds) {
> -    this.ctxs =
> +      this.ctxs =
> -      new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> +        new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> -  }
> +    }
> +    //XXX(Mossop): This crashes the browser :(
> +    //this.registerObservers();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 83

6 months ago
mozreview-review
Comment on attachment 8987643 [details]
Bug 1455649 - DocumentL10n, part 3 - Update L10nRegistry to operate on FTLResources.

https://reviewboard.mozilla.org/r/252878/#review259428


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/Localization.jsm:119
(Diff revision 1)
>      this.generateMessages = generateMessages;
>      if (resourceIds) {
>        this.ctxs =
>          new CachedAsyncIterable(this.generateMessages(this.resourceIds));
>      }
>      //XXX(Mossop): This crashes the browser :(

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: intl/l10n/Localization.jsm:120
(Diff revision 1)
>      if (resourceIds) {
>        this.ctxs =
>          new CachedAsyncIterable(this.generateMessages(this.resourceIds));
>      }
>      //XXX(Mossop): This crashes the browser :(
>      //this.registerObservers();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 84

6 months ago
mozreview-review
Comment on attachment 8987649 [details]
Bug 1455649 - DocumentL10n, part 5 - Add directionality setting to DOMLocalization::translateRoots.

https://reviewboard.mozilla.org/r/252890/#review259436


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/DOMLocalization.jsm:522
(Diff revision 1)
>          throw new Error("Cannot add a root that overlaps with existing root.");
>        }
>      }
>  
>      if (this.windowElement) {
>        if (this.windowElement !== newRoot.ownerDocument.defaultView) {

Error: Use .ownerglobal instead of .ownerdocument.defaultview [eslint: mozilla/use-ownerGlobal]

::: intl/l10n/DOMLocalization.jsm:526
(Diff revision 1)
>      if (this.windowElement) {
>        if (this.windowElement !== newRoot.ownerDocument.defaultView) {
>          throw new Error("This DOMLocalization already has a root attached for a different window.");
>        }
>      } else {
>        this.windowElement = newRoot.ownerDocument.defaultView;

Error: Use .ownerglobal instead of .ownerdocument.defaultview [eslint: mozilla/use-ownerGlobal]

Comment 85

6 months ago
mozreview-review
Comment on attachment 8987641 [details]
Bug 1455649 - DocumentL10n, part 1 - Add FTLResource object to Fluent.

https://reviewboard.mozilla.org/r/252874/#review259564

::: intl/l10n/MessageContext.jsm:29
(Diff revision 1)
>  
>  const entryIdentifierRe = /-?[a-zA-Z][a-zA-Z0-9_-]*/y;
>  const identifierRe = /[a-zA-Z][a-zA-Z0-9_-]*/y;
>  const functionIdentifierRe = /^[A-Z][A-Z_?-]*$/;
>  
> +class FTLResource {

I'd like FTLResource to be called FluentDocument. I'll use that going forward.

::: intl/l10n/MessageContext.jsm:34
(Diff revision 1)
> +class FTLResource {
> +  constructor(source) {
> +    const [entries, errors] = parse(source);
> +    if (errors.length > 0) {
> +      errors.forEach(console.error);
> +      throw new Error(`FTLResource could not be parsed.`);

From the discussion in https://github.com/projectfluent/fluent.js/issues/217, I understood that FluentDocument was intended to be a low-level and lightweight abstraction over a map of messages. I'd like to avoid any side effects in the constructor, like the console.error.

Throwing an error also significantly changes Fluent's error recovery strategy. I'd expect such change to first be discussed upstream.

I'm also concerned about FluentDocument's dependency on the parse() method, which implicitly makes the class bound to the MessageContext.

I'd like to suggest a different API which address all the above concerns:

    class FluentDocument extends Map {
        constructor(entries, errors = []) {
            super(entries);
            this.errors = errors;
        }
    }

    class MessageContext {
        …
        static parseDocument(source) {
            // Alternatively, create the instance
            // of FluentDocument inside of parse().
            let [entries, errors] = parse(source);
            return new FluentDocument(entries, errors);
        }
    }

::: intl/l10n/MessageContext.jsm:36
(Diff revision 1)
> +    const [entries, errors] = parse(source);
> +    if (errors.length > 0) {
> +      errors.forEach(console.error);
> +      throw new Error(`FTLResource could not be parsed.`);
> +    }
> +    this._entries = entries;

I like that FluentDocument is flat in your patch and doesn't distinguish between terms and messages. That's MessageContext's job. If you make FluentDocument extend Map, you won't need the `_entries` field at all.

::: intl/l10n/MessageContext.jsm:1801
(Diff revision 1)
> +
> +  addResource(res) {
> +    return this._loadEntries(res._entries);
> +  }
> +
> +  _loadEntries(entries, errors = []) {

I associate "load" with some kind of fetching, so I'd prefer a different verb here. "Add" or "read" might be a good alternatives.

That said, I don't think this internal method is even needed. With the static parseDocument which I suggested above, you could have addDocument with all the logic currently in `_loadEntries` here, and then addMessages implemented as:

    addMessages(source) {
        const doc = this.parseDocument(source);
        return this.addDocument(doc);
    }

::: intl/l10n/MessageContext.jsm:1816
(Diff revision 1)
>        } else {
>          if (this._messages.has(id)) {
>            errors.push(`Attempt to override an existing message: "${id}"`);
>            continue;
>          }
>          this._messages.set(id, entries[id]);

I liked your idea to set the value to the FluentDocument containing this `id`. It makes the ownership of data clearer to me.

Comment 86

6 months ago
mozreview-review
Comment on attachment 8987642 [details]
Bug 1455649 - DocumentL10n, part 2 - Update FluentDOM to work with DocumentL10n.

https://reviewboard.mozilla.org/r/252876/#review259576

::: intl/l10n/DOMLocalization.jsm:411
(Diff revision 1)
>     * @param {Array<String>}    resourceIds      - List of resource IDs
>     * @param {Function}         generateMessages - Function that returns a
>     *                                              generator over MessageContexts
>     * @returns {DOMLocalization}
>     */
> -  constructor(windowElement, resourceIds, generateMessages) {
> +  constructor(resourceIds, generateMessages) {

I don't know much about XPIDL so I'm not sure I'm the right person to review this. I'm also confused. The commit message reads "we need to make the constructor work without taking arguments" but this clearly still takes two arguments. I'm assuming that you meant that the constructor can't take the reference to the window as the argument because we don't have it yet at the time when `DOMLocalization` is instantiated?

::: intl/l10n/DOMLocalization.jsm:420
(Diff revision 1)
>      this.roots = new Set();
>      // requestAnimationFrame handler.
>      this.pendingrAF = null;
>      // list of elements pending for translation.
>      this.pendingElements = new Set();
> -    this.windowElement = windowElement;
> +    this.windowElement = null;

Do we need to store `windowElement` on the instance? It looks like we could pass it as an argument to `translateMutations` when we create the MutationObserver.

::: intl/l10n/DOMLocalization.jsm:520
(Diff revision 1)
>          throw new Error("Cannot add a root that overlaps with existing root.");
>        }
>      }
>  
> +    if (this.windowElement) {
> +      if (this.windowElement !== newRoot.ownerDocument.defaultView) {

Is this possible now with XPIDL?

::: intl/l10n/DOMLocalization.jsm:521
(Diff revision 1)
>        }
>      }
>  
> +    if (this.windowElement) {
> +      if (this.windowElement !== newRoot.ownerDocument.defaultView) {
> +        throw new Error("This DOMLocalization already has a root attached for a different window.");

I find the "attached for" part of the error message confusing. Ideally, the error message would start with "Cannot connect a root" and then give a reason.

::: intl/l10n/DOMLocalization.jsm:554
(Diff revision 1)
> +    // Pause the mutation observer to stop observing `root`.
>      this.pauseObserving();
> +
> +    if (this.roots.size === 0) {
> +      this.mutationObserver = null;
> +      this.windowElement = null;

It looks like we can `return true;` here (and skip the `else` clause) to avoid the same check in line 559.

::: intl/l10n/DOMLocalization.jsm:592
(Diff revision 1)
>     * Resumes the `MutationObserver`.
>     *
>     * @private
>     */
>    resumeObserving() {
> +    if (!this.mutationObserver) {

Please make this check consistent with the one in `pauseObserving`. Either way is fine.

Comment 87

6 months ago
mozreview-review
Comment on attachment 8987649 [details]
Bug 1455649 - DocumentL10n, part 5 - Add directionality setting to DOMLocalization::translateRoots.

https://reviewboard.mozilla.org/r/252890/#review259582

::: intl/l10n/DOMLocalization.jsm:572
(Diff revision 1)
>     * @returns {Promise}
>     */
>    translateRoots() {
>      const roots = Array.from(this.roots);
>      return Promise.all(
> -      roots.map(root => this.translateFragment(root))
> +      roots.map(root => this.translateFragment(root).then(() => {

Please use await here if you can.

::: intl/l10n/DOMLocalization.jsm:574
(Diff revision 1)
>    translateRoots() {
>      const roots = Array.from(this.roots);
>      return Promise.all(
> -      roots.map(root => this.translateFragment(root))
> +      roots.map(root => this.translateFragment(root).then(() => {
> +        let primaryLocale = Services.locale.getAppLocaleAsBCP47();
> +        let directionality = Services.locale.isAppLocaleRTL;

I'm concerned that referencing Services will be an issue when upstreaming this code to fluent.js. Do you have a suggestion how to mitigate this?

::: intl/l10n/DOMLocalization.jsm:578
(Diff revision 1)
> +        let primaryLocale = Services.locale.getAppLocaleAsBCP47();
> +        let directionality = Services.locale.isAppLocaleRTL;
> +        root.setAttribute("lang", primaryLocale);
> +        root.setAttribute(
> +          root.namespaceURI === "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" ? "localedir" : "dir",
> +          directionality ? "rtl" : "ltr");

Should "directionality" be called "isRTL" or something similar?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #81)

>  - Stas - can you take a look at parts 1,2 and 9 and verify if this looks
> more-or-less sane to you? I'm happy to separate those patches out and
> provide them to git fluent repo as well, but first I'm trying to ensure that
> we, and talos, agree on the high level approach.

Thanks for looping me on. Parts 2 and 9 look good to me, although I can't really comment on the motives of part 2. In part 9, I'm a bit concerned about how we'll upstream this code to fluent.js.

Wrt. part 1, this looks like a good first step. I'd like for the API design discussion to happen in https://github.com/projectfluent/fluent.js/issues/217 where I started documenting the rationales last week.
Flags: needinfo?(stas)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 98

6 months ago
mozreview-review
Comment on attachment 8987641 [details]
Bug 1455649 - DocumentL10n, part 1 - Add FTLResource object to Fluent.

https://reviewboard.mozilla.org/r/252874/#review260172

::: intl/l10n/MessageContext.jsm:29
(Diff revision 1)
>  
>  const entryIdentifierRe = /-?[a-zA-Z][a-zA-Z0-9_-]*/y;
>  const identifierRe = /[a-zA-Z][a-zA-Z0-9_-]*/y;
>  const functionIdentifierRe = /^[A-Z][A-Z_?-]*$/;
>  
> +class FTLResource {

Document is the term used for the thing we're localising so I'm not sure it is a good choice here.
(Assignee)

Comment 99

6 months ago
Updated to :stas', :bz's and :reviewbot's comments. Thank you all!

Pending Mossop's feedback and help with the "XXX(Mossop)"'s.
(Assignee)

Comment 100

6 months ago
> Document is the term used for the thing we're localising so I'm not sure it is a good choice here.

Yeah, I agree here. I'm not particularly fond of a name "FluentDocument", because it doesn't correspond to anything else around it - we don't have CSSDocument or JSDocuments flying around.

I used it because :stas asked for it, but I'd be happy to see a conversation about naming where I could voice my opinion :)

Updated

6 months ago
Depends on: 1471726
Sorry, I got ahead of myself. Let's use FluentResource here and let's discuss FluentDocument in https://github.com/projectfluent/fluent.js/issues/217

Comment 102

6 months ago
mozreview-review
Comment on attachment 8987643 [details]
Bug 1455649 - DocumentL10n, part 3 - Update L10nRegistry to operate on FTLResources.

https://reviewboard.mozilla.org/r/252878/#review260330

::: intl/l10n/L10nRegistry.jsm:98
(Diff revision 2)
>      if (this.bootstrap !== null) {
>        await this.bootstrap;
>      }
>      const sourcesOrder = Array.from(this.sources.keys()).reverse();
>      for (const locale of requestedLangs) {
> -      yield * generateContextsForLocale(locale, sourcesOrder, resourceIds);
> +      for await (let set of generateResourceSetsForLocale(locale, sourcesOrder, resourceIds)) {

This is just a `for (let...`, right? `generateResourceSetsForLocale` isn't async anymore?

::: intl/l10n/L10nRegistry.jsm:104
(Diff revision 2)
> +        let dataSet = await Promise.all(set);
> +
> +        if (dataSet.every(d => d === false)) {
> +          continue;
> +        }
> +        const pseudoNameFromPref = Services.prefs.getStringPref("intl.l10n.pseudo", "");

I'd move this out of the loop, I think that'd be more consistent in cases where we race-condition the pref setting.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #81)
>  - Pike - can you take a look at part 3 and verify that it looks sane? Once
> again, high level direction - plugging FTLResource and accepting contexts
> with some FTLResources missing (as per yours and Flod's feedback).

lgtm. I took the liberty to leave some code-level comments regardless.
Flags: needinfo?(l10n)
(Reporter)

Comment 104

6 months ago
mozreview-review
Comment on attachment 8987642 [details]
Bug 1455649 - DocumentL10n, part 2 - Update FluentDOM to work with DocumentL10n.

https://reviewboard.mozilla.org/r/252876/#review260528

::: intl/l10n/DOMLocalization.jsm:583
(Diff revision 2)
>     */
>    pauseObserving() {
> +    if (!this.mutationObserver) {
> +      return;
> +    }
> +

Minor issue but if someone calls pause and then connects the first root it will not be paused. Not sure if you care about that case or not.

::: intl/l10n/Localization.jsm:120
(Diff revision 2)
> +    if (resourceIds) {
> -    this.ctxs =
> +      this.ctxs =
> -      new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> +        new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> -  }
> +    }
> +    // XXX(Mossop): This crashes the browser :(
> +    // this.registerObservers();

Stack trace?
(Reporter)

Comment 105

6 months ago
mozreview-review
Comment on attachment 8987644 [details]
Bug 1455649 - DocumentL10n, part 4 - Add mozIDOMLocalization API.

https://reviewboard.mozilla.org/r/252880/#review260534

::: intl/l10n/mozDOMLocalization.js:12
(Diff revision 2)
> +mozDOMLocalization.prototype.classID =
> +  Components.ID("{29cc3895-8835-4c5b-b53a-0c0d1a458dee}");
> +mozDOMLocalization.prototype.QueryInterface =
> +  ChromeUtils.generateQI([Ci.mozIDOMLocalization]);
> +
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory([mozDOMLocalization]);

Rather than declaring an entire class that extends DOMLocalization you could just create your own factory function here and return an instance of DOMLocalization directly. Might be a bit better memory-wise.
(Reporter)

Comment 106

6 months ago
mozreview-review
Comment on attachment 8987645 [details]
Bug 1455649 - DocumentL10n, part 5 - Add C++ DocumentL10n API.

https://reviewboard.mozilla.org/r/252882/#review260538

::: dom/webidl/DocumentL10n.webidl:24
(Diff revision 2)
> +  [NewObject] Promise<DOMString> formatValue(DOMString aId, optional object aArgs);
> +
> +  [Throws] void setAttributes(Element aElement, DOMString aId, optional object aArgs);
> +  [Throws] L10nKey getAttributes(Element aElement);
> +
> +  /* XXX(Mossop): I can't find a way to get thos two APIs to work as WebIDL */

What error are you seeing?
(Reporter)

Comment 107

6 months ago
Looks generally sane, I can look in more detail at the missing bits next week.
(Assignee)

Comment 108

6 months ago
(In reply to Dave Townsend [:mossop] from comment #104)
> ::: intl/l10n/Localization.jsm:120
> (Diff revision 2)
> > +    if (resourceIds) {
> > -    this.ctxs =
> > +      this.ctxs =
> > -      new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> > +        new CachedAsyncIterable(this.generateMessages(this.resourceIds));
> > -  }
> > +    }
> > +    // XXX(Mossop): This crashes the browser :(
> > +    // this.registerObservers();
> 
> Stack trace?

JavaScript error: resource://gre/modules/Localization.jsm, line 251: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIObserverService.addObserver]
Assertion failure: mRawPtr != nullptr (You can't dereference a NULL nsCOMPtr with operator->().), at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/nsCOMPtr.h:808

(In reply to Dave Townsend [:mossop] from comment #106)
> Comment on attachment 8987645 [details]
> Bug 1455649 - DocumentL10n, part 5 - Add C++ DocumentL10n API.
> 
> https://reviewboard.mozilla.org/r/252882/#review260538
> 
> ::: dom/webidl/DocumentL10n.webidl:24
> (Diff revision 2)
> > +  [NewObject] Promise<DOMString> formatValue(DOMString aId, optional object aArgs);
> > +
> > +  [Throws] void setAttributes(Element aElement, DOMString aId, optional object aArgs);
> > +  [Throws] L10nKey getAttributes(Element aElement);
> > +
> > +  /* XXX(Mossop): I can't find a way to get thos two APIs to work as WebIDL */
> 
> What error are you seeing?

 0:26.49 In file included from /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dom/bindings/UnifiedBindings4.cpp:134:
 0:26.49 /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dom/bindings/DocumentL10nBinding.cpp:588:101: error: too few arguments to function call, expected 3, have 2
 0:26.49   auto result(StrongOrRawPtr<Promise>(self->TranslateFragment(MOZ_KnownLive(NonNullHelper(arg0)), rv)));
 0:26.49                                       ~~~~~~~~~~~~~~~~~~~~~~~                                       ^
 0:26.49 /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/mozilla/dom/DocumentL10n.h:64:3: note: 'TranslateFragment' declared here
 0:26.49   already_AddRefed<Promise> TranslateFragment(JSContext* cx, Element& aElement, ErrorResult& aRv);
 0:26.49   ^
 0:26.50 In file included from /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dom/bindings/UnifiedBindings4.cpp:134:
 0:26.50 /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dom/bindings/DocumentL10nBinding.cpp:681:81: error: too few arguments to function call, expected 3, have 2
 0:26.50   auto result(StrongOrRawPtr<Promise>(self->TranslateElements(Constify(arg0), rv)));
 0:26.50                                       ~~~~~~~~~~~~~~~~~~~~~~~                   ^
 0:26.50 /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/mozilla/dom/DocumentL10n.h:65:3: note: 'TranslateElements' declared here
 0:26.50   already_AddRefed<Promise> TranslateElements(JSContext* cx, const Sequence<Element>& aElements, ErrorResult& aRv);
 0:26.50   ^
 0:26.50 2 errors generated.


====================

Ok, I'll start working on tests and docs then, while awaiting further help with the 4 remaining issues marked as "XXX(Mossop)" :)
(Assignee)

Comment 109

6 months ago
The error above (with 3 vs 2 arguments) is confusing because the third argument should be the promise. arg1 - elements/fragment, arg2 - promise, arg3 - rv. But somehow the codegen creates a call with only two.
> arg1 - elements/fragment, arg2 - promise, arg3 - rv. But somehow the codegen creates a call with only two.

For translateElements?  There should be two arguments: the elements and rv.  The promise is the return value.

The function declaration wants a JSContext for some reason; just take that part out.  It's unused anyway.

While we're here, the xpidl TranslateElements wants an Element**, right?  And you have a Sequence<Element*> (not Sequence<Element>)?  So you should be able to do:  

  mDOMLocalization->TranslateElements(aElements.Elements(), aElements.Length(),
                                      getter_AddRefs(promise));
(Assignee)

Comment 111

6 months ago
Thank you!

Got the IDL to work now!

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #110)
> While we're here, the xpidl TranslateElements wants an Element**, right? 
> And you have a Sequence<Element*> (not Sequence<Element>)?  So you should be
> able to do:  
> 
>   mDOMLocalization->TranslateElements(aElements.Elements(),
> aElements.Length(),
>                                       getter_AddRefs(promise));


Tried that:

```
already_AddRefed<Promise>
DocumentL10n::TranslateElements(const Sequence<OwningNonNull<Element>>& aElements, ErrorResult& aRv)
{
  RefPtr<Promise> promise;
  nsresult rv = mDOMLocalization->TranslateElements(
    aElements.Elements(), aElements.Length(), getter_AddRefs(promise));
  return promise.forget();
}
```

and got:

```
 0:07.66 /home/zbraniecki/projects/mozilla-unified/intl/l10n/DocumentL10n.cpp:222:7: error: cannot initialize a parameter of type 'mozilla::dom::Element **' with an rvalue of type 'const nsTArray_Impl<mozilla::OwningNonNull<mozilla::dom::Element>, nsTArrayFallibleAllocator>::elem_type *' (aka 'const mozilla::OwningNonNull<mozilla::dom::Element> *')
 0:07.66       aElements.Elements(), aElements.Length(), getter_AddRefs(promise));
 0:07.66       ^~~~~~~~~~~~~~~~~~~~
```

I assume I have to extract the pointer out of the `OwningNonNull`? Is there any equivalent of `map`? Never used the `OwningNonNull` and searchfox results are a bit confusing.
Flags: needinfo?(bzbarsky)
Oh, your input sequence is sequence<Element>, not sequence<Element*>?

In that case you're going to need a separate nsTArray<Element*> and copy things into it.  OwningNonNull<T> has a conversion operator to T*, so this should be as simple as:

  nsTArray<Element*> elements;
  elements.SetCapacity(aElements.Length());  // If you don't care about crash on possible oom
  for (auto& element : aElements) {
    elements.AppendElement(element);
  }

and then pass elements.Elements() to the xpidl thing.
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 122

6 months ago
Sweet, thank you!

We're down to two open request for help:

1) How to stringify a JSON in C++ (line 172 in DocumentL10n.cpp)
2) Why does setting Observer in Localization crashes the constructor (line 119 in Localization.jsm) - here I suspect it's something about XPIDL vs. JSM :(
> 1) How to stringify a JSON in C++ (line 172 in DocumentL10n.cpp)

So starting with a "const Optional<JS::Handle<JSObject*>>& aArgs"?

Something like this, assuming aArgs is not nullable in IDL:

  if (aArgs.WasPassed()) {
    JS::Rooted<JS::Value> val(cx, JS::ObjectValue(*aArgs.Value()));
    // stuff
  }

where "stuff" can be cribbed from either GetParamsForMessage in dom/base/nsFrameMessageManager.cpp or SerializeFromJSVal in dom/payments/PaymentRequestUtils.cpp.  Or better yet we should factor this out into an nsContentUtils helper function that takes a JSContext* and a JS::MutableHandle<JS::Value> and an nsAString& and returns boolean.

>2) Why does setting Observer in Localization crashes the constructor

Like mossop said, stack trace?  From a C++ debugger, to that null-deref assert or the crash itself.
(Assignee)

Comment 124

5 months ago
> Like mossop said, stack trace?  From a C++ debugger, to that null-deref assert or the crash itself.

```
#0  0x00007fffe3de6b57 in nsCOMPtr<mozIDOMLocalization>::operator->() const (this=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/nsCOMPtr.h:807
#1  0x00007fffe3de5798 in mozilla::dom::DocumentL10n::AddResourceIds(nsTArray<nsTString<char16_t> >&) (this=<optimized out>, aResourceIds=...) at /home/zbraniecki/projects/mozilla-unified/intl/l10n/DocumentL10n.cpp:78
#2  0x00007fffe52019ca in nsIDocument::OnL10nResourceContainerParsed() (this=0x7fffbaad9000)
    at /home/zbraniecki/projects/mozilla-unified/dom/base/nsDocument.cpp:3383
#3  0x00007fffe6b116c4 in mozilla::dom::XULDocument::AddElementToDocumentPost(mozilla::dom::Element*) (this=
    0x7fffbaad9000, aElement=0x7fffc051b160)
    at /home/zbraniecki/projects/mozilla-unified/dom/xul/XULDocument.cpp:1379
#4  0x00007fffe6b0dd23 in mozilla::dom::XULDocument::ResumeWalk() (this=0x7fffbaad9000)
    at /home/zbraniecki/projects/mozilla-unified/dom/xul/XULDocument.cpp:2391
#5  0x00007fffe6b0d377 in mozilla::dom::XULDocument::EndLoad() (this=0x7fffbaad9000)
    at /home/zbraniecki/projects/mozilla-unified/dom/xul/XULDocument.cpp:483
#6  0x00007fffe6b1a0ec in XULContentSinkImpl::DidBuildModel(bool) (this=0x7fffc06fe980, aTerminated=<optimized out>) at /home/zbraniecki/projects/mozilla-unified/dom/xul/nsXULContentSink.cpp:221
#7  0x00007fffe4ab903f in nsParser::DidBuildModel(nsresult) (this=0x7fffba68af60, anErrorCode=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsParser.cpp:492
#8  0x00007fffe4ab96ad in nsParser::ResumeParse(bool, bool, bool) (this=0x7fffba68af60, allowIteration=true, aIsFinalChunk=<optimized out>, aCanInterrupt=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsParser.cpp:1102
#9  0x00007fffe4abaaa6 in nsParser::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (this=0x7fffba68af60, request=0x7fffbaa090a8, aContext=0x0, status=nsresult::NS_OK)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsParser.cpp:1477
#10 0x00007fffe4a635c5 in nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (this=<optimized out>, request=0x7fffbaa090a8, aCtxt=0x0, aStatus=nsresult::NS_OK)
    at /home/zbraniecki/projects/mozilla-unified/uriloader/base/nsURILoader.cpp:368
#11 0x00007fffe3e32ec1 in nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (this=
    0x7fffbaa09060, request=<optimized out>, ctxt=<optimized out>, status=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/netwerk/base/nsBaseChannel.cpp:879
#12 0x00007fffe3e32f30 in non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) ()
    at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/nsCOMPtr.h:1176
#13 0x00007fffe3e57469 in nsInputStreamPump::OnStateStop() (this=0x7fffc1142f20)
    at /home/zbraniecki/projects/mozilla-unified/netwerk/base/nsInputStreamPump.cpp:706
#14 0x00007fffe3e56b11 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (this=0x7fffc1142f20, stream=<optimized out>) at /home/zbraniecki/projects/mozilla-unified/netwerk/base/nsInputStreamPump.cpp:436
#15 0x00007fffe3e5764d in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) ()
    at /home/zbraniecki/projects/mozilla-unified/netwerk/base/nsNetUtil.h:636
#16 0x00007fffe3d2b601 in nsInputStreamReadyEvent::Run() (this=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/xpcom/io/nsStreamUtils.cpp:102
#17 0x00007fffe3d66198 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffe0c5c120, aMayWait=<optimized out>, aResult=0x7fffffffcad7) at /home/zbraniecki/projects/mozilla-unified/xpcom/threads/nsThread.cpp:1051
#18 0x00007fffe3d680c5 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fffe0c5c120, aMayWait=false)
    at /home/zbraniecki/projects/mozilla-unified/xpcom/threads/nsThreadUtils.cpp:519
#19 0x00007fffe43cec83 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7ffff6ae4ac0, aDelegate=0x7ffff6a5a200) at /home/zbraniecki/projects/mozilla-unified/ipc/glue/MessagePump.cpp:97
#20 0x00007fffe4350cd1 in MessageLoop::RunInternal() (this=0x7ffff6a5a200)
    at /home/zbraniecki/projects/mozilla-unified/ipc/chromium/src/base/message_loop.cc:325
```

I'm not sure why registering observer would cause `mDOMLocalization` to be null tho :(
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 125

5 months ago
Oh, ok! The stack is only saying that mDOMLocalization is null, and it's null because the constructor threw.

Now as to why it threw my guess is that its something about the fact that here [0] I'm setting it on DOMLocalization object, but with my patch, I'm setting it on mozDOMLocalization XPCOM object, and this one doesn't expose observe method.
Thus the error is:

```
JavaScript error: resource://gre/modules/Localization.jsm, line 251: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIObserverService.addObserver]
```

Is the solution to add `observe` to XPIDL for mozDOMLocalization?


[0] https://searchfox.org/mozilla-central/source/intl/l10n/Localization.jsm#245
> but with my patch, I'm setting it on mozDOMLocalization XPCOM object, and this one doesn't expose observe method.

I'm not sure which of your diffs you're looking at, exactly.  But Localization in Localization.jsm doesn't implement nsIObserver.  Not sure whether that matters.

I have no idea what line 251 is in your copy of Localization.jsm, so can't really help you with that part...
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 127

5 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #122)
> Sweet, thank you!
> 
> We're down to two open request for help:
> 
> 1) How to stringify a JSON in C++ (line 172 in DocumentL10n.cpp)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #125)
> Oh, ok! The stack is only saying that mDOMLocalization is null, and it's
> null because the constructor threw.
> 
> Now as to why it threw my guess is that its something about the fact that
> here [0] I'm setting it on DOMLocalization object, but with my patch, I'm
> setting it on mozDOMLocalization XPCOM object, and this one doesn't expose
> observe method.
> Thus the error is:
> 
> ```
> JavaScript error: resource://gre/modules/Localization.jsm, line 251:
> NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE)
> [nsIObserverService.addObserver]
> ```
> 
> Is the solution to add `observe` to XPIDL for mozDOMLocalization?

That shouldn't matter, this is probably a problem with using weak references. It suggests the QueryInterface implementation isn't working for some reason.
Flags: needinfo?(dtownsend)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 137

5 months ago
mozreview-review
Comment on attachment 8987642 [details]
Bug 1455649 - DocumentL10n, part 2 - Update FluentDOM to work with DocumentL10n.

https://reviewboard.mozilla.org/r/252876/#review262318


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/Localization.jsm:250
(Diff revision 4)
>  
>    /**
>     * Register weak observers on events that will trigger cache invalidation
>     */
>    registerObservers() {
> -    Services.obs.addObserver(this, "intl:app-locales-changed", true);
> +    Services.obs.addObserver(this, "intl:app-locales-changed", false);

Error: AddObserver's third parameter can be omitted when it's false. [eslint: mozilla/no-useless-parameters]

::: intl/l10n/Localization.jsm:251
(Diff revision 4)
>    /**
>     * Register weak observers on events that will trigger cache invalidation
>     */
>    registerObservers() {
> -    Services.obs.addObserver(this, "intl:app-locales-changed", true);
> -    Services.prefs.addObserver("intl.l10n.pseudo", this, true);
> +    Services.obs.addObserver(this, "intl:app-locales-changed", false);
> +    Services.prefs.addObserver("intl.l10n.pseudo", this, false);

Error: AddObserver's third parameter can be omitted when it's false. [eslint: mozilla/no-useless-parameters]
(Assignee)

Comment 138

5 months ago
I was able to get it to work by using strong observer instead of a weak one.

I got the core codebase to work and there are no remaining open items, so I pushed the latest attempt to try and started extracting bits that need to land in Fluent:
 - https://github.com/projectfluent/fluent.js/pull/244
 - https://github.com/projectfluent/fluent.js/pull/245

I'm still not sure if the life cycle of DocumentL10n work well here. I tried setting printf in DocumentL10n::~DocumentL10n and it was not called after I closed Preferences tab.

I tested that nsIDocument::~nsIDocument destructor is called and it sets the mDocumentL10n to nullptr, so I'm not sure what's going on.

I hope to get reviewers help with this.

Comment 139

5 months ago
mozreview-review
Comment on attachment 8987643 [details]
Bug 1455649 - DocumentL10n, part 3 - Update L10nRegistry to operate on FTLResources.

https://reviewboard.mozilla.org/r/252878/#review262320


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/Localization.jsm:250
(Diff revision 4)
>  
>    /**
>     * Register weak observers on events that will trigger cache invalidation
>     */
>    registerObservers() {
>      Services.obs.addObserver(this, "intl:app-locales-changed", false);

Error: AddObserver's third parameter can be omitted when it's false. [eslint: mozilla/no-useless-parameters]

::: intl/l10n/Localization.jsm:251
(Diff revision 4)
>    /**
>     * Register weak observers on events that will trigger cache invalidation
>     */
>    registerObservers() {
>      Services.obs.addObserver(this, "intl:app-locales-changed", false);
>      Services.prefs.addObserver("intl.l10n.pseudo", this, false);

Error: AddObserver's third parameter can be omitted when it's false. [eslint: mozilla/no-useless-parameters]
(Assignee)

Comment 140

5 months ago
Created attachment 8990446 [details] [diff] [review]
patch to crash when trying to use weak observer

:mossop - here's a small patch that placed on top of the patchset crashes the browser when entering Preferences.

Because of the mentioned above issues with DocumentL10n lifecycle, I'd prefer to use weak observer here if you can help me figure out how to make it not crash :)
Assignee: dtownsend → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(dtownsend)
(Assignee)

Updated

5 months ago
Depends on: 1474786
(Assignee)

Updated

5 months ago
Depends on: 1384236
(Assignee)

Comment 141

5 months ago
I separated out bug 1384236 for fluent changes and bug 1474786 for l10nregistry changes.

I'm likely going to separate out mozIDOMLocalization API as well to iron out the unregisterobservers crash.
Flags: needinfo?(dtownsend)
Blocks: 1476097
(Assignee)

Updated

5 months ago
Depends on: 1475903
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8987641 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987642 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987643 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987644 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987645 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987646 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987647 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8987648 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8990446 - Attachment is obsolete: true
(Assignee)

Comment 147

5 months ago
Progress update:

 - Lowest level changes to Fluent and L10nRegistry have landed
 - The mozIDOMLocalization XPIDL API is in bug 1475903 and has r+
 - The remaining patches are here and eventually we'll be down to the first three really.

Now, the obstacles:

 - bug 1471726 is blocking both this bug and bug 1475903 since we currently can't pass arrays into XPIDL API from C++ which makes it impossible to test DocumentL10n WebIDL API
 - we struggle with the DocumentL10n's lifecycle vs. nsIDocument's lifecycle

I'll handle the latter once the former is usable and should have a patch with tests ready within a day or two after bug  1471726 lands.
(Assignee)

Updated

5 months ago
Depends on: 1480673
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8987649 - Attachment is obsolete: true
(Assignee)

Comment 153

5 months ago
Updates:
 - I separated out bug 1480798 for directionality change (r+ patch inside)
 - Based on comment 123 I filled bug 1480673 to add nsContentUtils::StringifyJSON (r? patches inside)
 - Bug 1475903 (mozIDOMLocalization) has r+ but is pending a leak (bug 1480500) that :nika is working on (bug 1480624)
 - Performance looks good
 - This is the final set of patches in this bug I believe. Need to finish tests and docs for them.

For the reviewer, I believe that the main open question is how the lifecycle of DocumentL10n fits into nsDocument's lifecycle. I tried to clean up in the nsDocument::~nsDocument but that seems to happen lazily much after the document is closed (fastcache?) and I'm not sure if this is the right logic.
I had to also guard against nullptr mDOMLocalization in DocumentL10n::RemoveResourceIds because it was fired somehow after mDOMLocalization is empty, so I further suspect lifecycle issues.

Overall, tunnel, light, and so on on.
(Assignee)

Comment 154

4 months ago
Created attachment 8998397 [details]
Bug 1455649 - DocumentL10n, part 4 - Switch Preferences to use DocumentL10n API.

With the new DocumentL10n API we can now use <linkset/> element to trigger
DOMLocalization resource list population and fetching.

MozReview-Commit-ID: Db6uxyjOx9m
(Assignee)

Comment 155

4 months ago
Created attachment 8998398 [details]
Bug 1455649 - DocumentL10n, part 3 - Plug DocumentL10n life cycle into DOM hooks.

Adds hooks to XULDocument, HTMLLinkElement etc. to trigger DocumentL10n lifecycle.

MozReview-Commit-ID: JpPSWHkl558
(Assignee)

Comment 156

4 months ago
Created attachment 8998399 [details]
Bug 1455649 - DocumentL10n, part 2 - Extend nsIDocument to use DocumentL10n.

Extend nsIDocument to store DocumentL10n instance for documents which
can be localized.

MozReview-Commit-ID: APXbRPHZxAg
(Assignee)

Comment 157

4 months ago
Created attachment 8998400 [details]
Bug 1455649 - DocumentL10n, part 1 - Add C++ DocumentL10n API.

DocumentL10n is a DOM C++ API which serves as a bridge between
nsIDocument and mozDOMLocalization APIs.

MozReview-Commit-ID: 8LfOR4Haqlu
(Assignee)

Comment 158

4 months ago
Created attachment 8998401 [details]
Bug 1455649 - DocumentL10n, part 2 - Extend nsIDocument to use DocumentL10n.

Extend nsIDocument to store DocumentL10n instance for documents which
can be localized.

MozReview-Commit-ID: APXbRPHZxAg
(Assignee)

Updated

4 months ago
Attachment #8995606 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8995607 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8995608 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8995609 - Attachment is obsolete: true
Attachment #8998399 - Attachment is obsolete: true
(Assignee)

Comment 159

4 months ago
With all dependencies landed and performance looking good I think this is ready for the final feedback round before review!

qDot - :bz pointed out that you may be able to take a look a the patchset and provide general feedback on my use of DOM API and how I hook into nsIDocument & friends.

I still have a couple more tests to add, but I believe the code to be documented, so if you see anything you'd like me to document more, please point it out.
Also, my main concern is about how the DocumentL10n/mDOMLocalization lifcycle fits into nsIDocument lifecycle. I'm not sure if I'm loading/unloading at the right places, and if the places I hook up the `LocalizationLinkeAdded/Removed` and `OnL10nResourceContainerParsed` and `OnDocumentParsed` are the correct ones to trigger localization work.

qDot, bz: There's one open question, in part 1 I have `DocumentL10n::TranslateRoots` which has a case where I have an already existing pending Promise, which some code might have `.then` into, and I need to make it resolve after the `mDOMLocalization::TranslateRoots` promise resolves. I don't know how to hook the promise to resolve another promise. Can you help?

I'm setting ni? because I don't know how to request feedback in phabricator yet.
Flags: needinfo?(kyle)
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 160

4 months ago
:smaug offered to step in for :bz and help us get this landed! :)
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
(Assignee)

Comment 161

4 months ago
:bz will actually have time to also review the patches!

One change I decided to make is to give up on trying to be overly smart with the `DocumentL10n::mReady` promise. I'll scale it down to what it does in the current model which means that the only thing it has to do is get initialized in constructor, and get resolved when the first `translateRoots` from `OnDocumentParsed` gets resolved.

I'll still need a help from DOM peers figuring out how to chain such promises.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 162

4 months ago
Ok, :bz, :smaug - the patchset has been updated to the latest round of feedback I got. It feels complete barring the promise chaining question for `mReady`.
With that, I'll be ready to set r?
gandalf, do you mind mailing me these diffs or attaching the raw diffs here?  I won't have access to Phabricator  until the end  of the month.
(Assignee)

Comment 164

4 months ago
Created attachment 9001332 [details] [diff] [review]
DocumentL10n, part 1 - Add C++ DocumentL10n API. (2018-08-15)
(Assignee)

Comment 165

4 months ago
Created attachment 9001333 [details] [diff] [review]
DocumentL10n, part 2 - Extend nsIDocument to use DocumentL10n. (2018-08-15)
(Assignee)

Comment 166

4 months ago
Created attachment 9001334 [details] [diff] [review]
DocumentL10n, part 3 - Plug DocumentL10n life cycle into DOM hooks. (2018-08-15)
(Assignee)

Comment 167

4 months ago
Created attachment 9001335 [details] [diff] [review]
DocumentL10n, part 4 - Switch Preferences to use DocumentL10n API. (2018-08-15)
Part 1 comments:

>+++ b/dom/webidl/DocumentL10n.webidl
>+dictionary L10nKey {
>+  object? args = null;

What would the actual structure of "args" look like?  Usually passing raw objects through webidl is a bit weird, especially if the callee is going to look at properties of the object, as here.  That usually means object Xrays, which can be a  bit surprising.

Is it possible to express args as a record<> type?  Or is the value space too complicated?

I assume there's a reasony we're allowing null args, not just missign ones?

>+  [NewObject] Promise<sequence<DOMString>> formatMessages(sequence<L10nKey> aKeys);
>+  [NewObject] Promise<sequence<DOMString>> formatValues(sequence<L10nKey> aKeys);
>+  [NewObject] Promise<DOMString> formatValue(DOMString aId, optional object aArgs);

These three need to be documented.

>+  [Throws] void setAttributes(Element aElement, DOMString aId, optional object aArgs);

Again, can that last arg be a record<> type?

>+   * Triggers translation of a DOM Fragment using the localization context.

This is actually translating a subtree rooted at aElement, not a DocumentFragment.  The comments and name suggest the latter...

Maybe calls this translateElement or translateElementTree (taking "Element aRoot" as an arg)?

>+   * Triggers translation of a list of Elements using the localization context.

Does this translate their descendants?  I assume not, right?  Might be good to make that clear.

>+   *    await document.l10n.translateFragment([elem1, elem2]);

translateElements?

>+++ b/intl/l10n/DocumentL10n.cpp
>+DocumentL10n::DocumentL10n(nsIDocument* aDocument)

This constructor can leave the object in a state where trying to use it would crah (i.e. if we take any of the early returns).  I would much rather we had a fallible Init() method and never returned these objects to script if they could not be initialized properly.

>+DocumentL10n::AddResourceIds(nsTArray<nsString>& aResourceIds)
>+  uint32_t ret;
>+  mDOMLocalization->AddResourceIds(aResourceIds,  &ret);
>+  return ret;

What if mDOMLocalization->AddResourceIds() fails and doesn't set the outparam?

>+DocumentL10n::RemoveResourceIds(nsTArray<nsString>& aResourceIds)
>+  if (!mDOMLocalization) {

If we do the Init() thing, this check can go away, right?

>+  uint32_t ret;
>+  mDOMLocalization->RemoveResourceIds(aResourceIds, &ret);
>+  return ret;

What if mDOMLocalization->RemoveResourceIds() fails?

>+DocumentL10n::FormatMessages(JSContext* aCx, const Sequence<L10nKey>& aKeys, ErrorResult& aRv)
>+      aRv.Throw(NS_ERROR_FAILURE);

aRv.NoteJSContextExeception(), since the exception is on aCx already.

Same thing in FormatValues.

+DocumentL10n::SetAttributes(JSContext* aCx, Element& aElement, const nsAString& aId, const Optional<JS::Handle<JSObject*>>& aArgs, ErrorResult& aRv)
>+      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

aRv.NoteJSContextExeception().

>+DocumentL10n::GetAttributes(JSContext* aCx, Element& aElement, L10nKey& aResult, ErrorResult& aRv)
>+{
>+    if (!JS_ParseJSON(aCx, l10nArgs.get(), l10nArgs.Length(), &json) ||
>+        !json.isObject()) {
>+      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

Really, it should be aRv.NoteJSContextExeception() when JS_ParseJSON fails and NS_ERROR_DOM_SYNTAX_ERR when the result isnot object...

>+DocumentL10n::OnDocumentParsed()
>+  //XXX: Here I need to make `mReady` resolve once `promise` resolves.

And mReady might already haveb been created before this point, because someone
can call the "ready" getter before this code runs, right?

I think your simplest option is to implement a PromiseNativeHandler that will
resolve mReady and AppendNativeHandler() it to the Promise that TranslateRoots returned.

>+DocumentL10n::GetReady(ErrorResult& aRv)
>+{
>+  return mReady.forget();

This doesn't seem right.  You want every get here to keep returning the same
PRomise*.  So just have the function return Promise* and do:

  return mReady;

There's no need to mark this one [Throws].

>+++ b/intl/l10n/DocumentL10n.h
>+ * Once initialized, DocumentL10n relies all API methods to an

s/relies/relays/

>+ * instance of mozIDOMLocalization and maintaines a single promise
>+ * which gets resolved every time the document gets retranslated.

I'm not sure I follow.  It can only get resolved once....

>+  uint32_t AddResourceIds(nsTArray<nsString>& aResourceIds);
>+  uint32_t RemoveResourceIds(nsTArray<nsString>& aResourceIds);

Need to be documented.
Flags: needinfo?(bzbarsky)
Part 2 comments:

>+++ b/dom/base/nsDocument.cpp
>+    mDocumentL10n(nullptr),

That's what RefPtr default-initializes to, so no need for this bit.

>+PrincipalAllowsL10n(nsIPrincipal* principal)
>+  if (NS_FAILED(rv) || (!isAbout)) {

Extra parens around !isAbout should go.

>+    return false;

OK, so after this point we know we have an about: URI, right?

>+  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_DANGEROUS_TO_LOAD, &isNonWeb);

And this is just trying to restrict to about: URIs that are not  web-exposed?

The comment says "all documents that are not exposed to the Web" but the
behavior is "system or non-web-exposed about:".  Presumably the comment is what
needs fixing?

>+nsIDocument::GetOrCreateLocalization()
>+    DocumentL10n* l10n = new DocumentL10n(this);
>+    if (l10n) {

Can't test true; global operator "new" never returns null in Gecko.

>+already_AddRefed<DocumentL10n>
>+nsIDocument::GetL10n(ErrorResult& aRv)
>+{
>+  if (!mDocumentL10n) {
>+    return nullptr;
>+  }
>+
>+  RefPtr<DocumentL10n> refL10n(mDocumentL10n);
>+  return refL10n.forget();

Why not just do:

  DocumentL10n*
  nsIDocument::GetL10n()
  {
    return mDocumentL10n;
  }

?

>+nsIDocument::LocalizationLinkAdded(Element* aLinkElement)
>+  if (!PrincipalAllowsL10n) {

That's testing the function pointer.  That test will always test false, because
the function pointer is non-null.

Maybe you want "if (!PrincipalAllowsL10n(NodePrincipal()))" or something?

>+nsIDocument::LocalizationLinkRemoved(Element* aLinkElement)
>+  if (!PrincipalAllowsL10n) {

Same issue.

>+  if (mDocumentL10n) {

And if not, shouldn't you remove it from mL10nResources?

>+nsIDocument::OnL10nResourceContainerParsed()
>+    DocumentL10n* l10n = GetOrCreateLocalization();
>+    if (l10n) {

GetOrCreateLocalization() never returns null.  Maybe we shoiuld just call it
EnsureLocalization() or something?

>+++ b/dom/base/nsIDocument.h
>+  void LocalizationLinkAdded(Element* aLinkElement);
>+  void LocalizationLinkRemoved(Element* aLinkElement);
>+  mozilla::dom::DocumentL10n* GetOrCreateLocalization();
>+  void OnL10nResourceContainerParsed();
>+  void OnDocumentParsed();

All those need documentation.  Should they all be public like this patch makes
them?

Even if they should, they should not be in the middle of the webidl bits on
nsIDocument like this, since these are not webidl methods.

>+  already_AddRefed<mozilla::dom::DocumentL10n> GetL10n(ErrorResult& aRv);

This should probably be after documentFlashClassification, to kinda match the
IDL order.

>+++ b/dom/webidl/Document.webidl
>+  [Throws, Func="nsDocument::DocumentSupportsL10n"] readonly attribute DocumentL10n? l10n;

Shouldn't be [Throws].
Part 3 comments:

>+++ b/dom/html/HTMLLinkElement.cpp
>+  if (aDocument && this->AttrValueIs(kNameSpaceID_None, nsGkAtoms::rel, nsGkAtoms::localization, eIgnoreCase)) {
>+    aDocument->LocalizationLinkAdded(this);
...
>+  if (oldDoc && this->AttrValueIs(kNameSpaceID_None, nsGkAtoms::rel, nsGkAtoms::localization, eIgnoreCase)) {

OK, but what happens if someone changes "rel" from "localization" to something
else, or vice versa?  Seems like you need to add/remove in that case too.

In general, this is all looking reasonable.  I did not carefully read the tests in part 3 or most of part 4, fwiw.
Blocks: 1476453
Blocks: 1438748
(Assignee)

Comment 171

4 months ago
Updated the patchset. It is now complete, albeit with several `XXX` comments based on :smaug's and :bz's questions that I wasn't able to answer myself.

I noticed a small performance regression in the latest try and a possible FOUC in HTML path which I'd like to debug before requesting r?.

My hope is to set r?smaug on the first three patches by EOD tomorrow.
Flags: needinfo?(bugs)
(Assignee)

Updated

4 months ago
Attachment #9001335 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #9001334 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #9001333 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #9001332 - Attachment is obsolete: true
(Assignee)

Comment 172

4 months ago
Olli: setting this to r? for you. There's a small perf regression on Preferences with this patchset [0] which I have trouble finding a way to debug. My hope is that in the review process we'll get to better fit the places where l10n is triggered to optimize that back.


[0] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7bfe96a412ae&newProject=try&newRevision=e7fb0351d5ff8e74bf670f1b12bd33a2fa2b733b&framework=1
(Assignee)

Comment 173

4 months ago
Here are profiles from loading Preferences in m-c vs the patchset:

m-c: https://perfht.ml/2BGuxyZ
patchset: https://perfht.ml/2BHO2qW
(Assignee)

Comment 174

4 months ago
updated link to the m-c profile using custom build, rather than downloaded Nightly: https://perfht.ml/2BGvFCD
(Assignee)

Comment 175

4 months ago
Summary of the current status:

We have the patches basically ready for landing, but we also have a performance regression on Preferences which prevents us from doing that.

Olli spent time investigating the profiles and identified two possible improvements:

1) LinkHandler is executed whenever <html:link> is added [0] and the patchset introduces a bunch of them
2) We execute the I/O trigger (OnL10nResourceContainerParsed) earlier than we do in the JS scenario ([1] vs [2])

I built 4 builds with profiles and try runs to see where it puts us:
(a) mozilla-central
(b) mozilla-central + patchset from this bug
(c) mozilla-central + patchset + removed LinkHandler events
(d) mozilla-central + patchset + removed LinkHandler events + Dispatch triggering OnL10nResourceContainerParsed


=== Results ===

(talos results for linux64)

+--------------+--------------+--------+-------------+---------------------------+
|    Build     |   Revision   | Talos  | Talos Delta |          Profile           |
+--------------+--------------+--------+-------------+---------------------------+
| m-c          | fb2862ae812e | 158.86 |             | https://perfht.ml/2MWKs0l |
| patchset     | 2289643c9b7c | 164.15 | 3.33%       | https://perfht.ml/2MUKmXc |
| -linkhandler | 7bfc70383dd4 | 162.23 | 2.13%       | https://perfht.ml/2BTJjmg |
| dispatch     | c0eb9f5585e2 | 162.98 | 2.59%       | https://perfht.ml/2Nrw0e5 |
+--------------+--------------+--------+-------------+---------------------------+

It seems that removing the link handler does impact the performance and reduces the hit, but Dispatch does not.

Here are talos perf-herder comparisons:

(a) vs (b) - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=2289643c9b7c&framework=1
(a) vs (c) - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=7bfc70383dd4&framework=1
(a) vs (d) - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=c0eb9f5585e2&framework=1

I'm currently stuck not sure what to do next. The performance hit is noticable, and may or may not prevent us from landing the patchset which is required to unblock a lot of other work and an upcoming Student project of migrating Firefox to Fluent.

Even if we decide to land with the regression, it likely pushes us further away from landing Fluent on the startup path, while we were hoping to use this patchset to get us closer to that goal.

Bz, Olli, Jared - thoughts?

[0] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/nsBrowserGlue.js#153
[1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/intl/l10n/l10n.js#45-50
[2] https://hg.mozilla.org/try/file/0a9214cd2997aafe7c9543745867bb3992b60a5d/dom/xul/XULDocument.cpp#l1048
Flags: needinfo?(jaws)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
When you say not using linkhandler, do you mean just removing some event listener, but still using html:link?
Flags: needinfo?(bugs)
(Assignee)

Comment 177

4 months ago
> When you say not using linkhandler, do you mean just removing some event listener, but still using html:link?

Yes, that is the case.

I also tested replacing HTML link elements with XUL link elements:


m-c vs that: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=f250d04e784d&framework=1

profile: https://perfht.ml/2NuDeOe

There doesn't seem to be any improvement over just disabling the LinkHandler. The talos regression stays within 2.5%.

Comment 178

4 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #177)
> There doesn't seem to be any improvement over just disabling the
> LinkHandler. The talos regression stays within 2.5%.

So I'm a bit confused - comment 175 suggests that the cause of the perf regression is the link handler and the fact that IO happens earlier than before. However, you then ran trypushes that try to nullify those effects, and you're still seeing a 2.5% regression. Doesn't that mean that there must still be some other cause, too? Have you looked at what else is happening here? Have you tried bisecting the 4 patches (e.g. do you see an impact after landing just part 1/2/3 without part 4, or perhaps there are further bits in the patches you could try to isolate the perf problem to?)

Ultimately it feels like the outcome for prefs here (in terms of having localized strings) isn't changing (ie we don't actually *need* to do more work all of a sudden), so it should be possible for this to land without a significant regression...


While we could obviously also unblock this in the immediate term by finding something else to optimize about the prefs loading, that doesn't help us for the browser startup / main window / other pages / long-term side of things...
(Assignee)

Comment 179

4 months ago
> So I'm a bit confused - comment 175 suggests that the cause of the perf regression is the link handler and the fact that IO happens earlier than before.

The LinkHandler was identified by smaug as a potential contributor to the regression.

> However, you then ran trypushes that try to nullify those effects, and you're still seeing a 2.5% regression.

I tested this hypothesis and confirmed that it contributes, but fixing that part does not nullify the regression which means that there must be other reasons.

> Doesn't that mean that there must still be some other cause, too?

Yes :( And we don't know what.

> Have you tried bisecting the 4 patches (e.g. do you see an impact after landing just part 1/2/3 without part 4, or perhaps there are further bits in the patches you could try to isolate the perf problem to?)

I don't know how I would do that - patch 1 is not going to impact anything because it's not plugged anywhere. patch2 exposes `document.l10n` which already prevents Preferences from working, but doesn't plug the lifecycle into parser lifecycle, so it just will not work. part 3 plugs lifecycle but doesn't switch the callsites to use `<linkset/>` etc. part 4 does the last part, so only in conjunction we are able to test the performance.

> Ultimately it feels like the outcome for prefs here (in terms of having localized strings) isn't changing (ie we don't actually *need* to do more work all of a sudden), so it should be possible for this to land without a significant regression...

Agree. We all believe that the performance should be on par or better, with strong hope to end up better because we have more control over where and how we plug things and we can skip more JS reflections.

> While we could obviously also unblock this in the immediate term by finding something else to optimize about the prefs loading, that doesn't help us for the browser startup / main window / other pages / long-term side of things...

Agree. But we also are running out of time, this patch blocks a lot, and we do not have at the moment any actionable idea on what to do next. :(

That's why I'm setting NI on bz/smaug/jaws to make a decision on what should be the next step.
Flags: needinfo?(bugs)
The next step should be figuring out why the performance regression happens.

The initial patch used html:link, and that is causing quite some DOMLinkAdded calls and event handling.
Another thing which is different to m-c is the timing of AddResourceIds call. The Dispatch approach tries to make it a bit closer to the current setup, but that is still just one runnable, when the script compilation/execution may have taken more time. (Just for debugging purposes, it might be interesting to know how it would affect to page load times if AddResourceIds call was postponed a bit more using a timer)

I haven't noticed other obvious differences. I think need to have several profiles with and without patches and look at them all.
Flags: needinfo?(bugs)
Comment on attachment 8998397 [details]
Bug 1455649 - DocumentL10n, part 4 - Switch Preferences to use DocumentL10n API.

Olli Pettay [:smaug] has approved the revision.
Attachment #8998397 - Flags: review+
(Assignee)

Comment 182

4 months ago
Olli asked me to produce 5x profiles per mode.

I amended my patches to switch the <html:link/> to XUL's <link/> which leaves us with three builds:

(a) mozilla-central
(b) mozilla-central + patchset
(c) mozilla-central + patchset + dispatch

Try runs:
(a) https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb2862ae812e
(b) https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d70d3970290
(c) https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bd8462c4e7d

I collected profiles for (a) and (c):

(a):
 - https://perfht.ml/2wwARU5
 - https://perfht.ml/2MWBoZH
 - https://perfht.ml/2wAY4oa
 - https://perfht.ml/2N2pMEn
 - https://perfht.ml/2wsyIZq

(c):
- https://perfht.ml/2N27S4v
- https://perfht.ml/2LDIVYz
- https://perfht.ml/2N29uv5
- https://perfht.ml/2LE9Z9W
- https://perfht.ml/2MXchWA

perfherder (a) vs (c) - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=1bd8462c4e7d&framework=1

It still shows 2-4ms regression, which shows up as significant for some jobs (Linux, Windows).

Olli - is that a good material for profiling?
Flags: needinfo?(bugs)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #175)
> Summary of the current status:
> 
> We have the patches basically ready for landing, but we also have a
> performance regression on Preferences which prevents us from doing that.
> 
> Olli spent time investigating the profiles and identified two possible
> improvements:
> 
> 1) LinkHandler is executed whenever <html:link> is added [0] and the
> patchset introduces a bunch of them

That doesn't seem super relevant. The most expensive part of the LinkHandler callbacks is loading FaviconLoader.jsm, which happens because of the favicon link, which exists regardless of these patches.

Then again, the .split() calls on the rel attribute add up to something annoyingly non-trivial. It shows up as 3ms in that profile, so maybe it's as much as 6ms in real life. I really hate that we have to do that when we already have a relList attribute which should be much cheaper to access, but it unfortunately filters the values that it returns, and doesn't include most of the ones we need...
(Assignee)

Comment 185

4 months ago
Olli was able to identify the source of the remaining regression:

```
nsINode::Localize uses several promises, and LocalizationHandler gets resolved later with patches, since it is created when we aren't in microtask. Without patches it is resolved during MozBeforeInitialXULLayout event dispatch, with patches when PresShell::Initialize is called (and it ends up somewhat accidentally trigger microtask while executing xbl constructors)
and this causes extra frameconstruction. (the profiles with patches have one extra frameconstruction step soon after nsINode::Localize)
```

I modified the patchset to perform the `OnDocumentParsed` within `MozBeforeInnitialXULLayout` microtask and we not only removed the performance hit, but even got a nice 1-3ms perf win!

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=fc0438952ed0&framework=1

I updated the patches and now am fixing remaining tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eb8f139aed6ddbb9cbf811e1311667ccb49b85e&selectedJob=196799490
Flags: needinfo?(jaws)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
(Assignee)

Comment 186

4 months ago
Had to fix one test and switch `translateFragment` to take `nsINode` instead of an `Element` so that it handles `DocumentFragment` properly.

I think it's ok since we use `nsINode.localize` inside it. I will want to change the name to sth like `translateNode`, but in order to minimize the patchset, I'd prefer to not touch `DOMLocalization.jsm` and all callsites that use `translateFragment` now here, and do it in the follow up.

Here's a new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17d89ac3be90f7340740c5b73b651b3c617a7737
(Assignee)

Comment 187

3 months ago
I was able to fix all test bugs and added one more test to verify that `document.l10n.ready` fails if the document cannot be localized due to missing resources.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1509be0214931b49b80b3e21919ca697a14a81f

The talos comparison against master is here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fb2862ae812e&newProject=try&newRevision=cd684fa2e1b2&framework=1

Olli: I think this is ready for the final review :)
Comment on attachment 8998401 [details]
Bug 1455649 - DocumentL10n, part 2 - Extend nsIDocument to use DocumentL10n.

Olli Pettay [:smaug] has approved the revision.
Attachment #8998401 - Flags: review+
Comment on attachment 8998398 [details]
Bug 1455649 - DocumentL10n, part 3 - Plug DocumentL10n life cycle into DOM hooks.

Olli Pettay [:smaug] has approved the revision.
Attachment #8998398 - Flags: review+
(Assignee)

Comment 190

3 months ago
Created attachment 9006724 [details]
Bug 1455649 - DocumentL10n, part 0 - Add `eager` parameter to mozDOMLocalization::AddResources to trigger I/O early. r=mossop
(Assignee)

Comment 191

3 months ago
I updated the patches and rebased on them on top of today's m-c. The patch I landed yesterday in bug 1483038 required an update to mozDOMLocalization to reintroduce the eager I/O which we did with l10n.js, but via mozIDOMLocalization.

I also updated tests and added a new test as per smaug's request (testing iframe in unprivileged).

Perf remains good: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=832ed50f0a39&newProject=try&newRevision=af2797f67822bac705af417a19d35bcd1258ca1c&framework=1
(Assignee)

Comment 192

3 months ago
Ugh, during final testing we noticed that this patchset will require further refinements to work in non-system-principal scenario because the Promises returned from system principal cannot be passed to non-system one.

We decided to finish the patch as is, because it already enables Fluent in more scenarios and it improves performance, and file a follow up to work on aboutRobots case.
(Assignee)

Updated

3 months ago
Blocks: 1488973
Attachment #9006724 - Attachment description: Bug 1455649 - DocumentL10n, part 0 - Add `eager` parameter to mozDOMLocalization::AddResources to trigger I/O early. r=smaug → Bug 1455649 - DocumentL10n, part 0 - Add `eager` parameter to mozDOMLocalization::AddResources to trigger I/O early. r=mossop
Comment on attachment 8998400 [details]
Bug 1455649 - DocumentL10n, part 1 - Add C++ DocumentL10n API.

Olli Pettay [:smaug] has approved the revision.
Attachment #8998400 - Flags: review+
(Assignee)

Updated

3 months ago
Blocks: 1489283
(Reporter)

Comment 194

3 months ago
Comment on attachment 9006724 [details]
Bug 1455649 - DocumentL10n, part 0 - Add `eager` parameter to mozDOMLocalization::AddResources to trigger I/O early. r=mossop

Dave Townsend [:mossop] (he/him) has approved the revision.
Attachment #9006724 - Flags: review+

Comment 196

3 months ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aeb86c8f7ca
DocumentL10n, part 0 - Add `eager` parameter to mozDOMLocalization::AddResources to trigger I/O early. r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/f451b9310466
DocumentL10n, part 1 - Add C++ DocumentL10n API. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e522c31a0306
DocumentL10n, part 2 - Extend nsIDocument to use DocumentL10n. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0728ae9f89
DocumentL10n, part 3 - Plug DocumentL10n life cycle into DOM hooks. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/709a197ccc2e
DocumentL10n, part 4 - Switch Preferences to use DocumentL10n API. r=smaug
Blocks: 1490457
Blocks: 1491549
You need to log in before you can comment on or make changes to this bug.