Closed
Bug 1293132
Opened 8 years ago
Closed 8 years ago
The "context" type in Schemas.inject and Schemas.load are unclear
Categories
(WebExtensions :: Untriaged, defect)
Tracking
(firefox51 fixed)
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: robwu, Assigned: robwu)
References
Details
(Whiteboard: triaged)
Attachments
(1 file)
For a while I've been thinking that the "Context" in Schemas.jsm is related to ExtensionContext/BaseContext, because of the similar properties and the fact that Schemas.normalize is sometimes invoked with an ExtensionContext (see e.g. connectNative / sendNativeMessage).
Further, there are methods names that suggest that there is no return value (callFunctionNoReturn), yet the body contains a return statement that takes the return value from an arbitrary method.
These issues makes it more difficult to reason about the logic of schema-generated APIs, so I'm going to submit a patch that documents how they work as I understand it.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69882/
Attachment #8778725 -
Flags: review?(wmccloskey)
Updated•8 years ago
|
Whiteboard: triaged
Attachment #8778725 -
Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Depends on bug 1295473 to not fail unit tests (runtime.sendMessage is currently implemented using callFunctionNoReturn, while it should be handled by callAsyncFunction).
Depends on: 1295473
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize
https://reviewboard.mozilla.org/r/69882/#review69646
::: toolkit/components/extensions/Schemas.jsm:165
(Diff revision 2)
> + /**
> + * @param {nsIURI} url
> + * @returns {boolean} Whether the context may load |url|.
> + */
Please provide a description for the method and its parameters. Also, please use backticks rather than |...| around symbol names. Same for other methods.
::: toolkit/components/extensions/Schemas.jsm:166
(Diff revision 2)
> get principal() {
> return this.params.principal || Services.scriptSecurityManager.createNullPrincipal({});
> }
>
> + /**
> + * @param {nsIURI} url
The `url` parameter is a string, not a nsIURI.
::: toolkit/components/extensions/Schemas.jsm:248
(Diff revision 2)
> - * Logs the given error to the console. May be overridden to enable
> + * Logs the given error to the console.
> - * custom logging.
I don't see a reason for this change.
Attachment #8778725 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8778725 [details]
Bug 1293132 - Document and enforce contract for Schemas.inject and Schemas.normalize
https://reviewboard.mozilla.org/r/69882/#review69664
Attachment #8778725 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•8 years ago
|
||
Actually no checkin-needed yet. This patch is blocked on bug 1295473 to not fail tests as I explained in comment 3.
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
The other patch was pushed to autoland. Please land this one after the patch for 1295473 lands.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d71c400313a2
Document contract for Schemas.inject and Schemas.normalize r=kmag
Keywords: checkin-needed
Comment 12•8 years ago
|
||
Backed out for failing test_ext_schemas_api_injection.js on Android's Sets test:
https://hg.mozilla.org/integration/autoland/rev/1c6f07b604f671e7a1844e6351e4b7174b58b2ef
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=2150586&repo=autoland
Flags: needinfo?(rob)
Assignee | ||
Comment 13•8 years ago
|
||
Failure is caused by a different bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1295082#c29, no action needed here.
Flags: needinfo?(rob)
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
test_ext_schemas_api_injection.js failed because its schema declared a function without return value, while the actual API did return a value. I fixed the test by adding the correct return type and confirmed locally that it runs as expected.
Comment 16•8 years ago
|
||
Try push has test_ext_storage_tab.html failures still.
Keywords: checkin-needed
Assignee | ||
Comment 17•8 years ago
|
||
Test is looking good to me. The listed failures are caused by the absence of the patch for bug 1295473 on the try server. I ran the tests locally with that patch applied and the tests passed (and without the patch I see the same errors as on try).
Keywords: checkin-needed
Comment 18•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/38ad193a808c
Document and enforce contract for Schemas.inject and Schemas.normalize r=kmag
Keywords: checkin-needed
Comment 19•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•