Closed Bug 1558602 Opened 4 months ago Closed 4 months ago

Add an option to localize document synchronously

Categories

(Core :: Internationalization, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Currently, Fluent in DocumentL10n is fully asynchronous due to async I/O.

bz pointed out that in case of some documents such as browser.xhtml we will likely want to block I/O, at least until we refactor browser.xhtml to handle lazy localization.

Until then, we block layout on it, so first paint, so there's no benefit in using async I/O.

From my early talos tests it also seems like we may be able to improve performance for the startup scenario and unblock things like bug 1501886.

My approach is to add mIsSync flag to DOMLocalization and data-l10n-sync attribute on a document that we want to localize synchronously.

All API calls to document.l10n.* are still asynchronous, but the initial translation uses a new method FormatMessagesSync which should be faster because it doesn't have to go through microtasks.

Assignee: nobody → gandalf
Blocks: 1441035
Status: NEW → ASSIGNED
Priority: -- → P2

Olli - I think my use of JSAPI to juggle strings and L10nMessages between JS and C++ here is atrocious. I'm sorry for that :(

I also will want to refactor tests to be able to run them with sync and async and verify that both cases work, but first I'd like to get your initial feedback on the patch itself.

Bugbug thinks this bug is a enhancement, but please change it back in case of error.

Type: defect → enhancement

I have some thoughts on the patch. Taking them here because they're more about "what" and less about "how".

First and foremost, I think it's the right thing to lift the ban of sync IO in our Fluent APIs. That sounds subtle, but it actually isn't.

In my book, we need some additional changes:

  • Deprecation and then removal of LocalizationSync.
  • Unification of sync and async generateBundles. Right now, you can give different locales and resources and even algorithm for each. That was OK when the two were completely distinct, but now that they're not, that's not how it should work.
  • Documentation updates.

The first is relatively straightforward, I guess. It's just mozIntl right now?

The second is a bit hairy. I think that the tuple of generators should be replaced by an object that builds in the cache, and implement both async and sync iteration. This would unify and obsolete CachedIterator. This might make it easier to write testing for anything that currently modifies and resets the l10n registry?

The third is not just docs, but also policy and guidelines on when it's OK to use sync IO. That's even actual browser-arch work, I'd think?

Let's talk about how to get there?

Let's talk about how to get there?

The changes you propose seem to cover:

And in result

I have a lot of questions about your proposal. I don't understand what signature of methods on Localization do you propose, how such object that builds in the cache should look like, etc.

But the bottom line is that if I'm not mistaken, this patch can land and enable me now, while we work on the proposed refactor. We're one r+ away from enabling removal of DTD on the startup path. Do you believe we should block that on your proposed changes?

Flags: needinfo?(l10n)

FYI, I don't think that the l10n registry changes would not bleed into that much code paths. I'm still expecting the generateBundles to return an iterable, as it does now.

Zibi, looking at bug 1557713, should the sync API in the content process be blocked until we can work it out? How would mozIntl be affected?

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

How would mozIntl be affected?

getLanguageDisplayNames, getRegionDisplayNames and getLocaleDisplayNames would not work in content process.
They also don't work now I believe.

Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24e366d72108
Allow DocumentL10n to use LocalizationSync. r=smaug,Pike
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.