1269347 - Expose the optional embedded webextension as a builtin SDK module

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 => {
>   ...
> });

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1269347 - Part1 Expose the optional embedded webextension as a builtin SDK module.

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

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

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

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1269347 - Part3 Add SDK with embedded webextension test addon.

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

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1269347 - Part4 [webext] Add more hybrid (SDK or bootstrap) addon tests.

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

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

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)

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

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)

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

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.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/3-4/

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/3-4/

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/3-4/

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 20

[Dave Townsend [:mossop]]

Comment on attachment 8756888 [details]
Bug 1269347 - Part2 Add support for embedded webextension in SDK test addons.

https://reviewboard.mozilla.org/r/55460/#review77348

Comment 21

[Gabor Krizsanits (INACTIVE)]

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.

Comment 22

[Gabor Krizsanits (INACTIVE)]

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

Comment 23

[Gabor Krizsanits (INACTIVE)]

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 24

[Gabor Krizsanits (INACTIVE)]

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.

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/4-5/

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/4-5/

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

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/4-5/

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try of the updated version of these patches:

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70c93b940d71
https://hg.mozilla.org/mozilla-central/rev/013eb78e6709
https://hg.mozilla.org/mozilla-central/rev/28459fc537c7

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 33

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 34

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 35

[Gerry Chang [:gchang]]

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?

Comment 36

[Luca Greco [:rpl] [:luca] [: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.

Comment 37

[Andy McKay]

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.

Comment 38

[Gerry Chang [:gchang]]

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 39

[Gerry Chang [:gchang]]

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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47f67174f70d
https://hg.mozilla.org/releases/mozilla-aurora/rev/308ca5e672dc
https://hg.mozilla.org/releases/mozilla-aurora/rev/2bbebba2ef17

Comment 41

[Will Bamberg [:wbamberg]]

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.

Comment 42

[Luca Greco [:rpl] [:luca] [: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).

Comment 43

[Will Bamberg [:wbamberg]]

Thanks Luca, I've added that note.