Refactor Localization into DOMLocalization

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: stas)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment)

40 bytes, text/x-github-pull-request
gandalf
: review+
gandalf
: feedback+
Details | Review | Splinter Review
(Reporter)

Description

2 years ago
Follow up to bug 1304350

We want to update naming scheme based on :pike's decision and refactor the Localization class slightly according to Stas' proposal.
(Reporter)

Updated

2 years ago
Assignee: nobody → gandalf

Updated

2 years ago
Depends on: 1304350
(Reporter)

Updated

2 years ago
Blocks: 1315811
(Reporter)

Updated

2 years ago
Blocks: 1288443
(Assignee)

Updated

2 years ago
Summary: Apply renaming + refactor of Localization/DOMLocalization → Refactor Localization into DOMLocalization
(Assignee)

Comment 3

2 years ago
Created attachment 8810562 [details] [review]
Pull request

Here's a WIP.  I need to fix comments and docstrings, but wanted to get your early feedback on the general direction.  Let me know if you have any questions!
Assignee: gandalf → stas
Status: NEW → ASSIGNED
Attachment #8810562 - Flags: feedback?(gandalf)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8810562 [details] [review]
Pull request

Yeah, if you get good perf results I'm ok with this landing before release.
Attachment #8810562 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Comment 6

2 years ago
I updated the PR.  All tests now pass.  Here's a summary of changes.

 - The old LocalizationObserver class is no more.  Instead, there are two subclasses of Localization: DOMLocalization and DocumentLocalization.

 - DOMLocalization is a subclass of Localization and implements methods for translating DOM trees and elements, like translateFragment and translateElement.  Localization itself is environment-agnostic.  Right now it still sanitizes the arguments in a way that is specific to HTML, but I'll fix that in bug 1315975 or a follow-up.

 - DocumentLocalization is a subclass of DOMLocalization.  In HTML/XUL documents, an instance of DocumentLocalization will be available as document.l10n.  It implements the observer interface and supports delegating to sub-localizations. The sub-localizations are stored in the document.l10n.delegates map.

 - I considered factoring the observer interface into its own class or a mixin, but eventually decided to leave it as part of DocumentLocalization to keep things simple.

 - We used to have two methods related to observing roots:  observeRoot and disconnectRoot.  They would serve two purposes:  1) observe/unobserve the root via the MutationObserver instance and 2) add/remove the root to/from the list of known roots used by top-down methods like translateRoots.  In this PR, I separated these two purposes into two pairs of methods:

   - observeRoot and unobserveRoot are available only on DocumentLocalization (because it implements the observer interface) and they only handle 1) from above; they aren't part of the public API,

   - connectRoot and disconnectRoot are available on every DOMLocalization (because now every DOMLocalization including DocumentLocalization has its own set of roots) and it handles 2) from above;  it will also use observeRoot/unobserveRoot if they're available;  thus, connectRoot and disconnectRoot can be considered part of the public API.
(Assignee)

Comment 7

2 years ago
Comment on attachment 8810562 [details] [review]
Pull request

Zibi, this is now ready for your review.  See comment 6 for a summary of changes.
Attachment #8810562 - Flags: review?(gandalf)
(Reporter)

Comment 9

2 years ago
Comment on attachment 8810562 [details] [review]
Pull request

awesome work :stas! r+ :)
Attachment #8810562 - Flags: review?(gandalf) → review+
(Assignee)

Comment 11

2 years ago
https://hg.mozilla.org/projects/larch/rev/4de23d5b8758f314a92b3409087d6ba01ddb64dc
Bug 1316083 - Refactor Localization into DOMLocalization. r=gandalf
(Assignee)

Comment 12

2 years ago
Thanks for the review!
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Blocks: 1318086
You need to log in before you can comment on or make changes to this bug.