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.

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.

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 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.

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.

(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.

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

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.

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.

(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?

> 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.

(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 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.

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.

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.

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.

(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.

(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.

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.

Updated talos against the latest patch from :mossop: https://pike.github.io/talos-compare/?revision=f7a7f51df5e9&revision=21ce2c05a50e&revision=a748ed2a0da7

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.

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?

Kicked off another talos with debug builds as well - https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc0510f7a7ee842752270fad7ec1ce983555d4fe

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.

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).

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.

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 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?

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 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.

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

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.

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.

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 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.

(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?

> 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 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.

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.

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 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 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 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 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 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.

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.

Updated to :stas', :bz's and :reviewbot's comments. Thank you all! Pending Mossop's feedback and help with the "XXX(Mossop)"'s.

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

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 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.

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?

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.

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?

Looks generally sane, I can look in more detail at the missing bits next week.

(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)" :)

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));

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.

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.

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.

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

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...

(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.

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]

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 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]

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

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.

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.

The new set of patches finally got a green talos - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5e22651213b1&newProject=try&newRevision=3fd432ae6b8f&framework=1

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.

With the new DocumentL10n API we can now use <linkset/> element to trigger DOMLocalization resource list population and fetching. MozReview-Commit-ID: Db6uxyjOx9m

Adds hooks to XULDocument, HTMLLinkElement etc. to trigger DocumentL10n lifecycle. MozReview-Commit-ID: JpPSWHkl558

Extend nsIDocument to store DocumentL10n instance for documents which can be localized. MozReview-Commit-ID: APXbRPHZxAg

DocumentL10n is a DOM C++ API which serves as a bridge between nsIDocument and mozDOMLocalization APIs. MozReview-Commit-ID: 8LfOR4Haqlu

Extend nsIDocument to store DocumentL10n instance for documents which can be localized. MozReview-Commit-ID: APXbRPHZxAg

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.

:smaug offered to step in for :bz and help us get this landed! :)