1425104 - [tracking] Add Fluent API to WebExtensions

Status: NEW

(Core :: Internationalization, enhancement, P3)

Description

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

This is a tracking bug for adding Fluent API into WebExtensions.

Comment 1

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

Here are docs on how to do this: https://bugzilla.mozilla.org/show_bug.cgi?id=1423425

Comment 2

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

Kris, I believe you said you'll be able to create a shell of an API for Stage 0 of https://docs.google.com/document/d/1s1ICaPMUpamiRYJaApj4yky5_VYvyW3luWZuEuCf8Jc/edit#heading=h.28eu2xksyi0j for us.

Setting a NI on you then :)

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1425104: Part 1a - Support unrestricted matching in MatchPattern.

Review commit: https://reviewboard.mozilla.org/r/238302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238302/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1425104: Part 1b - Split matching logic for content scripts into MozDocumentMatcher base class.

Review commit: https://reviewboard.mozilla.org/r/238304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238304/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1425104: Part 1c - Add MozDocumentObserver class to notify on new pattern-matched documents.

Review commit: https://reviewboard.mozilla.org/r/238306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238306/

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1425104: Part 2 - Add stub fluent API for built-in content pages.

This is all very rough and tentative at this point.

Review commit: https://reviewboard.mozilla.org/r/238308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238308/

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8969553 [details]
Bug 1425104: Part 2 - Add stub fluent API for built-in content pages.

This is all *very* rough at this point. Just need initial feedback before I go any further, and ideally some guidance on how to write sensible tests (because I have basically no idea).

Comment 8

[Tim Nguyen :ntim]

In bug 1386004, I only need to localize the manifest.json fields, will this API allow this ? Any reason not to integrate the browser.i18n WE API in the l10n workflow ?

Comment 9

[Axel Hecht [:Pike]]

(In reply to Tim Nguyen :ntim from comment #8)
> In bug 1386004, I only need to localize the manifest.json fields, will this
> API allow this ? Any reason not to integrate the browser.i18n WE API in the
> l10n workflow ?

I filed bug 1457865 on that, which is rather orthogonal to this one.

i18n support still needs to be fleshed out, there's a dummy tracker in bug 1454248. I also added comments about the data-access aspects in bug 1386004 comment 14.

Comment 10

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

Comment on attachment 8969553 [details]
Bug 1425104: Part 2 - Add stub fluent API for built-in content pages.

Sorry it took me so long to look at this.

I was mostly trying to see if there's any overlap between bug 1455649 and this, but now I don't think so.

The code looks ok. It seems pretty twisted and a lot of wrapping/xrays boilerplate which I'd like to document a bit if I'm going to have to maintain it in intl/l10n, but it does what it should :)

One thing I'd like to brainstorm more is the name. ContentLocalization for me indicates a class responsible for localization of Content, which a) is pretty vague, and b) in context of Firefox UI often relates to content process or non-chrome UI.

Since this API is intended for WebExtensions, would WebExtLocalization make more sense?

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8969551 [details]
Bug 1425104: Part 1b - Split matching logic for content scripts into MozDocumentMatcher base class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238304/diff/1-2/

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8969552 [details]
Bug 1425104: Part 1c - Add MozDocumentObserver class to notify on new pattern-matched documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238306/diff/1-2/

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8969553 [details]
Bug 1425104: Part 2 - Add stub fluent API for built-in content pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238308/diff/1-2/

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8969551 [details]
Bug 1425104: Part 1b - Split matching logic for content scripts into MozDocumentMatcher base class.

https://reviewboard.mozilla.org/r/238304/#review257362

A DOM peer should obviously review this.  I'm not really clear how any of this works with (optionally) empty mExtension is MozDocumentMatcher.  

Also, what happened with part 1a?

::: dom/chrome-webidl/WebExtensionContentScript.webidl:87
(Diff revision 2)
> -  /**
> -   * A set of paths, relative to the extension root, of JavaScript scripts to
> -   * execute in matching pages.
>     */
> -  [Cached, Constant, Frozen]
> -  readonly attribute sequence<DOMString> jsPaths;
> +  [Constant]
> +  readonly attribute WebExtensionPolicy? extension;

Why is this on MozDocumentMatcher?  It doesn't seem to be set ever, but is used in several methods without a guard.

