Closed Bug 1627809 Opened 5 years ago Closed 5 years ago

Clean up Localization initialization

Categories

(Core :: Internationalization, task, P1)

Product:
Core
Component:
Internationalization
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla77
Tracking Flags:
Tracking Status
firefox77 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(8 files, 2 obsolete files)

Bug 1627809 - Rename *DocumentTranslation to *Translation.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Remove unncessary eager attribute from Localization API.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Move Localization.jsm construction to Localization constructor.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Rename Document::TriggerInitialTranslation to Document::OnParsingCompleted.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Add Localization::AddResourceId and Localization::RemoveResourceId for DOM to use.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Move DocumentL10n construction earlier.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Make DocumentL10n translate all roots and add ConnectRoot with initial translation.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1627809 - Make the DocumentL10n translate the document when initialized lazily.
47 bytes, text/x-phabricator-request
Details | Review

As a follow-up to bug 1621674 and before bug 1613705, I'd like to clean up the initialization of the Localization class (and DOMLocalization and DocumentL10n) in order to enable better integration with Web Components.

In particular, we currently have a gap in our coverage where if the document doesn't have l10n links, but web component injects one, it's possible that document.l10n won't be enabled until DOMContentLoaded.

In the new model, I'd like to make it clear that construction of Localization doesn't "enable" it. There's a separate Init method that has to be called and only then translateMessages, translateValue and translateValues becomes operational.

This means that right after initial link injection document.l10n becomes available, but only connectRoot, disconnectRoot etc. can be called on it. Any attempt to translateFragment or translateElements or translateRoots will fail.

Next, when initial l10n container is parsed (</linkset> or </head>), we call Init and then all methods are available, but the initial translateRoots happens only after parsing of the document completes.

If the link is injected after that moment, it'll skip the initial translation completely.

In order to make that logic work, DocumentL10n gets a method overload - connectRoot has a second param, which when set to true means "perform initial translation as soon as possible".

Assignee: nobody → gandalf
Blocks: 1613705, 1491549
Status: NEW → ASSIGNED
Depends on: 1621674
Priority: -- → P1

With the following patches I was able to apply this:

diff --git a/browser/components/migration/content/migration.xhtml b/browser/components/migration/content/migration.xhtml
--- a/browser/components/migration/content/migration.xhtml
+++ b/browser/components/migration/content/migration.xhtml
@@ -11,19 +11,16 @@
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
         xmlns:html="http://www.w3.org/1999/xhtml"
         windowtype="Browser:MigrationWizard"
         title="&migrationWizard.title;"
         onload="MigrationWizard.init()"
         onunload="MigrationWizard.uninit()"
         style="width: 40em;"
         buttons="accept,cancel">
-<linkset>
-  <html:link rel="localization" href="toolkit/global/wizard.ftl"/>
-</linkset>
 
 <script src="chrome://global/content/customElements.js"/>
 <script src="chrome://browser/content/migration/migration.js"/>
 
 <wizard data-branded="true">
   <wizardpage id="importSource" pageid="importSource" next="selectProfile"
               label="&importSource.title;">
 #ifdef XP_WIN
diff --git a/toolkit/content/widgets/wizard.js b/toolkit/content/widgets/wizard.js
--- a/toolkit/content/widgets/wizard.js
+++ b/toolkit/content/widgets/wizard.js
@@ -520,16 +520,17 @@
   class MozWizardButtons extends MozXULElement {
     connectedCallback() {
       this._wizard = this.getRootNode().host;
 
       this.textContent = "";
       this.appendChild(MozXULElement.parseXULToFragment(this._markup));
 
       MozXULElement.insertFTLIfNeeded("toolkit/global/wizard.ftl");
+      document.l10n.connectRoot(this, true);
 
       this._wizardButtonDeck = this.querySelector(".wizard-next-deck");
 
       this.initializeAttributeInheritance();
 
       const listeners = [
         ["back", () => this._wizard.rewind()],
         ["next", () => this._wizard.advance()],

and it just works. The reasons why, are:

  1. insertFTLIfNeeded now synchronously enables document.l10n.
  2. DocumentL10n::ConnectRoot overrides DOMLocalization:ConnectRoot and adds a second argument which triggers the "translate when ready" mode.

The (2) is something that I need to be able to trigger this callback at any point - either while the document is still parsing (and then the root will be translated together with other roots later) or after the initial translation completed (in which case I trigger the translation of the new root immediately).

It's going to be quite tricky to get this reviewed as the knowledge o the components is fairly thinly spread between Smaug, Mossop and Stas.

I'll start with the big picture summary of the goals (read comment 0 for more details):

Part 1 (Localization):

*) Separate construction from initialization for DocumentL10n to allow for mRoots/MutationObserver/resourceIds to be adjusted before the initial translation. Notice, that this does not affect manually constructed new Localization or new DOMLocalization, just ones constructed from DocumentL10n.
*) Narrow down the XPIDL interface for Localization.jsm - this is the second (after bug 1621674) step to get us to remove the JSM file. I kept the core logic there to minimize the patch size, and will handle the final move later.
*) Clean up the eager flag which can be only set by DocumentL10n, and custom created DOMLocalization or Localization don't have access to it.
*) Separate out AddResourceIds as the only place where ids are added. Thanks to separation of the constructor from init resources that are meant to be loaded in initialization phase can be now added via regular AddResourceIds.

Part 2 (DocumentL10n):
*) Enable document.l10n after the first link is loaded, not after l10n links container is parsed
*) Remove the mPendingInitialTranslation and mL10nResources because now we have mDocumentL10n available immediately so we can consult its state and store resources in it already.
*) Vastly simplify the logic in LocalizationLinkAdded
*) Add an overridden ConnectRoot on DocumentL10n that takes a second argument which indicates that if the root has been added after initial translation is completed, it should trigger its translation (otherwise the translation will happen when the document is done parsing)

:stas, :mossop - this is unlikely going to be the final review, but I'd like to get your big-picture feedback on the changes.

I expect that to improve our Web Components hooking interoperatibility and put us on a straight path to replace Localization.jsm with fluent-fallback crate soon.
NI because I don't know how well phabricator works with r? on WIP patches.

Flags: needinfo?(stas)
Flags: needinfo?(dtownsend)

I won't have time to look at this today and I'm away tomorrow. I'll try to have some feedback by the end of the week. Keeping the ni to make sure I won't forget.

Attachment #9138677 - Attachment is obsolete: true
Attachment #9138678 - Attachment is obsolete: true

Cleaned up the patches, maintaining the same approach as in comment 0.

Keeping the NI to get feedback, but I'm going to move forward and start asking for reviews in the next days.

perfherder: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2cd32a245505fbff76d7e25390317e0bf365e3e1&newProject=try&newRevision=60637656dd1c2ca59ac8081ecc8f2042346b1183&framework=1

Initially, I expected there to be a regression - we delay JSM loading on browser.xhtml for a reason, Unfortunately, it seems that this ship has sailed since with the increasing number of Web Components and dynamically injected DOM, we now have 6 elements that are not captured by the cache in browser.xhtml: [text-action-select-all, text-action-delete, text-action-paste, text-action-copy, text-action-cut, text-action-undo]

So, here I'm just cleaning up the order of initialization and helping Web Components integrate with less event loop trickery.

After this, I'll remove Localization.jsm and that should give us a speed win, unless L10nRegistry will continue to block (which it may to load the context for the 6 strings).

The path to perf win is now the elimination of JSMs, and/or improvements in the XUL Cache to cover Web Components.
Without that, we may end up removing the cache since at least for Fluent incomplete cache is not very useful at all.

I filed bug 1630743 to discuss the future of XUL Mini Cache.

If we want to keep the cache, I can bring it back while maintaining the cleanups I accumulated here.
The way I'd do this is by moving all state from Localization.jsm to Localization CPP and keep Localization.jsm as a stateless set of functions that just benefit from JS async iterator and cached iterator.

This would mean that we would load Localization.jsm only the first time we need to call translateFragment, translateElements etc. so all state management, root connection, resource adding/removing, would be done on the class.
I think it's a good step toward JSM removal, so I'm going to work on that independently of the decision in the mini cache bug, but if we decided remove the mini cache, we could additionally clean up the whole Initial Translation logic because it'd become just DOMLocalization::TranslateRoots.

Depends on: 1630743

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #0)

In particular, we currently have a gap in our coverage where if the document doesn't have l10n links, but web component injects one, it's possible that document.l10n won't be enabled until DOMContentLoaded.

What does it mean that document.l10n isn't enabled? That it's not available? Or that its internal state prevents it from doing anything? If it's the latter, can you briefly explain why?

This means that right after initial link injection document.l10n becomes available, but only connectRoot, disconnectRoot etc. can be called on it. Any attempt to translateFragment or translateElements or translateRoots will fail.

Would it make sense to make document.l10n available and "live" always for all documents, similar to document.styleSheets? Would it help solve the problem in this bug if document.l10n could always be assumed to be "live", i.e. capable of translating the DOM? If there are no l10n links, it would localize as if all resources where missing. I'm assuming here that there are in fact very few caes where we don't want document.l10n -- but it would be helpful to know what the actual data is.

Flags: needinfo?(stas) → needinfo?(gandalf)

What does it mean that document.l10n isn't enabled? That it's not available? Or that its internal state prevents it from doing anything? If it's the latter, can you briefly explain why?

enabled means that document.l10n is not null.

The issue is that if your Web Component gets connected while the document is parsing, and if there were no FTL links before, we fall into this branch: https://searchfox.org/mozilla-central/source/dom/base/Document.cpp#3873

And the result is that document.l10n is not available in connectedCallback, forcing users to do sth like:

connectedCallback() {
  MozXULElement.insertFTLIfNeeded("browser/editBookmarkOverlay.ftl");
  document.addEventListener("DOMContentLoaded", () => {
    document.l10n.connectRoot(this.shadowRoot);
    document.l10n.translateFragment(this.shadowRoot);
  }
}

My goal is to enable document.l10n as soon as the first FTL file is inserted and allow for root adding/removing right after:

connectedCallback() {
  MozXULElement.insertFTLIfNeeded("browser/editBookmarkOverlay.ftl");
  document.l10n.connectRoot(this.shadowRoot);
  document.l10n.translateFragment(this.shadowRoot);
}

The issue with that translateFragment requires (at the moment) the JSM file to be loaded. So to prevent JSM from having to be loaded (which is what we do to avoid perf hit on browser.xhtml), I'd have to root translate* and format* methods via a Promise that gets resolved when JSM gets loaded, and then it can be delegated to JSM's translate* and format* promises.

This is quite a bit of work, which I will do if we don't find a better solution.

Would it make sense to make document.l10n available and "live" always for all documents, similar to document.styleSheets?

No, that would mean that you need to initialize DocumentL10n, DOMLocalization, Localization and, at the moment Localization.jsm for all instances of Document and we have hunderds of those on the startup path of the product.
L10n affects a small number of them and should be lazy.

We could make document.l10n lazily initialize it, but the remaining question is if we need to load JSM as soon as the first interaction with DOMLocalization happens.

We have at the moment two classes of operations on DOMLocalization:

  1. Root and Resource management - AddResourceId, RemoveResourceId, ConnectRoot, DisconnectRoot.
  2. Translation and Formatting related - FormatMessages, TranslateElements, TranslateFragments, TranslateRoots

The former can be performed without loading of JSM since they're stored on the C++ side. The latter require the JSM to be loaded.

Further, for performance reasons, we need to kick-off fetching of I/O eagerly, so we can't just say "once the first TranslateFragment gets called, initialize JSM" - we need to "initialize JSM and fetch I/O" as soon as possible and hope that by the time document is parsed the I/O is done.

So, if we want to allow people to add/remove roots and resources, but delay loading of JSM for (2), we need to be able to be in a state where you can call document.l10n.connectRoot but you cannot call translateFragment yet.

Alternatively, we can always load JSM, or we can remove JSM (I'm working on that), or we can reduce JSM to be stateless and then you could hook all methods of Localization to be pending on JSM being initialized, and then they become available immediatelly, but resolve only after the JSM is loaded.

Flags: needinfo?(gandalf)

Since the conversation so far and further analysis leads me to believe that:

a) XUL Mini Cache is useful, even if it only reduces the amount of strings we localize at warm startup
b) JSM is loaded eagerly right now, and making it load lazily is actually more work than just skip the non-proto-elements
c) This patchset doesn't impact performance (see comment 15)

I suggest that we:

  1. Land these patches
  2. Re-introduce lazy-JSM in a new bug in which I'll move all state management to C++
  3. Work on Mini Cache for UA widgets in bug 1630743
  4. Work on not-caching if localization is incomplete in bug 1629291

This means that the value of this patchset is:

  1. We clean up initialization and progress toward de-JSMification
  2. We enable Web Components hooking during document parsing via ConnectRoot(root, true)

For (2) in particular, since ConnectRoot is easily greppable, if we ever decide to further change it, we can.

I filed bug 1631593 for the follow up after this lands.

Flags: needinfo?(dtownsend)

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #19)

The issue with that translateFragment requires (at the moment) the JSM file to be loaded. So to prevent JSM from having to be loaded (which is what we do to avoid perf hit on browser.xhtml), I'd have to root translate* and format* methods via a Promise that gets resolved when JSM gets loaded, and then it can be delegated to JSM's translate* and format* promises.

This is quite a bit of work, which I will do if we don't find a better solution.

This comment made me understand the purpose of this bug, thanks.

Alternatively, we can always load JSM, or we can remove JSM (I'm working on that), or we can reduce JSM to be stateless and then you could hook all methods of Localization to be pending on JSM being initialized, and then they become available immediatelly, but resolve only after the JSM is loaded.

I was going to say that the stateless Localization.jsm sounded like an interesting alternative, but I see that you already have a WIP in bug 1631593 :)

I think the approach here makes sense given the constraints and the goal of ultimately removing Localization.jsm. My only comment about the API is that perhaps you could consider a separate method rather than an overload with a boolean argument, which hides the purpose a bit. Something like ConnectRootAndTranslate or even TranslateRoot. However, this is just a suggestion, I understand that the overload is temporary anyways. Overall, it looks good!

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e8bf2e148410 Rename *DocumentTranslation to *Translation. r=smaug https://hg.mozilla.org/integration/autoland/rev/fe7120dc7e90 Remove unncessary eager attribute from Localization API. r=stas https://hg.mozilla.org/integration/autoland/rev/4eeeb315a2bc Move Localization.jsm construction to Localization constructor. r=smaug https://hg.mozilla.org/integration/autoland/rev/d129e3c9fe7f Rename Document::TriggerInitialTranslation to Document::OnParsingCompleted. r=smaug https://hg.mozilla.org/integration/autoland/rev/01746241d8a2 Add Localization::AddResourceId and Localization::RemoveResourceId for DOM to use. r=stas https://hg.mozilla.org/integration/autoland/rev/1f62dc57e38e Move DocumentL10n construction earlier. r=smaug https://hg.mozilla.org/integration/autoland/rev/aa4e68d8ad46 Make DocumentL10n translate all roots and add ConnectRoot with initial translation. r=smaug https://hg.mozilla.org/integration/autoland/rev/5262be825fdf Make the DocumentL10n translate the document when initialized lazily. r=smaug
Regressions: 1634665
No longer depends on: 1630743
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: