1552714 - Extract DOMLocalization out of DocumentL10n, expose it via WebIDL and replace DOMLocalization.jsm with it.

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P2)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

The remaining cleanup after bug 1546432 lands is to:

• Extract DOMLocalization API from DocumentL10n, leaving in DocumentL10n just the initialization logic of DOMLocalization for the Document
• Expose it via WebIDL
• Replace DOMLocalization.jsm uses with the new API

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Extract DOMLocalization out of DocumentL10n.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Olli, I'd like to ask for design feedback for this bug.

The basic idea is that in JS world, we had a class called DOMLocalization which extended a class called Localization. We also had an interface called DocumentL10n which is what you see on document.l10n.

In the most common case, when you create a Document you instantiate its DocumentL10n which implements all of DOMLocalization and Localization. Done.

Now, that we moved DOMLocalization to C++ we de-facto moved it into DocumentL10n which used to wrap DOMLocalization, but after bug 1546432 it wraps Localization and implement all of DOMLocalization itself.

That results in us having two DOMLocalization implementations - one in JS accessible via DOMLocalization.jsm, and another folded into DocumentL10n in C++.

I'd like to disentangle it and get to the point where we have:

• Localization.jsm (in the future this will be moved to Rust to use fluent-rs)
• DOMLocalization in C++
• DocumentL10n in C++

The benefit of this division is that we would get a standalone API for most of the functionality that now ended in DocumentL10n, making DocumentL10n much thinner and easier to reason about.
What's left in DocumentL10n is basically initial translation bits (collecting resource links, triggering initial translation, unblocking layout etc.) and one public field mReady which is resolved once initial translation is completed.
We would also remove DOMLocalization.jsm, because all places that now call for DOMLocalization.jsm manually will just use the WebIDL and C++ version of it.

There are two open question that I'm not sure how to resolve:

1. DocumentL10n is a "public" webidl, while I so far placed DOMLocalization.webidl in chrome-webidl. The issue is that most of the helper structures like L10nKey and L10nValue live in webidl/L10nUtils.webidl and webidl/DocumentL10n.webidl. They will be only called from DOMOverlays and DOMLocalization so I'd like to move them to DOMLocalization.webidl, but since it's chrome-webidl, I'm not sure if webidl/DocumentL10n.webidl can use them? Should I make DOMLocalization part of webidl?

2. DocumentL10n class can instantiate DOMLocalization as a field and cast all methods like setAttributes, getAttributes, translateElements, translateFragment, pauseObserver, resumeObserver, connectRoot and disconnectRoot to it, or it can extend it. It feels to me that what we should do is to make DocumentL10n.webidl extend DOMLocalization.webidl and do the same to classes, but I'm not sure if its viable and desired. It also likely depends on (1).

Does my plan sound reasonable to you?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Refactor constructors.

Depends on D32397

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Move DocumentL10n to dom/l10n.

Depends on D32956

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Merge L10nUtils.webidl into DOMOverlays.webidl.

Depends on D32957

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Move DOM L10n tests from intl/l10n to dom/l10n.

Depends on D33196

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Refactor constructors of mozILocalization and DOMLocalization to handle custom generateMessages.

Depends on D33197

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Move dom tests to use WebIDL DOMLocalization.

Depends on D33198

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Migrate all remaining callsites to use WebIDL DOMLocalization and remove mozIDOMLocalization and DOMLocalization.jsm.

Depends on D33199

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

talos looks clean - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8aae25367b873d549d1f24ed9302e3b43fb311ae&newProject=try&newRevision=9bb70cc869b7b115542a67139d5ca2003d75e11c

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab7bdb4c5a854f5c2cd13f4cb4ab819b903dbd44&selectedJob=249469584

It looks good I think, but I had to fix two tests that were testing for translated content without waiting for l10n.ready.

While I believe the tests should wait, it also makes me think that maybe somewhere in this patchset I am changing when the initial translation happens. I'll investigate a bit more, but the patches should be in a pretty reviewable state.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Why is the patch changing any behavior (like the test failures hint)?

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Olli Pettay [:smaug] from comment #13)

Why is the patch changing any behavior (like the test failures hint)?

I'm going patch by patch and investigating that now :)

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Remove dom::l10n namespace and unify class naming.

Depends on D33200

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Fix tests to compare the result of getAttributes against L10nKeys.

Depends on D33739

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1552714 - Fix two tests which were caught not waiting for l10n but checking for l10n values.

Depends on D33740

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

New talos - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b21951955b1477c7be72a3e9c4a0701ab3e2ec75&newProject=try&newRevision=e4ea57dfdc04233f12beb7052d167db335e815e6

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

