Closed Bug 1295082 Opened 8 years ago Closed 8 years ago

Follow-up fix for WebExtension experiments prototype

Categories

(WebExtensions :: Untriaged, defect)

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

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

Attachments

(3 files)

Bug 1295082 - Put Extension in BaseContext
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1295082 - Minor fixups for experimental WebExtensions APIs
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id
58 bytes, text/x-review-board-request
kmag
: review+
Details 
- 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 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.
Attachment #8781035 - Flags: review?(kmaglione+bmo) → review+
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 on attachment 8781036 [details]
Bug 1295082 - Minor fixups for experimental WebExtensions APIs

https://reviewboard.mozilla.org/r/71540/#review69578
Attachment #8781036 - Flags: review?(kmaglione+bmo)
Attachment #8781715 - Flags: review?(kmaglione+bmo)
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.
Attachment #8781036 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8781715 [details]
Bug 1295082 - BaseContext.extensionId -> BaseContext.extension.id

https://reviewboard.mozilla.org/r/72056/#review69588
Attachment #8781715 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebased
Keywords: checkin-needed
Rebased again. Bots were all-green for the previous rebase in comment 22: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58bea5a0c819
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(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
Thanks for the info Luca. I updated the test and started another try (not expecting it to fail, so adding checkin-needed).
Flags: needinfo?(rob)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: