Closed Bug 1269347 Opened 8 years ago Closed 7 years ago

Expose the optional embedded webextension as a builtin SDK module

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(3 files, 1 obsolete file)

Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.
58 bytes, text/x-review-board-request
gkrizsanits
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.
58 bytes, text/x-review-board-request
mossop
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1269347 - Part3 Add SDK with embedded webextension test addon.
58 bytes, text/x-review-board-request
gkrizsanits
: review+
gchang
: approval-mozilla-aurora+
Details 
Once Bug 1269342 will expose a minimal API to exchange messaging with a webextension embeddded into a classic extension, the SDK should provide a builtin module which returns the API object received by the builtin bootstrap method (SDK-based extensions don't usually define their own custom bootstrap.js file, but use a builtin default bootstrap file).

e.g. changes to the SDK builtin bootstrap file:

> diff --git a/addon-sdk/source/lib/sdk/addon/bootstrap.js > b/addon-sdk/source/lib/sdk/addon/bootstrap.js
> --- a/addon-sdk/source/lib/sdk/addon/bootstrap.js
> +++ b/addon-sdk/source/lib/sdk/addon/bootstrap.js
> @@ -118,17 +118,21 @@ Bootstrap.prototype = {
>         paths: Object.assign({
>           "": "resource://gre/modules/commonjs/",
>           "devtools/": "resource://devtools/",
>           "./": baseURI
>         }, readPaths(id)),
>         manifest: metadata,
>         metadata: metadata,
>         modules: {
>-          "@test/options": {}
>+          "@test/options": {},
>+          "embedded-webextension-api": addon.embeddedWebextensionAPI ||
>+            Promise.reject(new Error("No 'embedded-webextension-api' module available. " +
>+                                     "('enableEmbeddedWebextension' option not enabled" +
>+                                     "in the install.rdf)"))
>         },

so that the addon developer can require it into its addon modules:

> const embeddedWebext = require('embedded-webextension-api');
> embeddedWebext.onConnect.addListener(port => {
>   ...
> });
Blocks: 1252227
Whiteboard: triaged
Depends on: 1269342
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment on attachment 8756890 [details]
MozReview Request: Bug 1269347 - Part4 [webext] Add more hybrid (SDK or bootstrap) addon tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55464/diff/1-2/
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55458/diff/1-2/
Comment on attachment 8756888 [details]
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55460/diff/1-2/
Comment on attachment 8756889 [details]
Bug 1269347 - Part3 Add SDK with embedded webextension test addon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55462/diff/1-2/
Attachment #8756890 - Attachment is obsolete: true
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

This first patch in this queue introduces the minimal changes needed to integrate into the SDK the "Embedded WebExtension" feature.

it defines a new small SDK module wrapper for the API object and it introduces the minimal changes needed in the SDK bootstraping phase to retrieve the API object provided as a bootstrap parameter.

As additional context about this change, this bugzilla issues provides the patches related to the step 3 of the "Embedded WebExtensions" roadmap (as briefly described in the following document: https://docs.google.com/document/d/1NhcfqEnPVNdvHys6d1CnrMGxnSpMQDCY7Q2v_TwysZY/edit#heading=h.fmpa7q13wjas).
Attachment #8756886 - Flags: review?(gkrizsanits)
Comment on attachment 8756888 [details]
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

Unfortunately the SDK test addons are currently built using the old cfx tool, and for this reason:

- they are using a different bootstrap.js file, which needs the same changes that we have introduced in the new one.
- the test addons xpi files are build during the "./mach build" step, and the install.rdf and the xpi file is build during that step using the old cfx tool

To be able to create a test addon inside the SDK test suite, this patch introduces the minimal changes to be able to:

- include the "webextension/" dir in the top-level dir of the xpi file

- generate an install.rdf file which contains the expected "enableEmbeddedWebExtension" RDF property (if the test addon's package.json file contains a "enableEmbeddedWebExtension" JSON property set to true)
Attachment #8756888 - Flags: feedback?(gkrizsanits)
Comment on attachment 8756889 [details]
Bug 1269347 - Part3 Add SDK with embedded webextension test addon.

This last patch introduces a minimal SDK test addon which contains an embedded webextension (more tests are going to be added in the WebExtensions test suite which provides nicer helpers to create more complex test scenarios, e.g. as in the patch attached to Bug 1252227 https://reviewboard.mozilla.org/r/56912/diff/1#1)
Attachment #8756889 - Flags: feedback?(gkrizsanits)
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55458/diff/2-3/
Attachment #8756886 - Attachment description: MozReview Request: Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module. → Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.
Attachment #8756888 - Attachment description: MozReview Request: Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons. → Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.
Attachment #8756889 - Attachment description: MozReview Request: Bug 1269347 - Part3 Add SDK with embedded webextension test addon → Bug 1269347 - Part3 Add SDK with embedded webextension test addon
Attachment #8756886 - Flags: review?(gkrizsanits)
Attachment #8756888 - Flags: feedback?(gkrizsanits)
Attachment #8756889 - Flags: feedback?(gkrizsanits)
Comment on attachment 8756888 [details]
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55460/diff/2-3/
Comment on attachment 8756889 [details]
Bug 1269347 - Part3 Add SDK with embedded webextension test addon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55462/diff/2-3/
Minor updates on the attached patches due to the renaming of the RDF property from "enableEmbeddedWebExtension" into "embeddedWebExtension", as it has been suggested during the presentation and the related discussion during MozLondon.
The last update on the patches attached to this issues contains:

- changes to the "sdk/webextension" SDK module, based on the last version of the dependencies which have been recently landed on mozilla-central (Bug 1252215 and Bug 1269342)
- all patches rebased on a recent mozilla-central tip

This issue is splitted in 3 patches:

- the first patch defines the new SDK "sdk/webextension" module, and it initializes the method using the bootstrap method params (from the new SDK module loader, the one which is currently used when the addons are built using the jpm tool)

- the second patch contains changes to the old cfx tool (and its related module loader), so that we can add an "SDK hybrid addon" as part of the SDK test addons in "addon-sdk/source/test/addons" dir

- the third patch adds a new test addon to test the behavior of the new SDK "sdk/webextension" module, and that an SDK module can exchange messages with a background page and a content script using the provided API object
Comment on attachment 8756888 [details]
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

https://reviewboard.mozilla.org/r/55460/#review77348
Attachment #8756888 - Flags: review?(dtownsend) → review+
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

https://reviewboard.mozilla.org/r/55458/#review54718

::: addon-sdk/source/lib/sdk/webextension.js:23
(Diff revision 2)
> +  },
> +
> +  get api() {
> +    if (!embeddedWebExtensionAPI) {
> +      let err = new Error(
> +        `The 'sdk/webextension' is currently disabled. ${HELP_MSG}`

What's the point of splitting up this string?

::: addon-sdk/source/lib/sdk/webextension.js:11
(Diff revision 4)
> +
> +module.metadata = {
> +  "stability": "experimental"
> +};
> +
> +const HELP_MSG = `('hasEmbeddedWebExtension' option not enabled in the install.rdf)`;

I'm not sure what is the reason for breaking up this string.
Attachment #8756886 - Flags: review?(gkrizsanits)
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

https://reviewboard.mozilla.org/r/55458/#review78068
Attachment #8756886 - Flags: review+
Ehh... sorry for the spam. I accidentally clicked publish in the middle of the review. Anyway, I was going to r+ and mention that warning related question only once...
Comment on attachment 8756889 [details]
Bug 1269347 - Part3 Add SDK with embedded webextension test addon.

https://reviewboard.mozilla.org/r/55462/#review78072

Looks good to me.
Attachment #8756889 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

https://reviewboard.mozilla.org/r/55458/#review54718

> I'm not sure what is the reason for breaking up this string.

The only reason was that the message string was long. 

Based on the last review comments, in the updated version of this patch, I changed it to compose the message using two regular strings (instead of using a ES6 template strings like in the previous version).
Push to try of the updated version of these patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=05ce4f34767d
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70c93b940d71
Part1 Expose the optional embedded webextension as a builtin SDK module. r=krizsa
https://hg.mozilla.org/integration/autoland/rev/013eb78e6709
Part2 Add support for embedded webextension in SDK test addons. r=mossop
https://hg.mozilla.org/integration/autoland/rev/28459fc537c7
Part3 Add SDK with embedded webextension test addon. r=krizsa
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/70c93b940d71
https://hg.mozilla.org/mozilla-central/rev/013eb78e6709
https://hg.mozilla.org/mozilla-central/rev/28459fc537c7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

Approval Request Comment

[Feature/regressing bug #]: Bug 1269347

[User impact if declined]:
This first patch introduces a new SDK "sdk/webextension" module and the changes needed to integrate it in the SDK loader, so that the new SDK "sdk/webextension" module can have access to the value of the bootstrap parameters, that the XPIProvider use to provide the embedded webextension to hybrid add-ons.

Bootstrapped hybrid add-ons are going to be supported in Firefox 51, but developers of legacy SDK add-ons don't usually write their own install.rdf and bootstrap.js files (these files are usually generated and included automatically in the xpi file created with the jpm tool), and so, without these patches, developers of legacy SDK addons are going to choose to defer the transition of their SDK add-on to a WebExtensions or to use more manual workaround strategies (e.g. create their SDK hybrid addons by manually writing and including an install.rdf and a custom bootstrap.js file for their add -on, directly using the feature as already exposed to bootstrapped add-ons in Firefox 51, and then remove and/or rewrite that parts once Firefox 52 has been released).

[Describe test coverage new/current, TreeHerder]:
This set of patches contains a new SDK test addon as part of Attachment 8756889 [details], which specifically tests the SDK hybrid addons scenario.

[Risks and why]:
Thi patch defines a new SDK module and applies a small change on a very specific part of the current SDK internals (the new SDK module loader), these changes (and the new SDK module) are going to be activated and used only for addons that are explicitly choosing to embed a WebExtensions inside the legacy SDK add-on.
For the above reasons, the risks are going to be limited to users with SDK hybrid addons currently installed and enabled.

[String/UUID change made/needed]:
None
Attachment #8756886 - Flags: approval-mozilla-aurora?
Comment on attachment 8756888 [details]
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

Approval Request Comment

[Feature/regressing bug #]: Bug 1269347

[User impact if declined]:
This second path applies changes to the deprecated cfx build tool and the old SDK loader, because these two components are currently used to build and run the SDK test addons, and so this change is needed to be able to add a SDK test addons for the embedded webextension use case in Attachment 8756889 [details] 

Bootstrapped hybrid add-ons are going to be supported in Firefox 51, but developers of legacy SDK add-ons don't usually write their own install.rdf and bootstrap.js files (these files are usually generated and included automatically in the xpi file created with the jpm tool), and so, without these patches, developers of legacy SDK addons are going to choose to defer the transition of their SDK add-on to a WebExtensions or to use more manual workaround strategies (e.g. create their SDK hybrid addons by manually writing and including an install.rdf and a custom bootstrap.js file for their add -on, directly using the feature as already exposed to bootstrapped add-ons in Firefox 51, and then remove and/or rewrite that parts once Firefox 52 has been released).

[Describe test coverage new/current, TreeHerder]:
This set of patches contains a new SDK test addon as part of Attachment 8756889 [details], which specifically tests the SDK hybrid addons scenario.

[Risks and why]:
This patch apply changes which are limited to components that are still used in our test suite, but at the same time they have been already completely deprecated for the users for a while (e.g. no addon built with cfx and using the old module loader can be currently be submitted to AMO).
For the above reasons the risks of this patch is low, as its impact is limited to our build infrastructure and should not have no impact on real-world Firefox users.

[String/UUID change made/needed]:
None
Attachment #8756888 - Flags: approval-mozilla-aurora?
Comment on attachment 8756889 [details]
Bug 1269347 - Part3 Add SDK with embedded webextension test addon.

This is a "test only patch" (and it depends on Attachment 8756886 [details] and Attachment 8756888 [details]).


Approval Request Comment

[Feature/regressing bug #]: Bug 1269347

[User impact if declined]:
This third patch adds the new SDK test addon which specifically test the SDK hybrid addon scenario. 

Bootstrapped hybrid add-ons are going to be supported in Firefox 51, but developers of legacy SDK add-ons don't usually write their own install.rdf and bootstrap.js files (these files are usually generated and included automatically in the xpi file created with the jpm tool), and so, without these patches, developers of legacy SDK addons are going to choose to defer the transition of their SDK add-on to a WebExtensions or to use more manual workaround strategies (e.g. create their SDK hybrid addons by manually writing and including an install.rdf and a custom bootstrap.js file for their add -on, directly using the feature as already exposed to bootstrapped add-ons in Firefox 51, and then remove and/or rewrite that parts once Firefox 52 has been released).

[Describe test coverage new/current, TreeHerder]:
This patches contains the new SDK test addon, which specifically tests the SDK hybrid addons scenario:

Push to try of all the three patches attached to this issue (Attachment 8756886 [details], Attachment 8756888 [details] and Attachment 8756889 [details]) applied on aurora:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af2d9d66f00

[Risks and why]:
The risks of this patch are low, as it is a test only patch.

[String/UUID change made/needed]:
None
Attachment #8756889 - Flags: approval-mozilla-aurora?
Hi :rpl, 
Regarding to these patches, I have a few questions.
First, the "feature/regression" section in template should not be the bug itself. 
Second, In aurora phase, we are not supposed to uplift new features. From your introduction, I will consider "a new SDK "sdk/webextension module" as a new feature.
Third, do we have bug number for introducing a new SDK "sdk/webextension" module?
Forth, what if we let this ride the train on 52 and get the extra bake time?
Flags: needinfo?(lgreco)
Hi Gerry,
My apologies, follows additional informations about this request:

## additional informations related to the feature

This bug is the last piece of a feature that is already in Firefox 51:

- Bug 1252215 - Provide to other addon types a way to communicate with WebExtensions addons
- Bug 1269342 - XPIProvider should provide optional embedded webextension instance to classic extensions

In Firefox 51 the feature is easily accessible from bootstrapped addons because they always have to define their own bootstrap.js file and so they can easily access the bootstrap parameters directly,
on the contrary any SDK add-ons can still use this feature in Firefox 51, even without this last change, but it will be potentially more error prone because it requires a manually customized bootstrap.js file, which is not something that SDK addons developers do so often.

## additional informations related to the new SDK module

These changes add a new SDK module (and do not change anything on the existent SDK modules), but this new SDK module doesn't implement the actual feature, it wraps a feature that already exists in Firefox 51 so that it can be more easily accessible (and less error prone) for the SDK addons developers.

The riskier part of these patches is the change in the SDK module loader (which is where the bootstrap parameters are processed in a regular SDK addons) that initializes the new SDK module with the bootstrap parameters.

## additional informations related to the "not uplifted" scenario

It would be nice if the transitioning strategy could be provided to all the legacy addon types starting from the same version, but if not uplifted, we will change the documention to suggest the simpler (and less error prone) transition strategy (bootstrapped addons starting from Firefox 51 and SDK addons starting from Firefox 52), and to highlight it as clearly as possible in the docs so that the addon developer would know which version they can target more easily based of their addon types.
Flags: needinfo?(lgreco)
I'd like to chime in that we consider this feature key for add-ons developers allowing them to move away from old style add-ons that have problems with e10s and towards WebExtensions. As such this feature has a priority in my mind. I'm not sure that sdk/webextension is worthy of the designation new feature, but my knowledge of that is a bit limited and I'd be happy to discuss it more.
Hi :rpl,
Thanks for the all the detailed information which answer all my questions. In order to make life easier, let's take it in 51 aurora.
Comment on attachment 8756886 [details]
Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

This patch provides a transitioning strategy that could be applied to all the legacy addon types. Take it in 51 aurora.
Attachment #8756886 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8756888 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8756889 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This SDK module is already mentioned in the Embedded WebExtensions page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions#Starting_the_WebExtension

I've added a short SDK API page on it, too: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/webextension

I think this covers it, but please let me know if you can think of anything else.
Flags: needinfo?(lgreco)
Thanks Will, it looks great!

The only additonal note that it would be probably good to add in the "https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/webextension" MDN page is the initial version of Firefox where this module is actually available (even if it is already mentioned in the other MDN page linked above).
Flags: needinfo?(lgreco)
Thanks Luca, I've added that note.
Keywords: dev-doc-needed → dev-doc-complete
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.