1316083 - Refactor Localization into DOMLocalization

Status: RESOLVED FIXED

(L20n :: General, defect)

Description

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

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.

Comment 1

[Staś Małolepszy :stas]

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

Comment 2

[Staś Małolepszy :stas]

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

Comment 3

[Staś Małolepszy :stas]

Attachment: 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!

Comment 4

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

Comment on attachment 8810562 [details] [review]
Pull request

Yeah, if you get good perf results I'm ok with this landing before release.

Comment 5

[Staś Małolepszy :stas]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=553880b403b2

Comment 6

[Staś Małolepszy :stas]

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.

Comment 7

[Staś Małolepszy :stas]

Comment on attachment 8810562 [details] [review]
Pull request

Zibi, this is now ready for your review.  See comment 6 for a summary of changes.

Comment 8

[Staś Małolepszy :stas]

Unfortunately the windows and macos builds failed.  The linux builds show no perf impact of this change:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=3dd4ca4c35eb&newProject=try&newRevision=919b65e98ca7379dfa7aca2685fbf9bb12d818c2&framework=1&showOnlyImportant=0

Comment 9

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

Comment on attachment 8810562 [details] [review]
Pull request

awesome work :stas! r+ :)

Comment 10

[Staś Małolepszy :stas]

Landed in l20n.js: https://github.com/l20n/l20n.js/commit/a91721c72880f6ff626b3943d759c223bf7b140f

Comment 11

[Staś Małolepszy :stas]

https://hg.mozilla.org/projects/larch/rev/4de23d5b8758f314a92b3409087d6ba01ddb64dc
Bug 1316083 - Refactor Localization into DOMLocalization. r=gandalf

Comment 12

[Staś Małolepszy :stas]

Thanks for the review!