Closed Bug 1552714 Opened 6 months ago Closed 6 months ago

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

Categories

(Core :: Internationalization, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(11 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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
Priority: -- → P2
Depends on: 1546432
Assignee: nobody → gandalf
Status: NEW → ASSIGNED

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?

Flags: needinfo?(bugs)

Depends on D32397

Flags: needinfo?(bugs)

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.

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

(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 :)

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. :(

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.

Flags: needinfo?(gijskruitbosch+bugs)

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)?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.

Flags: needinfo?(gandalf)

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

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. :(

(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.

Pushed by zbraniecki@mozilla.com:
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
Duplicate of this bug: 1507008
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.