Open Bug 1425104 Opened 7 years ago Updated 2 years ago

[tracking] Add Fluent API to WebExtensions

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

People

(Reporter: zbraniecki, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(1 file, 6 obsolete files)

Bug 1425104: Part 2 - Add stub fluent API for built-in content pages.
(deleted), text/x-review-board-request
Details
This is a tracking bug for adding Fluent API into WebExtensions.
Blocks: 1365426
Priority: -- → P3
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 :)
Flags: needinfo?(kmaglione+bmo)
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).
Flags: needinfo?(kmaglione+bmo)
Attachment #8969553 - Flags: feedback?(gandalf)
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 ?
(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 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?
Attachment #8969553 - Flags: feedback?(gandalf) → feedback+
Attachment #8969550 - Attachment is obsolete: true
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
Attachment #8969551 - Flags: review?(tomica)
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 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 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"
Attachment #8969551 - Flags: review+
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()`?
Attachment #8969552 - Flags: review?(tomica) → review+
With bug 1455649, what work is needed for this bug?
Flags: needinfo?(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
Flags: needinfo?(gandalf)
Depends on: 1457865
Blocks: 1451485
(Moved bug 1451485 to see also because the block is worked around.)
No longer blocks: 1451485
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1451485
Attachment #8969551 - Attachment is obsolete: true
Attachment #8969552 - Attachment is obsolete: true

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

Flags: needinfo?(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.

Flags: needinfo?(gandalf) → needinfo?(dtownsend)

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.

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

Attachment #9083821 - Attachment is obsolete: true
Blocks: 1568270
Attached patch part1.patch (obsolete) (deleted) — — Splinter Review
Attached patch part2.patch (obsolete) (deleted) — — Splinter Review
Depends on: 1580816
Comment on attachment 9085778 [details] [diff] [review] part1.patch Moving this over to bug 1580816
Attachment #9085778 - Attachment is obsolete: true
Attachment #9085779 - Attachment is obsolete: true
Flags: needinfo?(dtownsend)
No longer blocks: 1568270
Depends on: 1492593

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.

Attachment #8969553 - Flags: review?(zbraniecki)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: