1295082 - Follow-up fix for WebExtension experiments prototype

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

- Experimental APIs are currently loaded in Management.generateAPIs, which is called twice for every API generation.
- The sandbox's addon ID is initialized to `${api}@experiments.addons.mozilla.org`, but |api| is an object, not a string. This should probably be |apiName|.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1295082 - Put Extension in BaseContext

ExtensionContext in Extension.jsm has |extension| as an instance member,
so use it instead of passing |extension| to registerSchemaAPI's
callback.

And to make sure that this pattern also works in content processes, move
the |extension| member to BaseContext.

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

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1295082 - Minor fixups for experimental WebExtensions APIs

- Remove unused registerAPI and registerPrivilegedAPI
- Generate the APIs just once.
- Fix typo in addon ID.

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

Comment 3

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

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

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

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

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

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

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

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

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71540/diff/3-4/

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71538/diff/3-4/

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71540/diff/4-5/

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

https://reviewboard.mozilla.org/r/71538/#review69566

::: toolkit/components/extensions/ExtensionUtils.jsm:172
(Diff revision 4)
>      this.checkedLastError = false;
>      this._lastError = null;
>      this.contextId = ++gContextId;
>      this.unloaded = false;
> -    this.extensionId = extensionId;
> +    this.extension = extension;
> +    this.extensionId = extension.id;

I'd rather just use `context.extension.id` where this is needed than define this alias. It's not worth saving one character.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

https://reviewboard.mozilla.org/r/71540/#review69576

::: toolkit/components/extensions/Extension.jsm:164
(Diff revision 5)
> -  // Called by an ext-*.js script to register an API. The |api|
> -  // parameter should be an object of the form:
> +  // Called by an ext-*.js script to register an API. The |api| parameter
> +  // should have a method |getAPI| that returns an object of the form:

There's no `api` parameter. Do you mean 'the `getAPI` parameter should be a function that returns...`

In any case, it would be better to rewrite this comment in JSDoc syntax.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

https://reviewboard.mozilla.org/r/71540/#review69578

Comment 13

[Rob Wu [:robwu]]

Attachment: Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id

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

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71538/diff/4-5/

Comment 15

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71540/diff/5-6/

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

https://reviewboard.mozilla.org/r/71540/#review69586

::: toolkit/components/extensions/Extension.jsm:167
(Diff revision 6)
>    },
>  
> -  // Called by an ext-*.js script to register an API. The |api|
> -  // parameter should be an object of the form:
> -  // {
> -  //   tabs: {
> +  /**
> +   * Called by an ext-*.js script to register an API.
> +   *
> +   * @param {string} namespace

Please add a description for this parameter.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8781715 [details]
Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id

https://reviewboard.mozilla.org/r/72056/#review69588

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 19

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71538/diff/5-6/

Comment 20

[Rob Wu [:robwu]]

Comment on attachment 8781715 [details]
Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id

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

Comment 21

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71540/diff/6-7/

Comment 22

[Rob Wu [:robwu]]

Rebased

Comment 23

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71538/diff/6-7/

Comment 24

[Rob Wu [:robwu]]

Comment on attachment 8781715 [details]
Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id

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

Comment 25

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71540/diff/7-8/

Comment 26

[Rob Wu [:robwu]]

Rebased again. Bots were all-green for the previous rebase in comment 22: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58bea5a0c819

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ce1cc39aa3d
Put Extension in BaseContext r=kmag
https://hg.mozilla.org/integration/autoland/rev/dc7b09331202
BaseContext.extensionId -> BaseContext.extension.id r=kmag
https://hg.mozilla.org/integration/autoland/rev/7c4c98ac6b3f
Minor fixups for experimental WebExtensions APIs r=kmag

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in test_ext_schemas_api_injection.js:

https://hg.mozilla.org/integration/autoland/rev/169273f710d8519ff36bb740935759d84d3df3ee
https://hg.mozilla.org/integration/autoland/rev/4ccd5f15a01d1eca664c94340fccf490b6a2ad30
https://hg.mozilla.org/integration/autoland/rev/cbe853256aae7038964204fbee48bd8298da3733

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7c4c98ac6b3f4ff180de8ead6500d520e06d4d0a
Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7c4c98ac6b3f4ff180de8ead6500d520e06d4d0a

Comment 29

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

(In reply to Rob Wu [:robwu] from comment #26)
> Rebased again. Bots were all-green for the previous rebase in comment 22:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=58bea5a0c819

Hey Rob,
There is a new "test_ext_schemas_api_injection.js" xpcshell tests, landed recently, which tests the `Management.registerAPI` method, and it needs the same changes applied to the other registered API (`context` as first parameter, and get `extension` from the `context` object in the function body):

- https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas_api_injection.js#50
- https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas_api_injection.js#67

Comment 30

[Rob Wu [:robwu]]

Comment on attachment 8781035 [details]
Bug 1295082 - Put Extension in BaseContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71538/diff/7-8/

Comment 31

[Rob Wu [:robwu]]

Comment on attachment 8781715 [details]
Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72056/diff/3-4/

Comment 32

[Rob Wu [:robwu]]

Comment on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71540/diff/8-9/

Comment 33

[Rob Wu [:robwu]]

Thanks for the info Luca. I updated the test and started another try (not expecting it to fail, so adding checkin-needed).

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc29a8095469
Put Extension in BaseContext r=kmag
https://hg.mozilla.org/integration/autoland/rev/60eff0d178eb
BaseContext.extensionId -> BaseContext.extension.id r=kmag
https://hg.mozilla.org/integration/autoland/rev/4fcdf2ca248c
Minor fixups for experimental WebExtensions APIs r=kmag

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc29a8095469
https://hg.mozilla.org/mozilla-central/rev/60eff0d178eb
https://hg.mozilla.org/mozilla-central/rev/4fcdf2ca248c

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/23c2ec5544b9
Fix merge bustage. a=bustage