::: toolkit/components/extensions/WebExtensionContentScript.h:135
(Diff revision 2)
> -  {
> -    aPaths.AppendElements(mJsPaths);
> -  }
>  
>  
>    WebExtensionPolicy* GetParentObject() const { return mExtension; }

I don't understand what this is used for, but it looks a part of webidl GC/CC mechanics -- how would it work with an empty mExtension?

::: toolkit/components/extensions/WebExtensionPolicy.cpp:516
(Diff revision 2)
>        aDoc.URL().Spec().EqualsLiteral("about:blank") &&
>        aDoc.Principal() && aDoc.Principal()->GetIsNullPrincipal()) {
>      return true;
>    }
>  
>    if (mRestricted && mExtension->IsRestrictedDoc(aDoc)) {

How is this safe to do a possibly null mExtension?  Here and in other places?

::: toolkit/components/extensions/WebExtensionPolicy.cpp:569
(Diff revision 2)
>  {
>    return WebExtensionContentScriptBinding::Wrap(aCx, this, aGivenProto);
>  }
>  
>  
> -NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(WebExtensionContentScript,
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MozDocumentMatcher,

I'm guessing WebExtensionContentScript class just inherits all these CC helpers

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8969551 [details]
Bug 1425104: Part 1b - Split matching logic for content scripts into MozDocumentMatcher base class.

https://reviewboard.mozilla.org/r/238304/#review257362

> Why is this on MozDocumentMatcher?  It doesn't seem to be set ever, but is used in several methods without a guard.

It's set in the WebExtensionContentScript constructor

> I don't understand what this is used for, but it looks a part of webidl GC/CC mechanics -- how would it work with an empty mExtension?

It's part of the binding contract. It's used to determing which global the object belongs to. It's allowed to return null if the object doesn't belong to a particular global.

> How is this safe to do a possibly null mExtension?  Here and in other places?

`mRestricted → mExtension`

> I'm guessing WebExtensionContentScript class just inherits all these CC helpers

Yes

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8969551 [details]
Bug 1425104: Part 1b - Split matching logic for content scripts into MozDocumentMatcher base class.

https://reviewboard.mozilla.org/r/238304/#review257362

Part 1a landed in another bug

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8969551 [details]
Bug 1425104: Part 1b - Split matching logic for content scripts into MozDocumentMatcher base class.

https://reviewboard.mozilla.org/r/238304/#review258204

I'd still like an answer to why several of the properties (like mExtension, or mActiveTabPermission) only used in WebExtensionContentScript are also defined on MozDocumentMatcher.  It doesn't feel like a clean design.

(even if the reason is something like "less code repetition/simpler this way")

::: dom/chrome-webidl/WebExtensionContentScript.webidl:18
(Diff revision 2)
>     * URI, without reference to attributes such as `allFrames`.
>     */
>    boolean matchesURI(URI uri);
>  
>    /**
> -   * Returns true if the script matches the given URI and LoadInfo objects.
> +   * Returns true if the the given URI and LoadInfo objects match.

"the the"

Comment 18

[Tomislav Jovanovic :zombie]

Comment on attachment 8969552 [details]
Bug 1425104: Part 1c - Add MozDocumentObserver class to notify on new pattern-matched documents.

https://reviewboard.mozilla.org/r/238306/#review258232

r=me with a smoketest or two added (which would cover MozDocumentMatcher from previous patch as well).

::: dom/chrome-webidl/MozDocumentObserver.webidl:7
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +callback interface MozDocumentCallback {

The name seems a little too generic for the global namespace.  `MozMatcherCallback` and `MozMatcherObserver` perhaps?

::: dom/chrome-webidl/MozDocumentObserver.webidl:15
(Diff revision 2)
> +};
> +
> +[ChromeOnly, Constructor(MozDocumentCallback callbacks), Exposed=System]
> +interface MozDocumentObserver {
> +  [Throws]
> +  void observe(sequence<MozDocumentMatcher> matchers);

Please add comments describing the api, especially if it's something non-obvious.  It seems [from below] `observe()` can only be called once without a call to `disconnect()`.

If that is by design, then I don't understand the purpose of these methods, since all the config could be passed on in the constructor, and a new object created when new matchers are required.

Also, it's  surprising because the similarly-named and similarly-shaped MutationObserver api behaves differently.

::: toolkit/components/extensions/ExtensionPolicyService.cpp:362
(Diff revision 2)
>  
>    CheckContentScripts(aWindow, false);
>  }
>  
>  void
>  ExtensionPolicyService::CheckContentScripts(const DocInfo& aDocInfo, bool aIsPreload)

This now needs a better name, or at least a comment.

::: toolkit/components/extensions/WebExtensionPolicy.cpp:601
(Diff revision 2)
> +
> +void
> +DocumentObserver::Observe(const dom::Sequence<OwningNonNull<MozDocumentMatcher>>& matchers, ErrorResult& aRv)
> +{
> +  if (!EPS().RegisterObserver(*this)) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Hm, does this throw the second time `observe()` is called?

::: toolkit/components/extensions/WebExtensionPolicy.cpp:603
(Diff revision 2)
> +DocumentObserver::Observe(const dom::Sequence<OwningNonNull<MozDocumentMatcher>>& matchers, ErrorResult& aRv)
> +{
> +  if (!EPS().RegisterObserver(*this)) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +  }
> +  mMatchers.Clear();

Doesn't this belong in `Disconnect`?  Otherwise, is it holding matchers alive after a call to `disconnect()`?

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

With bug 1455649, what work is needed for this bug?

Comment 20

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

I didn't get to it because of bug 1455649, but Andrew is interested in picking up some design decisions around system addons localization. Here are live links to the patches that Kris wrote:

https://hg.mozilla.org/mozreview/gecko/rev/466c39fc8af5170ea39d19d6d7aa13b8fe5cd838#index_header
https://hg.mozilla.org/mozreview/gecko/rev/52562e8d57a8d2973cfaca9c350b6727996df39f#index_header
https://hg.mozilla.org/mozreview/gecko/rev/fe8af2cc822f1e645a88cbc81bcfae009bd110ac#index_header

Comment 21

[David Durst [:ddurst]]

(Moved bug 1451485 to see also because the block is worked around.)

Comment 22

[Andrew Swan [:aswan]]

Attachment: Bug 1425104: Add stub fluent API for built-in content pages.

Comment 23

[Andrew Swan [:aswan]]

:gandalf, I just rebased and fixed up this patch a bit. The test isn't passing yet, but since you've done a bunch of work on document.l10n is this patch a dead end? ie, would we be better off extending document.l10n to extension pages? Do you have a rough idea how much work that would entail?

Comment 24

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

I think this is more a question for Mossop. He wrote the document.l10n exposed to content approach. I'm not familiar with WebExt to understand if they always have a document associate with them that they can leverage or if this is the best approach.

Comment 25

[Axel Hecht [:Pike]]

A couple of thoughts:

For web-extensions, we don't want them to have access to Firefox' Fluent files. Which means that they'll likely want their own instance of an L10nRegistry.

For system add-ons, we might want to use Firefox Fluent files, as that's easier for l10n repacks.

We probably want to expose a DOMLocalization, and not just a Localization object?

How much overlap is there between this, content built-in pages, and in-page UA widgets (bug 1504363) ?

Detail note on the patch, we dropped the "Context" terminology all over our Fluent code, we'll want to convert that to localization or bundle.

Comment 26

[Andrew Swan [:aswan]]

(In reply to Axel Hecht [:Pike] from comment #25)

For web-extensions, we don't want them to have access to Firefox' Fluent files. Which means that they'll likely want their own instance of an L10nRegistry.

For system add-ons, we might want to use Firefox Fluent files, as that's easier for l10n repacks.

Right, my immediate goal was just to get something working for system addons which sounds simpler to me since it doesn't entail using a separate L10nRegistry.

We probably want to expose a DOMLocalization, and not just a Localization object?

Yes, this was an old patch that was written before DOMLocalization existed, I think we should probably just drop it and expand DOMLocalization to other context (assuming that's actually feasible, I don't know much about how it is implemented internally)

Comment 27

[Andrew Swan [:aswan]]

Attachment: part1.patch

Comment 28

[Andrew Swan [:aswan]]

Attachment: part2.patch

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 9085778 [details] [diff] [review]
part1.patch

Moving this over to bug 1580816

Comment 30

[Dan Minor [:dminor]]

Comment on attachment 8969553 [details]
Bug 1425104: Part 2 - Add stub fluent API for built-in content pages.

Clearing old review flag so this doesn't show up in triage.