And a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f176671349c919eef0efd3970de5e0d80c86f4a

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think I have one orange left in the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f176671349c919eef0efd3970de5e0d80c86f4a&selectedJob=250227211

I don't understand a) why is it only happening in opt, not debug, and b) why I can't reproduce it locally with either opt or debug build even with --verify. :(

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Sorry to bother you Gijs, but you were one of the reviewers of the cert viewer migration to Fluent. I'm trying to find anything to start debugging this orange. I don't know where to start.

Comment 22

[:Gijs (he/him)]

Does it actually fail outside of TV? I don't see that on try. There are a lot of issues on file for this test in TV, which seem to just have been closed because they weren't hit because it's TV (so only runs if the test changes).

Using SimpleTest.requestCompleteLog and logging all calls to l10n.set/getAttributes would probably get you a start here, plus making https://searchfox.org/mozilla-central/source/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js#167-169 dump a stack with Cu.reportError.

How are you changing this test (given that otherwise it wouldn't run on TV at all)?

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Does it actually fail outside of TV?

I don't know how to find where outside of TV this test is but it seems like it doesn't - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f176671349c919eef0efd3970de5e0d80c86f4a&selectedJob=250225826

There are a lot of issues on file for this test in TV, which seem to just have been closed because they weren't hit because it's TV (so only runs if the test changes).

Right, I've seen that closed bug. But I can't reproduce this issue locally (the test passes both in opt and debug)

Using SimpleTest.requestCompleteLog and logging all calls to l10n.set/getAttributes would probably get you a start here, plus making https://searchfox.org/mozilla-central/source/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js#167-169 dump a stack with Cu.reportError.

Trying that now.

How are you changing this test (given that otherwise it wouldn't run on TV at all)?

DOMLocalization would return {id: "key1} when args are not set. We now changed that to be a bit more strict about the shape and always return {id, args} even when args are null. I had to update tests that deepEquals what document.l10n.getAttributes returns.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, I think this is bug 1409910. I launched the try build with logs, but it seems like the test needs more attention.

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Alternatively, I could just land this, it'll trigger the TV, but since the TV will disappear in the next run it shouldn't bother us too much. :(

Comment 26

[:Gijs (he/him)]

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

Alternatively, I could just land this, it'll trigger the TV, but since the TV will disappear in the next run it shouldn't bother us too much. :(

Yes, just do this. We can use bug 1409910 to work out why this fails under TV.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bffc5993b912
Extract DOMLocalization out of DocumentL10n. r=smaug
https://hg.mozilla.org/integration/autoland/rev/bb1d8fcccda5
Refactor constructors. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5f5924f91943
Move DocumentL10n to dom/l10n. r=smaug
https://hg.mozilla.org/integration/autoland/rev/cf6ce31cd14b
Merge L10nUtils.webidl into DOMOverlays.webidl. r=smaug
https://hg.mozilla.org/integration/autoland/rev/9504f0040d43
Move DOM L10n tests from intl/l10n to dom/l10n. r=fluent-reviewers,smaug,Pike
https://hg.mozilla.org/integration/autoland/rev/886ea903b3a6
Refactor constructors of mozILocalization and DOMLocalization to handle custom generateMessages. r=smaug
https://hg.mozilla.org/integration/autoland/rev/902e613c66a6
Move dom tests to use WebIDL DOMLocalization. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e752661a7915
Migrate all remaining callsites to use WebIDL DOMLocalization and remove mozIDOMLocalization and DOMLocalization.jsm. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3a5e7c585784
Remove dom::l10n namespace and unify class naming. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6161a2d18549
Fix tests to compare the result of getAttributes against L10nKeys. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6bbaacc9646a
Fix two tests which were caught not waiting for l10n but checking for l10n values. r=smaug

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bffc5993b912
https://hg.mozilla.org/mozilla-central/rev/bb1d8fcccda5
https://hg.mozilla.org/mozilla-central/rev/5f5924f91943
https://hg.mozilla.org/mozilla-central/rev/cf6ce31cd14b
https://hg.mozilla.org/mozilla-central/rev/9504f0040d43
https://hg.mozilla.org/mozilla-central/rev/886ea903b3a6
https://hg.mozilla.org/mozilla-central/rev/902e613c66a6
https://hg.mozilla.org/mozilla-central/rev/e752661a7915
https://hg.mozilla.org/mozilla-central/rev/3a5e7c585784
https://hg.mozilla.org/mozilla-central/rev/6161a2d18549
https://hg.mozilla.org/mozilla-central/rev/6bbaacc9646a