Closed Bug 1363862 Opened 7 years ago Closed 6 years ago

Add a chrome-only DOM API for localization

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files, 3 obsolete files)

Bug 1363862 - Add Node.localize API as a fast-path for Fluent DOM localization.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM.
59 bytes, text/x-review-board-request
stas
: review+
Details 
In bug 1347799 we're adding JS version of the new Localization API for DOM.

:smaug prepared the C++ version that we can consider for performance reasons.
Depends on: 1347799
Attached patch L10n DOM API (obsolete) — — Splinter Review 
Priority: -- → P3

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
Comment on attachment 8874463 [details]
Bug 1363862 - Add a chrome-only DOM API for localization.

https://reviewboard.mozilla.org/r/145832/#review219884


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

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


::: dom/base/nsINode.cpp:3141
(Diff revision 2)
> +
> +
> +class LocalizationHandler : public PromiseNativeHandler
> +{
> +public:
> +  LocalizationHandler() {}

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

  LocalizationHandler() {}
  ^
                        = default;

::: dom/base/nsINode.cpp:3240
(Diff revision 2)
> +  {
> +    mReturnValuePromise->MaybeRejectWithUndefined();
> +  }
> +
> +private:
> +  ~LocalizationHandler() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~LocalizationHandler() {}
  ^
                         = default;

    
    

    
  
Olli - can I get a bit of your magic to update this POC patch?

I tried it today rebased on top of m-c and it applied cleanly but broke at build time - https://treeherder.mozilla.org/#/jobs?repo=try&revision=230d0296d5b820c2ff8e9662187ac87d25367cef&selectedJob=162703707
Flags: needinfo?(bugs)

    
    

    
  
Looks like some #include hell.
Not sure what is easiest way to fix it.

Perhaps BindingUtils.h shouldn't include nsIDocument.h, which means 
DeprecationWarning methods should be moved elsewhere.
Flags: needinfo?(bugs)

    
    

    
  
I tried to look into that, but the DeprecationWarning is used in enough places around DOM that I didn't feel like trying to decide on my own where it should go, and also it is used by the servo code which makes it two components (DOM and Servo) I don't know at all.

https://searchfox.org/mozilla-central/search?q=DeprecatedOperations&case=false&regexp=false&path=

Would you have time to unbitrot that patch for me? If not, do you know who from the DOM team may be able to help me here?
Flags: needinfo?(bugs)

    
    

    
  
ok, let me try to update the patch.
Flags: needinfo?(bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        l10n_2.diff (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8874463 -
        Attachment is obsolete: true

        Attachment #8952796 -
        Attachment is obsolete: true

        Attachment #8866502 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8952869 [details]
Bug 1363862 - Add Node.localize API as a fast-path for Fluent DOM localization.

https://reviewboard.mozilla.org/r/222100/#review228040


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


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


::: dom/base/nsINode.cpp:3070
(Diff revision 1)
>  }
> +
> +class LocalizationHandler : public PromiseNativeHandler
> +{
> +public:
> +  LocalizationHandler() {}

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

  LocalizationHandler() {}
  ^
                        = default;

::: dom/base/nsINode.cpp:3169
(Diff revision 1)
> +  {
> +    mReturnValuePromise->MaybeRejectWithUndefined();
> +  }
> +
> +private:
> +  ~LocalizationHandler() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~LocalizationHandler() {}
  ^
                         = default;
No longer blocks: 1365426

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> I tried to look into that, but the DeprecationWarning is used in enough
> places around DOM that I didn't feel like trying to decide on my own where
> it should go, and also it is used by the servo code which makes it two
> components (DOM and Servo) I don't know at all.

For the record, that Servo code is just autogenerated bindings, so you shouldn't worry about those in general.

    
    

    
      
  

    
    

    
  
Comment on attachment 8952869 [details]
Bug 1363862 - Add Node.localize API as a fast-path for Fluent DOM localization.

https://reviewboard.mozilla.org/r/222100/#review228934


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


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


::: dom/base/nsINode.cpp:3066
(Diff revision 2)
>  }
> +
> +class LocalizationHandler : public PromiseNativeHandler
> +{
> +public:
> +  LocalizationHandler() {}

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

  LocalizationHandler() {}
  ^
                        = default;

::: dom/base/nsINode.cpp:3165
(Diff revision 2)
> +  {
> +    mReturnValuePromise->MaybeRejectWithUndefined();
> +  }
> +
> +private:
> +  ~LocalizationHandler() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  ~LocalizationHandler() {}
  ^
                         = default;

    
    

    
  
Comment on attachment 8952869 [details]
Bug 1363862 - Add Node.localize API as a fast-path for Fluent DOM localization.

https://reviewboard.mozilla.org/r/222100/#review229100

::: intl/l10n/DOMLocalization.jsm:474
(Diff revision 2)
>      const roots = Array.from(this.roots);
>      return Promise.all(
> -      roots.map(root => this.translateElements(this.getTranslatables(root)))
> +      roots.map(root => {
> +        return root.localize(l10nItems => {
> +          let keys = l10nItems.map(l10nItem => [l10nItem.l10nId, l10nItem.l10nArgs]);
> +          return this.formatEntities(keys).then(translations => {

Localization.formatEntites has been renamed to Localization.formatMessages. Did you see any errors while testing this code?

::: intl/l10n/DOMLocalization.jsm:475
(Diff revision 2)
>      return Promise.all(
> -      roots.map(root => this.translateElements(this.getTranslatables(root)))
> +      roots.map(root => {
> +        return root.localize(l10nItems => {
> +          let keys = l10nItems.map(l10nItem => [l10nItem.l10nId, l10nItem.l10nArgs]);
> +          return this.formatEntities(keys).then(translations => {
> +            return translations.map(translation => {

Readability nit: This code reaches the 8th level of indentation. I'd suggest to factor the callbacks to root.localize() and translations.map() out to separately defined functions.  Rather then nesting the logic in the then() callback, you can also use an async function here.

::: intl/l10n/DOMLocalization.jsm:490
(Diff revision 2)
> +              };
> +            });
> +          });
> +        });
> +      })
> +      // roots.map(root => this.translateElements(this.getTranslatables(root)))

How should we structure this code in the upstream fluent.js repository? I think overriding the translateRoots method in GeckoDOMLocalization should do the trick. Not sure if you tried it yet?

    
    

    
      
  

    
    

    
  
Thanks stas! I refreshed the patch and aligned it with Fluent to reduce the number of operations. The patch looks clean now and gives really good talos wins basically getting us on par with m-c on the startup path.

Unfortunately, it has one limitation - DOM Overlays.

If we had the final version of DOM Overlays we could reason about our options to move it to C++/Rust, but until then, I'm wondering how to approach this.

We can keep not landing it, but that has a tangible impact on Fluent XUL uses right now including ~3% on Preferences, and prevents us from landing on the startup path.

Alternatively, we could try to land it in some way that allows us to use it for majority of strings leaving the DOM Overlay enabled ones for JS implementation and assuming that the speedup for the 95% of cases will carry over.

One idea I had for such a flow is that we could do this:

1) elem.localize called fetches all localizable elements
2) we format messages looking for `!reOverlay.test(value)` and pass those back to the C++ API for application
3) for the ones which do have overlay, we would have to pass `undefined` and adapt the API to skip those elements
4) And then translate those inside JS

:stas - wondering if you have any thoughts on this, esp. in the context of DOM Overlays v2. Do you have any idea on timing? Will it affect how recognize overlays? (for example, if translation value doesn't match `reOverlay` but the source has children?)?
Do you think that such a hybrid solution may work?
Flags: needinfo?(stas)

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> Thanks stas! I refreshed the patch and aligned it with Fluent to reduce the
> number of operations. The patch looks clean now and gives really good talos
> wins basically getting us on par with m-c on the startup path.

Thanks. I wonder if it would make sense to create ContentDOMLocalization and ChromeDOMLocalization sublasses, each with their own translateFragment. We can discuss the specifics later on.

> 
> Unfortunately, it has one limitation - DOM Overlays.

> We can keep not landing it, but that has a tangible impact on Fluent XUL
> uses right now including ~3% on Preferences, and prevents us from landing on
> the startup path.

Is the +3% increase going to block us in Preferences? How far are we from having a first startup-path migration patch?

> Alternatively, we could try to land it in some way that allows us to use it
> for majority of strings leaving the DOM Overlay enabled ones for JS
> implementation and assuming that the speedup for the 95% of cases will carry
> over.

This might be a good path forward if we're able to find a good heuristic for choosing which implementation to use.

> 1) elem.localize called fetches all localizable elements
> 2) we format messages looking for `!reOverlay.test(value)` and pass those
> back to the C++ API for application
> 3) for the ones which do have overlay, we would have to pass `undefined` and
> adapt the API to skip those elements
> 4) And then translate those inside JS

We'd need to somehow mark those elements as still requiring translation.

> :stas - wondering if you have any thoughts on this, esp. in the context of
> DOM Overlays v2. Do you have any idea on timing? Will it affect how
> recognize overlays? (for example, if translation value doesn't match
> `reOverlay` but the source has children?)?

We'll need to verify this once we design the next iteration of DOM Overlays. My current thinking is that testing reOverlay should be enough. But I'll be able to say more once the details crystallize. I'll start the design work later this week. I expect testable code to be ready by mid-March and make its way into a new version of fluent-dom by the end of March.
Flags: needinfo?(stas)

    
    

    
  
> Thanks. I wonder if it would make sense to create ContentDOMLocalization and ChromeDOMLocalization sublasses, each with their own translateFragment. We can discuss the specifics later on.

Yeah, I believe that it would.

> Is the +3% increase going to block us in Preferences? How far are we from having a first startup-path migration patch?


It affects our product as it goes into release. I'd like to minimize the window where Fluent affects user experience. There's also a regression that may be (although I hope it's not) affected by our recent landing - bug 1442262.

I'm investigating it by testing talos with and without bug 1435912, but generally speaking, adding more Fluent into XUL without this patch is going to be negatively affecting performance.

I believe we need this patch to mitigate this.

> We'd need to somehow mark those elements as still requiring translation.

Yes, I have a POC patch that does just that - it translates majority of strings via `element.localize` and then remaining ones (the ones that use DOM Overlays) via regular `DOMLocalization.applyTranslations`.

The question is if we can squeeze the latter before first paint or not. Tbh I'd be ok stating that one limitation of Fluent in XUL+XBL scenario is that we lazily resolve DOM Overlay translations post first-paint. There are few of them and resolving them lazily doesn't cause flickers.

But first I'll try to get them in before first paint either by synchronously performing applyTranslations or asynchronously in a microtask or requestAnimationFrame.

> My current thinking is that testing reOverlay should be enough. But I'll be able to say more once the details crystallize.

That's surprising to me. I'd expect that we have to handle a scenario like this:

```
key = Regular text value

<p data-l10n-id="key">
  <img/>
</p>
```

without throwing away the `<img/>`?

But I'll leave it to you to figure it out.

Finally - once we get the final DOM Overlays design we may want to move it *all* to DOM C++. I don't think it's that much work and we can pretty much replicate all steps we're doing in JS from C++. It's just not worth it until we have the v2 of DOM Overlays, so let's wait for that :)

    
    

    
  
Interestingly, first results of the most naive approach are quite optimistic.

I basically perform the separate out the DOMOverlay affected elements, and translate them via JS while the rest get translated via the C++ code.


Locally the DOMOverlay separation did not bring any regressions to this patch on ts_paint. Testing on try.

    
    

    
      
  


  

    
    

    
      
  
Assignee: nobody → gandalf
Status: NEW → ASSIGNED

    
    

    
  
:baku, :stas - this patches fix the regression reported in bug 1442262 and also make Fluent ready for startup path.

I was hoping to wait till 61 with landing them just because startup path was not the priority yet, but with the regression on preferences talos landing this patches seems like one of the three options:

1) swallow the regression in 60 (ESR!) and fix it in 61
2) back out bug 1435912
3) land this

Here's a comparison between those three: https://pike.github.io/talos-compare/?revision=67b4498d75f9&revision=68cc89e4139e&revision=2f59d6fc4d35

My preference would be to go with (3) because the patches aren't big and we know we'll need them anyway for at least until we finish killing XBL (bug 1397874), so might as well land them now and benefit from them already.

Details on patches:

:baku - the patch has been written by :smaug. It works around the regression from how stylo-chrome handles XBL (see bug 1441037 for evaluation and profiling history).

It's chrome only and allows us to perform a localization of the DOM on the startup path before layout without having to create reflections and resolve XBL bindings.
It is temporary since we didn't need it before stylo-chrome landed and the profiling indicates that the XBL is the sole reason we regress. So once we fix XBL and move to Web Components we will be able to remove it.

:stas - This implements what we discussed above. I scan each translation for `reOverlay` and remove those that match from the translations applies via `Node.localize` using `overlayElement` instead.

I think there's room for further code cleanup and I'd like to do it as part of separating GeckoDOMLocalization out of DOMLocalization, but would prefer to avoid having to do this in 60.

    
    

    
  
Here's a talos compare with the final patches (the ones currently with r?) as the (3) scenario: https://pike.github.io/talos-compare/?revision=67b4498d75f9&revision=68cc89e4139e&revision=29ac9b6ce77e

And here's a regular talos compare between backout and this patch landing: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=68cc89e4139e&newProject=try&newRevision=29ac9b6ce77e&framework=1

The only relevant test here is the `about_preferences_basic` of course.

    
    

    
  
Comment on attachment 8952869 [details]
Bug 1363862 - Add Node.localize API as a fast-path for Fluent DOM localization.

https://reviewboard.mozilla.org/r/222100/#review230550

Looks good. I just want to see the binding changes for having JSContext from the WebIDL Binding.

::: dom/base/nsINode.cpp:3114
(Diff revision 4)
> +        if (!slotPtr) {
> +          mReturnValuePromise->MaybeRejectWithUndefined();
> +          return;
> +        }
> +
> +        L10nValue& slot = *slotPtr;

Why do you need this ref? Can you simply call: slotPtr->Init(aCx, temp) ?

::: dom/base/nsINode.cpp:3133
(Diff revision 4)
> +    for (size_t i = 0; i < l10nData.Length(); ++i) {
> +      Element* elem = mElements[i];
> +      nsString& content = l10nData[i].mValue;
> +      if (!content.IsVoid()) {
> +        elem->SetTextContent(content, rv);
> +        if (rv.Failed()) {

I would use NS_WARN_IF

::: dom/base/nsINode.cpp:3196
(Diff revision 4)
> +already_AddRefed<Promise>
> +nsINode::Localize(mozilla::dom::L10nCallback& aCallback,
> +                  mozilla::ErrorResult& aRv)
> +{
> +  nsAutoString l10nId;
> +  nsAutoString l10nArgs;

move these 2 near where you use them.

::: dom/base/nsINode.cpp:3198
(Diff revision 4)
> +                  mozilla::ErrorResult& aRv)
> +{
> +  nsAutoString l10nId;
> +  nsAutoString l10nArgs;
> +  AutoJSAPI jsapi;
> +  if (!jsapi.Init(OwnerDoc()->GetParentObject())) {

Don't do this.

in dom/bindings/Bindings.conf, in the 'Node' section, add:

'implicitJSContext': [ 'localize' ]

Doing this you will:

already_AddRefed<Promise>

nsINode::Localize(JSContext* aCx,
                  mozilla::dom::L10nCallback& aCallback,

                  mozilla::ErrorResult& aRv)

::: dom/base/nsINode.cpp:3214
(Diff revision 4)
> +    if (!node->IsElement()) {
> +      continue;
> +    }
> +
> +    Element* domElement = node->AsElement();
> +    if (domElement->GetAttr(kNameSpaceID_None, nsGkAtoms::datal10nid, l10nId)) {

if (!domElement->GetAttr(kNameSpaceID_None, nsGkAtoms::datal10nid, l10nId)) {
  continue;
}

::: dom/base/nsINode.cpp:3221
(Diff revision 4)
> +      L10nElement* element = l10nElements.AppendElement(fallible);
> +      if (!element) {
> +        aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
> +        return nullptr;
> +      }
> +      domElements.AppendElement(domElement);

Maybe we want this fallible as well...?

::: dom/base/nsINode.cpp:3227
(Diff revision 4)
> +
> +      domElement->GetNamespaceURI(element->mNamespaceURI);
> +      element->mLocalName = domElement->LocalName();
> +      element->mL10nId = l10nId;
> +      if (!l10nArgs.IsEmpty()) {
> +        JS::Rooted<JS::Value> json(jsapi.cx());

here you will use the aCx received by the webidl binding.

::: dom/webidl/L10nUtils.webidl:6
(Diff revision 4)
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +

Add a comment saying that these dictionaries are for mozilla only and why we need them.

        Attachment #8952869 -
        Flags: review?(amarchesini) → review-

    
    

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

> That's surprising to me. I'd expect that we have to handle a scenario like
> this:
> 
> ```
> key = Regular text value
> 
> <p data-l10n-id="key">
>   <img/>
> </p>
> ```
> 
> without throwing away the `<img/>`?

Well, if it's an argument to the translation then it makes sense to me to treat it the same way as we treat variables. If a string takes $num as argument but the localization doesn't use it, we don't just append it anyways. OTOH, to help solve the retranslation case, perhaps there are ways to not remove child elements but hide them (in an a11y-friendly manner) instead?

As I said, this was my initial thinking. I have not yet started the work on the next iteration of DOM overlays, so I can't say for sure. Which is also why I'd prefer not to rush anything here. If we land this in the current state in 60, can we go back on the decision about testing reOverlay later on?

    
    

    
  
Comment on attachment 8955366 [details]
Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM.

https://reviewboard.mozilla.org/r/224550/#review230662

I think there might be a problem in how you match overlayElements to overlayTranslations. Also, it was my impression that we agreed to use a separate subclass for chrome-only DOMLocalization instead of testing for `frag.localize`? Lastly, I would also like to see the PR to fluent.js before this lands. Thanks!

::: intl/l10n/DOMLocalization.jsm:551
(Diff revision 1)
>     *
>     * @param   {DOMFragment} frag - Element or DocumentFragment to be translated
>     * @returns {Promise}
>     */
>    translateFragment(frag) {
> +    if (frag.localize) {

As I mentioned before, we should separate this logic into its own subclass. Do you intend to do this in this patch? That would be my preference.

::: intl/l10n/DOMLocalization.jsm:566
(Diff revision 1)
> +        // implementation, while everything else is going to use the fast-path.
> +        const translations = await this.formatMessages(keys);
> +        const overlayL10nIds = [];
> +        const overlayTranslations = [];
> +
> +        for (const i in translations) {

Nit: Use Array.entries() here.

::: intl/l10n/DOMLocalization.jsm:581
(Diff revision 1)
> +
> +        this.pauseObserving();
> +
> +        if (overlayL10nIds.length > 0) {
> +          const query = overlayL10nIds.map(
> +            l10nId => `[data-l10n-id="${l10nId}"]`).join(",");

Should we be worried about the number of translations which require overlays? A quick test shows that quering for 10 ids takes around 1ms on my machine while quering for 100 takes 3-5ms. I wonder if it's worth exploring "tagging" these elements with a `data-l10n-overlay` attribute; but then we'd need to set it on the element. Would it possible to pass the element itself in `l10nItems`?

::: intl/l10n/DOMLocalization.jsm:586
(Diff revision 1)
> +            l10nId => `[data-l10n-id="${l10nId}"]`).join(",");
> +
> +          const overlayElements = frag.querySelectorAll(query);
> +
> +          for (let i = 0; i < overlayElements.length; i++) {
> +            overlayElement(overlayElements[i], overlayTranslations[i]);

There might be more than one element with the same `data-l10n-id` in which case overlayElements will have more items than overlayTranslations.

::: intl/l10n/DOMLocalization.jsm:591
(Diff revision 1)
> +            overlayElement(overlayElements[i], overlayTranslations[i]);
> +          }
> +        }
> +
> +        return translations;
> +      }).then(() => {

Please use async/await. In general, no more then()s in fluent code, please.

::: intl/l10n/DOMLocalization.jsm:592
(Diff revision 1)
> +          }
> +        }
> +
> +        return translations;
> +      }).then(() => {
> +        this.resumeObserving();

If the promise returned by frag.localize is rejected, we'll never resume the observer. Yet another reason to use async/await which allows you to `try {…} finally {…}`.

        Attachment #8955366 -
        Flags: review?(stas) → review-

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8955366 [details]
Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM.

https://reviewboard.mozilla.org/r/224550/#review230662

> As I mentioned before, we should separate this logic into its own subclass. Do you intend to do this in this patch? That would be my preference.

That's bug 1434434 and I will work on that early in 61. This patch is big enough as it is and I'd like to avoid any further increases if possible.

> Please use async/await. In general, no more then()s in fluent code, please.

I originally changed that to async/await and then my final patch didn't address the performance regression at all. After painful two days of bisecting, this change unfortunately is responsible for it.

I'll attach talos results, but I don't believe we can use it here. I'm not sure what's the reason but I suspect additional intermediate promises delaying the code being the cause.

Either way, I left it as a `.then`.

    
    

    
  
Thank you :baku and :stas for the review.

On Friday I worked through your feedback and got a really nice patchset with documentation and tests. I'm particularly happy about the solution to the DOM Overlays problem - returning untranslated elements creates reflections only for the affected ones, keeping the performance in shape and also ensuring we're not at risk of FOUCs.

Then I pushed it to talos which showed the new patchset on par with mozilla-central (i.e. - not fixing the regression) and I spent the whole weekend pushing tens of talos runs to bisect what caused it.

Initially I was afraid that even one JS reflection returned to `untranslatedElements` will be the cause, or that implicit JS Context somehow does it.

Eventually, I narrowed it down to a single change - from `Promise.then` to `async/await`.

Removing that change gets back the performance win while keeping the C++ API logic sane and the flow between JS and C++ relatively straightforward.

Below is the talos run with 5 builds (about_preferences_basic is the only test we care about now):

1) mozilla-central
2) backout patch
3) current patchset from this bug
4) same patchset as in (3) but on top of the new microtasks patch (bug 1193394)
5) same patchset but with `Promise.then` switched to `async/await`

I added (4) to verify that we don't regress there since bug 1193394 is landing today. We don't!

https://pike.github.io/talos-compare/?revision=e1fd4f620189&revision=a66fbccf6673&revision=12db8b0b6591&revision=8afdbfbf7995&revision=3eab75516e49

Overall, I'd really like to land this patch as close to its current state as possible. It removes the performance regression in 60, makes us avoid having to back out Fluent patches, gives us a nice performance boost and puts us on a path to be ready for Fluent on the startup path. :)

Requesting review.

    
    

    
  
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: replication log not available; all writes disabled
remote: pretxnopen.vcsreplicator hook failed
abort: push failed on remote

    
    

    
  
Comment on attachment 8955366 [details]
Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM.

https://reviewboard.mozilla.org/r/224550/#review230930

This looks mostly good but I'd like to see another iteration. I'd also appreciate a PR to fluent.js.

I'm sad to hear that async/await caused a performance hit. Perhaps it's the cost of running the generator behind the scenes? Can you file a bug about this in Core::JavaScript Engine?

Since this is getting more complex, I also suggest to add a few more tests:

  - a translation is missing in all locales (see my inline comment about a bug in overlayElement; I think we'll need an upstream fix in fluent-dom),
  - the same l10n-id is used more than once with simple translation,
  - the same l10n-id is used more than once with translation which requires overlays,
  - an element to be localized with Node.localize already has a localizable attribute (e.g. title).

::: intl/l10n/DOMLocalization.jsm:557
(Diff revision 3)
> +      // This is a temporary fast-path offered by Gecko to workaround performance
> +      // issues coming from Fluent and XBL+Stylo performing unnecesary
> +      // operations during startup.
> +      // For details see bug 1441037, bug 1442262, and bug 1363862.
> +
> +      const overlayTranslations = [];

Please document that this is a sparse array (if I'm reading the C++ code correctly.)

::: intl/l10n/DOMLocalization.jsm:559
(Diff revision 3)
> +      // operations during startup.
> +      // For details see bug 1441037, bug 1442262, and bug 1363862.
> +
> +      const overlayTranslations = [];
> +
> +      return frag.localize(async l10nItems => {

Given that `frag.localize()` starts a Promise chain, having this callback defined anonymously here makes it look as if it was a Promise callback. Let's factor it out to a function declared earlier (`const getTranslationsForItems = (l10nItems) => …`) or to a method.

::: intl/l10n/DOMLocalization.jsm:567
(Diff revision 3)
> +        const translations = await this.formatMessages(keys);
> +
> +        // Here we want to separate out elements that require DOM Overlays.
> +        // Those elements will have to be translated using our JS
> +        // implementation, while everything else is going to use the fast-path.
> +        for (const [i, translation] of Object.entries(translations)) {

You can use `translations.entries()` here which returns indexes as integers rather than strings as is the case of `Object.entries()`.

::: intl/l10n/DOMLocalization.jsm:568
(Diff revision 3)
> +
> +        // Here we want to separate out elements that require DOM Overlays.
> +        // Those elements will have to be translated using our JS
> +        // implementation, while everything else is going to use the fast-path.
> +        for (const [i, translation] of Object.entries(translations)) {
> +          if (reOverlay.test(translation.value)) {

reOverlay and isAttrNameLocalizable are not exported by overlay.js in the upstream fluent-dom source code. Please factor this code out into a function which lives in overlay.js and is imported by dom_localization.js.

This should also check if `translation !== undefined` first. In fact, I think there's a bug in the current implementation of `overlayElement`: it doesn't look like it support `undefined` translations passed into it.

Moving this to overlay.js would be a good opportunity to fix this, too.

::: intl/l10n/DOMLocalization.jsm:575
(Diff revision 3)
> +            overlayTranslations[i] = translations[i];
> +            translations[i] = undefined;
> +            continue;
> +          }
> +
> +          // Sanitizing attributes

There's code in overlayElement which also removes any old localizable attributes from the element before it adds any new ones. It's a fix for bug 922577.

I wonder if there's a way to emulate this in Node.localize, too. All I can think of is: l10nItems could include the string-typed name of the element which the JS callback would use to retrive the list of localizable attributes for this element type and put in `translations`. We'd then remove those attributes in C++. However, this sounds like passing many arrays of strings for each `l10nItem`.

Do you have any other ideas how to fix this? Or, should we consider it a limiation of this approach?

::: intl/l10n/DOMLocalization.jsm:581
(Diff revision 3)
> +          if (translation.attrs) {
> +            const l10nItem = l10nItems[i];
> +            const explicitlyAllowed = l10nItem.l10nAttrs === null ? null :
> +              l10nItem.l10nAttrs.split(",").map(i => i.trim());
> +            for (const j in translation.attrs) {
> +              const {name, } = translation.attrs[j];

Nit: no need for the comma. I'd also avoid using `in` for iterating over arrays. You can use array.entries() here as well:

    for (const [j, {name}] of translation.attrs.entries()) {
        …
    }

::: intl/l10n/DOMLocalization.jsm:589
(Diff revision 3)
> +              }
> +            }
> +          }
> +        }
> +
> +        this.pauseObserving();

Do the changes to element values and attributes done by frag.localize() trigger mutation events? If not, we don't have to pause the observer here but rather we could do it synchronously in the part which handles `overlayTranslations`.

::: intl/l10n/DOMLocalization.jsm:592
(Diff revision 3)
> +        }
> +
> +        this.pauseObserving();
> +
> +        return translations;
> +      }).then((untranslatedElements) => {

Nit: no need for the parens around `untranslatedElements`.

::: intl/l10n/DOMLocalization.jsm:600
(Diff revision 3)
> +              untranslatedElements[i] !== undefined) {
> +            overlayElement(untranslatedElements[i], overlayTranslations[i]);
> +          }
> +        }
> +
> +        this.resumeObserving();

Please copy this into a `.catch()` which follows this `then()`.

        Attachment #8955366 -
        Flags: review?(stas) → review-

    
    

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

> 1) mozilla-central
> 2) backout patch
> 3) current patchset from this bug
> 4) same patchset as in (3) but on top of the new microtasks patch (bug
> 1193394)
> 5) same patchset but with `Promise.then` switched to `async/await`

