1263011 - [tracking] Create WebExtension Experiments

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andy McKay]

This is the tracking bug to create the WebExtension Experiment as laid out in:

https://wiki.mozilla.org/WebExtensions/Experiments

Primarily this covers:
* Create an experiment add-on that can contain WebExtensions APIs
* Allow another WebExtension add-on to declare it uses the experiment add-on
* Allow another WebExtension add-on to call the API in the experiment add-on as well as APIs in mozilla-central.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1263011: Part 1 - Refactor Extension and ExtensionData to use ES6 classes.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1263011: Part 2 - Implement WebExtensions Experiments prototype.

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8763322 [details]
Bug 1263011: Part 1 - Refactor Extension and ExtensionData to use ES6 classes.

https://reviewboard.mozilla.org/r/59548/#review57178

::: toolkit/components/extensions/Extension.jsm:1401
(Diff revision 1)
>    // Reads the locale file for the given Gecko-compatible locale code, or if
>    // no locale is given, the available locale closest to the UI locale.
>    // Sets the currently selected locale on success.
> -  initLocale: Task.async(function* (locale = undefined) {
> +  initLocale(locale = undefined) {
> +    // Ugh.
> +    let super_ = super.initLocale.bind(this);

I think you could make this incrementally neater by leaving the super call out of the async task and doing something like:
```
return Task.spawn(function*() {
  ...
}.bind(this)).then(locale => super.initLocale(locale));
```

Comment 4

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59550/#review57180

I'm part way through this but am calling it quits for tonight.  I'll pick up tomorrow but meta-comment is that this would be a lot easier to review with at least minimal documentation of how this all is meant to work.  Related to that, is the RELEASE_BUILD checking meant to be permanent or just until this feature has matured enough to use it widely.  (also, and I'm sure I can find the answer to this myself, but I'm not sure if RELEASE_BUILD means "a build on the release channel" or "a build on any channel that will be available to the public")

::: toolkit/components/extensions/Extension.jsm:922
(Diff revision 1)
> +        let match = /^(\w+)(?:\.(\w+)(?:\.\w+)*)?$/.exec(perm);
> +        if (!match) {
> +          whitelist.push(perm);
> +        } else if (match[1] == "experiments" && match[2]) {
> +          this.apiNames.add(match[2]);
> +        }

The regexp is simple enough but still not all that readable, I think this would be easier to follow if you just did `perm.split(".", 2)`
(you'd still want the original regexp to distinguish origins from permissions, thanks google for lumping these all together)

Comment 5

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59548/#review57178

> I think you could make this incrementally neater by leaving the super call out of the async task and doing something like:
> ```
> return Task.spawn(function*() {
>   ...
> }.bind(this)).then(locale => super.initLocale(locale));
> ```

This is only temporary. It can be removed once bug 1185106 lands.

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59550/#review57180

The prototype version will never be available in release channels. The version that does make it to release channels will probably look significantly different from this, and will require automatic installing/enabling of approved APIs. At that point, my plan is to continue allowing developers to install their own API extensions only in non-release builds (possibly behind a preference).

The RELEASE_BUILD flag means we're building from a release channel (which means mainline or beta). It's not set in Aurora or nightly builds.

> The regexp is simple enough but still not all that readable, I think this would be easier to follow if you just did `perm.split(".", 2)`
> (you'd still want the original regexp to distinguish origins from permissions, thanks google for lumping these all together)

I suppose, except that the second argument to String.split is useless.

Comment 7

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59550/#review57288

looking good so far but ExtensionAPI.jsm is missing from your patch?

::: toolkit/components/extensions/Extension.jsm:1350
(Diff revision 1)
>    }
>  
> +  readManifest() {
> +    return super.readManifest().then(manifest => {
> +      if (AppConstants.RELEASE_BUILD) {
> +        return manifest;

If we're on a release build but the manifest does reference experiments, we should log something rather than just silently ignoring it.

::: toolkit/components/extensions/ExtensionManagement.jsm:114
(Diff revision 1)
>    getSchemas() {
>      return this.schemas;
>    },
>  };
>  
> +var APIs = {

I think there's a high potential for confusion with this name, I don't love long names but something like ExperimentalAPIs would be more descriptive.

::: toolkit/components/extensions/Schemas.jsm:1612
(Diff revision 1)
> +  load(url) {
>      if (Services.appinfo.processType != Services.appinfo.PROCESS_TYPE_CONTENT) {
>        return readJSON(url).then(json => {
>          this.schemaJSON.set(url, json);
>  
>          let data = Services.ppmm.initialProcessData;
>          data["Extension:Schemas"] = this.schemaJSON;
>  
>          Services.ppmm.broadcastAsyncMessage("Schema:Add", {url, schema: json});
>  
> -        loadFromJSON(json);
> +        this.flushSchemas();
>        });
> -    } else {
> -      if (this.loadedUrls.has(url)) {
> -        return;
> -      }
> +    }
> -      this.loadedUrls.add(url);
> +  },
>  
> -      let schema = this.schemaJSON.get(url);
> -      loadFromJSON(schema);
> -    }
> +  unload(url) {
> +    this.schemaJSON.delete(url);
> +
> +    let data = Services.ppmm.initialProcessData;
> +    data["Extension:Schemas"] = this.schemaJSON;
> +
> +    Services.ppmm.broadcastAsyncMessage("Schema:Delete", {url});
> +
> +    this.flushSchemas();
>    },

I gather that load() and unload() may only be called from the main process?  In that case, can we be consistent here?  I.e., either get rid of the test on processType in load() or preferrably, put a check at the beginning of both load() and unload() and log an error if either is called from a content process?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:223
(Diff revision 1)
>    dictionary: 64,
>    experiment: 128,
>  };
>  
> +if (!AppConstants.RELEASE_BUILD)
> +  TYPES.apiextension = 256;

Ugh, this looks-like-a-bitfield-but-really-isnt thing is a mess (but not something to fix here of course)

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:315
(Diff revision 1)
> +    },
> +  });
> +
> +  yield promiseInstallAllFiles([addonFile]);
> +
> +  let addon = yield new Promise(resolve => AddonManager.getAddonByID("meh@experiment", resolve));

`promiseAddonByID()`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension.js:342
(Diff revision 1)
> +    name: "Meh API",
> +  });
> +
> +  yield promiseInstallAllFiles([addonFile]);
> +
> +  let addons = yield new Promise(resolve => AddonManager.getAddonsByTypes(["apiextension"], resolve));

create `promiseAddonsByTypes()` in `head_addons.js` and use it here?

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59550/#review57288

Urgh. Fixed.

> If we're on a release build but the manifest does reference experiments, we should log something rather than just silently ignoring it.

We don't silently ignore it. If we're on a release build, we don't load the schema for that permission pattern, and it gets logged as an unknown permission.

> I think there's a high potential for confusion with this name, I don't love long names but something like ExperimentalAPIs would be more descriptive.

I named it something like that originally, but I don't think, in the end, this will only be used for experimental APIs.

> I gather that load() and unload() may only be called from the main process?  In that case, can we be consistent here?  I.e., either get rid of the test on processType in load() or preferrably, put a check at the beginning of both load() and unload() and log an error if either is called from a content process?

This will never be called from the content process, since it's only called because of `unregisterAPI`. `load` is called from other places, so it's sometimes called from both processes.

> Ugh, this looks-like-a-bitfield-but-really-isnt thing is a mess (but not something to fix here of course)

Yeah, I have no idea why we wound up with this numbering scheme (or any numbering scheme, if it comes to it).

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8763323 [details]
Bug 1263011: Part 2 - Implement WebExtensions Experiments prototype.

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

Comment 10

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59550/#review57288

> I named it something like that originally, but I don't think, in the end, this will only be used for experimental APIs.

That sounds intriguing, mostly for my own curiosity, what else do you see it being used for?

> This will never be called from the content process, since it's only called because of `unregisterAPI`. `load` is called from other places, so it's sometimes called from both processes.

In that case it looks like you removed the code that handles `load()` in the content process?

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8763323 [details]
Bug 1263011: Part 2 - Implement WebExtensions Experiments prototype.

https://reviewboard.mozilla.org/r/59550/#review57342

This looks good.  But lets make sure we're on the same page here since this was all presented with no context:
- dependencies are enforced insofar as a webextension that requires an apiextension will not start if the apiextension is not enabled, but it is up to the user to manually install and enable the apiextension?
- apiextensions can create new namespaces in chrome/browser as well as change (but not remove) anything in an existing namespace.  if multiple apiextensions create or change some property, the last one to be loaded wins.
- The entire implementation of an apiextension must be contained within a single js file that defines a class called API that includes a method getAPI() that returns an object to be injected based on the accompanying schema.

That all seems reasonable for a prototype, but it would also be nice to see something written about next steps and longer terms plans just to put this all into context.

::: toolkit/components/extensions/Extension.jsm:226
(Diff revision 2)
>        api = api.api(extension, context);
>        copy(obj, api);
>      }
>  
> +    for (let api of extension.apis) {
> +      copy(obj, api.getAPI(context));

I would have expected constraints here on the names of the properties that the experiment can inject here.  Or is the intent that experiments might be used to add or modify existing properties of browser/chrome?

::: toolkit/components/extensions/test/xpcshell/test_ext_experiments.js:98
(Diff revision 2)
> +  });
> +
> +  let boringAddonFile = Extension.generateXPI("[e-mail]", {
> +    background() {
> +      if (browser.meh) {
> +        browser.meh.hello("Here I should not be");

I see the logic here where you load this first then check below that you see the message from the other extension and not this message, but why not be more direct and just call browser.test.fail() here?

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59550/#review57342

> I see the logic here where you load this first then check below that you see the message from the other extension and not this message, but why not be more direct and just call browser.test.fail() here?

Because `browser.test` doesn't exist here.

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ef2413dc1b4a116e34eceea6cb178520e509df5
Bug 1263011: Part 1 - Refactor Extension and ExtensionData to use ES6 classes. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbc19bfac55ae5a43b5c5cdd8cfeb52b21d0c91
Bug 1263011: Part 2 - Implement WebExtensions Experiments prototype. r=aswan

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6ef2413dc1b4
https://hg.mozilla.org/mozilla-central/rev/fdbc19bfac55

Comment 15

[The 8472]

> Keywords: dev-doc-needed

I have a few things which currently are only possible by using chrome-privileges and frame scripts. I would definitely be interested in a process where I could write webext abstractions around those myself and then submit them.

Comment 16

[Will Bamberg [:wbamberg]]

-> https://webextensions-experiments.readthedocs.io/en/latest/index.html

MDN's focus is cross-browser technology. Since Experiments are Firefox-only, the docs aren't on MDN.