Closed Bug 1290901 Opened 8 years ago Closed 8 years ago

Fix devtools APIs schema incompatibilities

Categories

(WebExtensions :: Developer Tools, defect, P1)

Product:
WebExtensions
Component:
Developer Tools
Type:
defect

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Iteration:
51.1 - Aug 15
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [devtools.inspectedWindow][devtools.network][devtools.panels] triaged)

Attachments

(4 files)

Bug 1290901 - [webext] Handle nested namespaced API schema.
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.
58 bytes, text/x-review-board-request
aswan
: review+
Details 
This goal of this issue is to describe and implement the changes to the WebExtensions internals that handle the schema loading and the wrapping of the injected api needed to be able to load and use the devtools API's JSON schema files mostly unmodified (besides marking as `unsupported` what it is not going to be part of the initial subset of DevTools API, and as `async` any method which takes a callback).

Follows a brief list of the incompatibilities between the current "WebExtensions API JSON schema internals" and the syntaxes used in the DevTools API schemas [1]:

- the DevTools schema is not contained in a single schema file, on the contrary each of the namespaces nested in the `devtools` namespace has its own schema file which uses a nested namespace (e.g. `devtools.inspectedWindow`, `devtools.panels`, `devtools.network`)

- the custom "types" defined in the DevTools schemas have a set of "events" and "properties" (and currently we are support only set of "functions", which works correctly, but we are currently raising exceptions if "events" or "properties" are defined in a custom type)   

[1] The original schema file from the chromium repo: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/devtools/
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: -- → P1
Whiteboard: [devtools] triaged
Whiteboard: [devtools] triaged → [devtools.inspectedWindow][devtools.network][devtools.panels] triaged
Blocks: 1211859
needed to be able to return null when the registered schema API should not be available
based on the context type instead of WebExtensions permissions.

Review commit: https://reviewboard.mozilla.org/r/68960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68960/
The set of 4 patches attached are composed by:
 
- 3 small patches that introduces the needed changes (splitted into smaller patches to make it easier to figure out what is the goal of each of the patches), 

- the last patch introduces new xpcshell tests to check that the changes are working as expected (in particular that the nested namespaces becomes nested objects and if a registered API implementation returns a null or an empty object, the API is not injected in that context)

Push to try of the above set of 4 patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=785c9c984e355d34865de6dd8fff6a379cbc8a94
Attachment #8777380 - Flags: feedback?(aswan)
Attachment #8777381 - Flags: feedback?(aswan)
Attachment #8777382 - Flags: feedback?(aswan)
Attachment #8777383 - Flags: feedback?(aswan)
https://reviewboard.mozilla.org/r/68956/#review66086