Did you have a chance to try the async/await version on top of bug 1193394?

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Thanks stas! Updated the patch to your feedback and added tests. Re-requesting r?

> Do the changes to element values and attributes done by frag.localize() trigger mutation events? 

Yes, it does.

> Did you have a chance to try the async/await version on top of bug 1193394?

Yes, unfortunately, same perf regression. I'll file a bug for it.

> I wonder if there's a way to emulate this in Node.localize, too.

I documented it as a limitation and added a test that verifies that.

    
    

    
  
Comment on attachment 8955366 [details]
Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM.

https://reviewboard.mozilla.org/r/224550/#review231042

Thanks for addressing my previous review comments!

::: intl/l10n/DOMLocalization.jsm:614
(Diff revisions 3 - 5)
> -            translations[i] = undefined;
>              continue;
>            }
>  
> -          // Sanitizing attributes
> -          if (translation.attrs) {
> +          // this will return `false` if the translation contains DOM Overlays
> +          if (!sanitizeTranslationForNodeLocalize(l10nItems[i], translation)) {

Nit: perhaps use an intermediate variable rather than a comment?

    const hasOnlyText = sanitizeTranslationForNodeLocalize(...);
    
    if (!hasOnlyText) {
        // Remove to make Node.localize skip it.
    }

::: intl/l10n/DOMLocalization.jsm:622
(Diff revisions 3 - 5)
> -              }
> -            }
>            }
>          }
>  
>          this.pauseObserving();

Please document why we're doing this here rather than later.

::: intl/l10n/DOMLocalization.jsm:628
(Diff revisions 3 - 5)
>          return translations;
> -      }).then((untranslatedElements) => {
> +      };
> +
> +      return frag.localize(getTranslationsForItems.bind(this))
> +        .then(untranslatedElements => {
> +          if (overlayTranslations !== null) {

No need to check this: `overlayTranslations` is always an array.

::: intl/l10n/DOMLocalization.jsm:639
(Diff revisions 3 - 5)
> -        }
> +            }
> -
> +          }
> +          this.resumeObserving();
> +        })
> +        .catch((e) => {
> -        this.resumeObserving();
> +          this.resumeObserving();

Nit: This can be written as:

    .catch(() => this.resumeObserving());

        Attachment #8955366 -
        Flags: review?(stas) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Thanks Stas!

Updated the patch to your feedback.

Here are today's talos results: https://pike.github.io/talos-compare/?revision=3230251ec0b5&revision=34d2d72e4afa&revision=768a021719d7

Seems like this patch has a modest improvement over the backout!

:baku - I got a request from relmgmt to land it ASAP if we want to hit this cycle and I noticed that you have a list of review requests in you queue. Would you have time to review it or is there someone else you can redirect r? to ?
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 8952869 [details]
Bug 1363862 - Add Node.localize API as a fast-path for Fluent DOM localization.

https://reviewboard.mozilla.org/r/222100/#review231188

        Attachment #8952869 -
        Flags: review?(amarchesini) → review+

    
    

    
  
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ce625a51597
Add Node.localize API as a fast-path for Fluent DOM localization. r=baku
https://hg.mozilla.org/integration/autoland/rev/c483f8c3bf07
Use Node.localize for fragment translation in Fluent DOM. r=stas

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3ce625a51597
https://hg.mozilla.org/mozilla-central/rev/c483f8c3bf07
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(amarchesini)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: