Once localization has completed for a XUL document update the prototype with
the new content and attributes before it gets serialized for later loads to
use.

Bypass loading the l10n objects if the prototype for the XUL document already
has the l10n data in it.

Given that this changes localize() API, gandalf should take a look at first.

It would be nice if we didn't change that API, IMO. Currently the API is very clean, but the patch injects a small, currently very XUL specific, argument.

I'm almost done with adjustments to Dave's patch. I experimented a lot around it but ended up with two changes:

1. I removed the firstRun param to minimize the changes to the Fluent side. We now use OwnerDoc()->GetDocumentElement()->IsSameNode() to recognize that we're calling Node::localize on a document element and cache it.
2. I separated out ConnectRoot+RegisterObservers from GetDOMLocalization because we need to do both of those things even when we load from cache for the live translation to work. So now the cache only skips the initial translateRoots because it doesn't need it anymore.

I'm testing performance of the patch and will post it for feedback from :Mossop tomorrow.

Hmm, ok, so the (2) brings up a challenge.

In order for us to handle runtime translation changes, we need to initialize DOMLocalization and register the root in it to set up the MutationObserver.

But that means that even with the cache we have to initialize DOMLocalization during the document's bootstrap (so that bootstrap JS code can alter DOM and that will result in localization getting updated).

The result is that we're again seeing a performance hit: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=04091e4ba2532d09868504c151f3f523303fc132&framework=1&selectedTimeRange=172800

It's a smaller hit than with no cache, because we are skipping the initial translation, but it's still a hit.

We have two ways to workaround it:

1. Declare that we won't handle runtime localization in browser.xul temporarily
2. Migrate the MutationObserver to C++.

The latter would require us to also migrate DOM Overlays to C++, which is something I want to do anyway. That would allow us to plug full DOM Overlays to this cache as well, instead of doing a simplified translation that we do now.

I'm torn on the direction here. Migrating DOM Overlays and the MutationObserver to C++ will take time, but I'm concern about introducing a half-working Fluent solution that likely introduces a lot of quirky behaviors and potential papercuts between cached and non-cached scenarios on the same document. :/

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)

2. I separated out ConnectRoot+RegisterObservers from GetDOMLocalization because we need to do both of those things even when we load from cache for the live translation to work. So now the cache only skips the initial translateRoots because it doesn't need it anymore.

I'm not sure why this is necessary. What kind of live translation are you referring to?

I'm not sure why this is necessary. What kind of live translation are you referring to?

FluentDOM has two parts: initial translation, and runtime translation.

The former is what we focused so far, but we call connectRoot so that the instance of DOMLocalization can monitor the document for changes (via MutationObserver) and monitor the environment for events (like intl:app-locales-change) and retranslate the items.

You placed GetDOMLocalization on setAttributes to instantiate the class when user sets localization of an element, but this method is just a helper. Developer can insert a DOMFragment with data-l10n-ids, or call element.setAttribute("data-l10n-id", id) or an event with locales change may be fired.

If we don't connectRoot in the cached route, but we do call it in the non-cached, we create an inconsistency in behavior depending on the cache.

I'm afraid that we do need to move MutationObserver+Observers + DOMOverlays to C++ if we want to handle that.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)

The former is what we focused so far, but we call connectRoot so that the instance of DOMLocalization can monitor the document for changes (via MutationObserver) and monitor the environment for events (like intl:app-locales-change) and retranslate the items.

You placed GetDOMLocalization on setAttributes to instantiate the class when user sets localization of an element, but this method is just a helper. Developer can insert a DOMFragment with data-l10n-ids, or call element.setAttribute("data-l10n-id", id) or an event with locales change may be fired.

I was assuming we'd just make using l10n.setAttributes compulsory for making or modifying an element's localization. We can trigger l10n connection when the locale changes at runtime pretty easily I think.

There are so many ways to modify DOM that relying on setAttributes, which is just a helper, seems like a likely source of intermittent bugs.

I'm currently testing an approach where one can set 'data-l10n-cache' attribute on the document element and then we'd skip connecting the root.

I'd move the observers to c++ and for such document we would handle initial DOM translation, observer changes and lazily formatValues/translateFragment (which would technically enable people to localize elements they inject without Mutation observer)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)

There are so many ways to modify DOM that relying on setAttributes, which is just a helper, seems like a likely source of intermittent bugs.

Why would they be intermittent?

Because the behavior is different depending on if the document is cached or not. If you inject a document fragment with l10n nodes or even just do elem.setAttribute('data-l20n-id', 'foo') you will get different behavior.

Dave, I ended up applying some changes on top of your patch and wanted to run them through you for feedback.

This patch builds on top of bug 1532226 and yours, and it reduces the surface of changes to DOMLocalization.
It also introduces a concept of data-l10n-cache which makes us not connect MutatonObserver on documents with this attribute. The only way to provide "live" translation is to use translateElements or translateFragment manually.

If you'll agree with the adjustments, I'll work on the final patch and tests.

Hmm, I'm seeing some performance regression when I put this patch with browser-menubar switch to Fluent:

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=841356eae08e456bffedce94f82d98211b06874f
• https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=841356eae08e456bffedce94f82d98211b06874f&framework=1&selectedTimeRange=172800

I assume the concent-process is a fluke, but the ts_paint and sessionrestore ones are consistent across my try runs both with my alternations to the patch and without. :( Dave - would you be able to take a look at it?

We also need to refactor the patches on top of bug 1527977.

When creating a prototype document from the parser we cache any elements that
have datal10nid attributes. Then during the fluent pass we apply the changes to
the elements to the prototype as well and finally set a flag on the prototype
saying that its localization is already part of the prototype elements.

Because the fluent pass happens before completing the content sink this means
that the l10n info is applied before the prototype is written to disk and handed
out to any waiting document loads.

Because the l10n info is applied directly to the prototype no additional work
needs to happen when the prototype is subsequently loaded.

Depends on D34827

Depends on D34828

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd2c08772a8bb53f9696dd7d5a74a4a632a22e14

new try, now with less orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c265aaeb8699ae2ced5abd6acd8540188aee93a2

We've made some changes based on the observed crashes.

The major one is the change in how we approach caching in DOMLocalization::TranslateElements.

We now collect the localizable nodes and separate out the ones that are coming from the prototype, from the ones injected via script.
We then localize the ones from prototype only if the prototype was not localized, and we localize the ones from script irrelevant of the prototype status.

The exact algorithm is described in the CPP.

The result is a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99a0a50586c5cd4e824da6c1528f7d373c580b73&selectedJob=251973811

I'm going to re-request review on the last two patches due to changes.

Hmm, this approach does not work :(

When the document is loaded from cache, we don't build mL10nElements at all, and in result with my latest patch all elements from the document get translated again.

==== Summary of the problem ====

The "cold" path should look like this:

1. We build a prototype, collecting localizable prototype elements into mL10nElements array.
2. We then trigger initial translation, and as we translate elements, we update their prototype to reflect translated versions of the elements
3. Prototype gets saved and marked as l10nWasCached.

The "hot" path is:

1. We load from prototype
2. We then trigger initial translaton, and seeing that the prototype has l10nWasCached we skip localizing it.

Unfortunately, there is JS code that runs between (1) and (2) in the "cold" path, which injects new localizable elements.
Scripts, according to :bdahl, runs in the "second pass" of the XUL prototype content sink.

The result is that in the "cold" path we end up with elements to localize that are not in the prototype. We can still localize them, and skip storing them in the prototype, but then in the "hot" path we will skip localizing them.

My attempt above was to recognize which elements are in the document but not in the prototype at both "hot" and "cold" (2), and always translate them, while the ones that are in prototype only translate in "cold" (and cache).

The problem with that was that in the "hot" path we don't accumulate "mL10nElements" so we don't know which elements were in the prototype and in result in the "hot" path we end up localizing everything as if it was injected from the script.

We need to either find a way to store localizable element ids for each prototype, or, preferably, find other ways to make initial localization not be affected by elements injected from script.

Mossop, smaug - any ideas on how to approach this?

I want to know if we can make sure that scripts don't run between (1) and (2) instead of trying to work around that. I seem to remember we made the assumption that we could do this before JS ran when designing this system.

It also doesn't make sense to me that they would be running before the prototype is saved (at least, scripts running via <script> tag in the document). Which scripts exactly are running again? Are they loaded via a <script> tag in the document, or are they maybe running outside of the window scope (like a JSM) and reacting to an observer notification? If the latter, perhaps we could change when we fire said notification or something like that.

I want to know if we can make sure that scripts don't run between (1) and (2) instead of trying to work around that.

I see two ways to get there:

1. Stop executing scripts before Document::TriggerInitialDocumentTranslation
2. Move Document::TriggerInitialDocumentTranslation to be fired earlier

:bdahl, adding you to NI for your thoughts.

The consensus from the meeting (attendees: kmag, bz, smaug, mossop, bdahl, zbraniecki) is to follow the patch we have and add a code path that adds the element to L10nMutations::mPendingElements if we're loading from cache and:

1. The element gets injected not from parser
2. The element's data-l10n-id or data-l10n-args is set not from the parser

:smaug said he'll point me to the right places for both.

I have the patchset working, but I noticed a problem.

In the menubar we have a case where two UI items share the same translation [0]. It makes the prototype cache mL10nElements not work since they cache the key by l10n-id, which means it has to be unique per prototype.

We have been advising people to use unique l10n-ids, but we never enforced it. I can either enforce it via the patch and fix all cases where people already did it, or we need a different key for the hashmap.

:smaug, :mossop - what's better?

[0] https://searchfox.org/mozilla-central/source/browser/base/content/browser-menubar.inc#471-482

I'm not sure we should enforce uniqueness of l10n keys at that level. Say, you have two entry points to the same menu popup in the tree. It's the same popup, I would consider it to be fair game to just use the same localized strings.

We also have list items, which get the same l10n-ids for each item.

I agree.

Hmm, so we need element(with l10nid attr)->protoelement hashtable ?
PrototypeDocumentContentSink::CreateElementFromPrototype could do that.

(In reply to Axel Hecht [:Pike] from comment #27)

I'm not sure we should enforce uniqueness of l10n keys at that level. Say, you have two entry points to the same menu popup in the tree. It's the same popup, I would consider it to be fair game to just use the same localized strings.

But wouldn't those then be in different contexts? My understanding previously has been that in that case we need different strings (even if in most languages they end up being identical).

Hmm, so we need element(with l10nid attr)->protoelement hashtable ?

I assume we just need a hashtable of element->protoelement.

(In reply to Dave Townsend [:mossop] (he/him) from comment #29)

(In reply to Axel Hecht [:Pike] from comment #27)

I'm not sure we should enforce uniqueness of l10n keys at that level. Say, you have two entry points to the same menu popup in the tree. It's the same popup, I would consider it to be fair game to just use the same localized strings.

But wouldn't those then be in different contexts? My understanding previously has been that in that case we need different strings (even if in most languages they end up being identical).

For list items, they'd be surely in the same context.

For sub menus, I'd think they're also in the same context. I don't have a concrete example, though.

I think the decision here is that in the case we have right now, we can probably enforce unique l10n IDs. Even if we're just aliasing the doubles by adding different IDs and adding message references in the ftl.

But that limits the UX patterns that can be used with an active mini cache, notably excluding any list items.

Depends on D34827

Depends on D38970

Depends on D38971

New push. This seems to do what we wanted. There's a lot of logging in there now and it shows that it correctly caches all 87 elements of the menubar!

When looking at Preferences, the outcome is a bit more complex.

1. We load 324 l10n elements from prototype
2. But only 23 of them are in the body, so those are the ones we actually translate. The rest are in <template/> so we only translate them when they get injected into body.
3. For some reason, SetIsL10nCached doesn't work in Preferences. It gets called and on the next load of Preferences it WasL10nCached returns false. Either in the same session or after a restart.

I'm slightly concerned about (3) as it may indicate that we're doing something wrong when caching prototype for Preferences, but that doesn't affect my work here as I'm pushing for the menubar.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e8cc9be849190a3369564a7209438aa2b3c486

perfherder (with menubar patch) - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f0825acd35da9de368196b5dce544b7c15178ff4&newProject=try&newRevision=c43cb3affef0a4e638fc4a2d292bd8feb69c23d6

I'm hunting down a leak that is now visible in debug build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e074675a9ae8b654754f6cf27f8bc85566440e7b&selectedJob=258345904

@bdahl suggested that maybe my:

nsXULPrototypeDocument* proto = mDocument->GetPrototype();

leaks, so I tried replacing it with RefPtr but no impact:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3855ec3508c26846a01f02cd0b65e1c7aa760d9a&selectedJob=258387296

so, we learned that the issue is that we don't traverse the keys of a hashtable, so I added it manually.

Let's see if the try shows green - https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a534eecb81b0ad4b2bb119ef92c54daed5856d

premature landing, but the try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0be2bbb97f300d76c1f715766c0d1989e902adfc

Gonna land with latest fixes (in try) after backout.

Backout link: https://hg.mozilla.org/integration/autoland/rev/7efa66d980152f09b30322a75d7a2e8b5671385b

https://hg.mozilla.org/mozilla-central/rev/ead64fbc603d
https://hg.mozilla.org/mozilla-central/rev/0abab753a8fe
https://hg.mozilla.org/mozilla-central/rev/a8aa836d6df7
https://hg.mozilla.org/mozilla-central/rev/69084de36a8d
https://hg.mozilla.org/mozilla-central/rev/743b76c2c4fe