::: toolkit/components/extensions/Schemas.jsm:1633
(Diff revision 1)
> +      } else if (namespace.includes(".")) {
> +        // Special handling needed on any nested namespaced API (e.g devtools.inspectedWindow)
> +        // to turn `dest["nested.namespace"]` into `dest["nested"]["namespace"]`.
> +
> +        let apiObj = dest[namespace];
> +        delete dest[namespace];
> +
> +        let nsLevels = namespace.split(".");
> +        let currentObj = dest;
> +        for (let nsLevel of nsLevels.slice(0, -1)) {
> +          if (!currentObj[nsLevel]) {
> +            // Create the namespace level if it doesn't exist yet.
> +            currentObj = Cu.createObjectIn(currentObj, {defineAs: nsLevel});
> +          } else {
> +            // Move currentObj to the nested object if it already exists.
> +            currentObj = currentObj[nsLevel];
> +          }
> +        }
> +
> +        // Copy the apiObj as the final nested level.
> +        currentObj[nsLevels.pop()] = apiObj;

On first reading, I was wondering why you don't just do all this work before injecting.  But I guess that doing it this way means you don't need any special case cleanup code to clear the whole hierarchy if you end up not injecting anything into a nested namespace?
Attachment #8777380 - Flags: feedback?(aswan) → feedback+
https://reviewboard.mozilla.org/r/68956/#review66086

> On first reading, I was wondering why you don't just do all this work before injecting.  But I guess that doing it this way means you don't need any special case cleanup code to clear the whole hierarchy if you end up not injecting anything into a nested namespace?

Exactly, if the nested namespace is empty then it is cleared by the "cleaup code" that was already there in 1 step.

If it is not empty, then we turn the nested namespace into nested objects.
https://reviewboard.mozilla.org/r/68958/#review66110

::: toolkit/components/extensions/Schemas.jsm:1348
(Diff revision 1)
> -      checkTypeProperties("functions");
> +      // NOTE: "events" and "properties" are currently ignored, because they are used
> +      // in the DevTools schema files, but they are not currently used by anything in the
> +      // initial set of supported DevTools APIs. See Bug 1290901 for rationale.

Is there a separate bug to add full schema-level support for these fields?
Attachment #8777381 - Flags: feedback?(aswan) → feedback+
https://reviewboard.mozilla.org/r/68960/#review66114

Sorry I don't understand this one, can you give a concrete example of when this is needed?
https://reviewboard.mozilla.org/r/68960/#review66114

for sure! 

An example of its usage can be found in the [initial patch that implements the devtools APIs](https://reviewboard.mozilla.org/r/68966/diff/1#0).

basically, I'd like to be able to disable an API in a particular context by returning a null/empty implementation, e.g. the api can decide that the API should not be available for a particular context type (or even based on the extension):

```
extensions.registerSchemaAPI("mycontextspecificapi", (extension, context) => {
  // Filter out any contexts that should not have access to this API object
  if (!isThisTheExpectedTypeOfContextForThisAPI(context)) {
    return null;
  }
  
  return {
    mycontextspecificapi: {
      ...
    }
  };
});
      
```
https://reviewboard.mozilla.org/r/68958/#review66110

> Is there a separate bug to add full schema-level support for these fields?

I was waiting to open the issue (just to be sure that the plan that I'm proposing has been already double-checked at least once ;-)), I intended to open it after the initial feedback on this patch.

I'm going to create the new bugzilla issue and add its bug number into this comment as a reference.
Blocks: 1293301
Blocks: 1293298
Attachment #8777382 - Flags: feedback?(aswan) → feedback+
Attachment #8777383 - Flags: feedback?(aswan) → feedback+
Comment on attachment 8777381 [details]
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

https://reviewboard.mozilla.org/r/68958/#review66110

> I was waiting to open the issue (just to be sure that the plan that I'm proposing has been already double-checked at least once ;-)), I intended to open it after the initial feedback on this patch.
> 
> I'm going to create the new bugzilla issue and add its bug number into this comment as a reference.

added the bug numbers of the related follow up issue in the new version of the above inline comment.
side note: in the last update, the attached patches have been rebased, to adapt them to recent changes around the code where these patches are applied.
Blocks: 1291737
Comment on attachment 8777380 [details]
Bug 1290901 - [webext] Handle nested namespaced API schema.

https://reviewboard.mozilla.org/r/68956/#review68824

::: toolkit/components/extensions/Schemas.jsm:1656
(Diff revision 2)
>          }
>        }
>  
>        if (!Object.keys(obj).length) {
>          delete dest[namespace];
> +      } else if (namespace.includes(".")) {

Could you include a comment just noting that this needs to come after lines 1654-5 so that we don't accidentally inject empty intermediate nodes for nested namespaces?
I also think the flow would be marginally clearer if you put return after line 1655 and then a separate "if namespace.includes..." clause with the comment leading into it, but that's a matter of taste, feel free to leave it as is if you prefer it.
Attachment #8777380 - Flags: review?(aswan) → review+
Comment on attachment 8777381 [details]
Bug 1290901 - [webext] Allow "events" and "properties" in SubModuleType defined in JSON schema.

https://reviewboard.mozilla.org/r/68958/#review68826
Attachment #8777381 - Flags: review?(aswan) → review+
Comment on attachment 8777382 [details]
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

https://reviewboard.mozilla.org/r/68960/#review68828
Attachment #8777382 - Flags: review?(aswan)
Comment on attachment 8777382 [details]
Bug 1290901 - [webext] Allow APIs implementation to return null when the API should not be injected in a context.

https://reviewboard.mozilla.org/r/68960/#review68830
Attachment #8777382 - Flags: review+
Comment on attachment 8777383 [details]
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

https://reviewboard.mozilla.org/r/68962/#review68836

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_api_injection.js:86
(Diff revision 2)
> +    return null;
> +  });
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background() {
> +      if (browser.testnamespace.noBackgroundAPI) {

could you put the two schemas in different intermediate namespaces and test that the intermediate name doesn't get injected when its leaf object is empty?
i.e., i think you can just invert the order of testnamespace and {backgroundAPI,noBackgroundAPI} then test that browser.noBackgroundAPI doesn't exist.
Attachment #8777383 - Flags: review?(aswan) → review+
Comment on attachment 8777380 [details]
Bug 1290901 - [webext] Handle nested namespaced API schema.

https://reviewboard.mozilla.org/r/68956/#review68824

> Could you include a comment just noting that this needs to come after lines 1654-5 so that we don't accidentally inject empty intermediate nodes for nested namespaces?
> I also think the flow would be marginally clearer if you put return after line 1655 and then a separate "if namespace.includes..." clause with the comment leading into it, but that's a matter of taste, feel free to leave it as is if you prefer it.

Agree, I've update the patch to `continue` to the next namespace if the produced api object is empty (we can't use `return` because that `if` statement is inside a `for` loop that needs to complete to process all the defined API namespaces) and added a comment to explain that any non-empty nested namespace API objects will then be turned into nested objects.
Comment on attachment 8777383 [details]
Bug 1290901 - [webext] Add xpcshell test cases on nested namespaces and null/empty api objects.

https://reviewboard.mozilla.org/r/68962/#review68836

> could you put the two schemas in different intermediate namespaces and test that the intermediate name doesn't get injected when its leaf object is empty?
> i.e., i think you can just invert the order of testnamespace and {backgroundAPI,noBackgroundAPI} then test that browser.noBackgroundAPI doesn't exist.

Suggested change applied to the updated version of this patch.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8f56101b72b
[webext] Handle nested namespaced API schema. r=aswan
https://hg.mozilla.org/integration/autoland/rev/33b059c68484
[webext] Allow "events" and "properties" in SubModuleType defined in JSON schema. r=aswan
https://hg.mozilla.org/integration/autoland/rev/d28428dae62b
[webext] Allow APIs implementation to return null when the API should not be injected in a context. r=aswan
https://hg.mozilla.org/integration/autoland/rev/90418dcd93db
[webext] Add xpcshell test cases on nested namespaces and null/empty api objects. r=aswan
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: