1245678 - create json schema for chrome.downloads

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Having a json schema will automate parameter validation, async interfaces, etc.

Comment 1

[Andrew Swan [:aswan]]

Attachment: boilerplate for chrome.downloads

Comment 2

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

Comment on attachment 8715877 [details] [diff] [review]
boilerplate for chrome.downloads

Review of attachment 8715877 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Andrew,
here is a couple of changes needed and a couple of ideas about alternative approaches for the new corner cases (e.g. the additional permissions) introduced by this API.

::: toolkit/components/extensions/Extension.jsm
@@ +188,5 @@
> +        let permissions = api.permission;
> +        if (!Array.isArray(permissions)) {
> +          permissions = [ permissions ];
> +        }
> +        if (!permissions.every(permission => extension.hasPermission(permission))) {

I would prefer to don't change this internal |generateAPI| helper method.
 
How about the strategy suggested in the "ext-downloads.js" review comments?

::: toolkit/components/extensions/ext-downloads.js
@@ +6,5 @@
> +const {
> +  ignoreEvent,
> +} = ExtensionUtils;
> +
> +extensions.registerSchemaAPI("downloads", (extension, context) => {

registerSchemaAPI has the signature: registerSchemaAPI(namespace, permission, api)

so this should be:

    extensions.registerSchemaAPI("downloads", "downloads", (extension, context) => {
      ...
    });

@@ +10,5 @@
> +extensions.registerSchemaAPI("downloads", (extension, context) => {
> +  return {
> +    downloads: {
> +      downloads(options, callback) {
> +        throw new Error("not yet implemented");

raise "not yet implemented" errors is not needed, because any method marked as unsupported in the schema will not be copied in the API object available to the extension contexts.

(I agree that unsupported API methods should raise an exception with an helpful error message, but we should work on it in a separate issue, where we can discuss it and implement this behaviour so that it works automatically on any APIs registered with a corresponding schema file)

@@ +14,5 @@
> +        throw new Error("not yet implemented");
> +      },
> +
> +      search(query, callback) {
> +        throw new Error("not yet implemented");

Errors raised from an API methods to an extension contexts needs to have the same principal as the extension context or the code from the extension will not being able to access the object (a security exception will be raised), as an example: 

       throw new context.cloneScope.Error("...")

@@ +65,5 @@
> +    },
> +  };
> +});
> +
> +extensions.registerSchemaAPI([ "downloads", "downloads.open" ], (extension, context) => {

With the changes you make in the generateAPI method in Extension.jsm, this should be:

  extensions.registerSchemaAPI("downloads", ["downloads", "downloads.open"], ...

But, instead of changing the generateAPI method in Extension.jsm and register 3 different API objects in ext-downloads.js, how about this alternative approach? (which works without any change to the generateAPI method):

we can define all the methods in a single API object and raise an exception from the |open| and |setShelfEnabled| methods if the extension doesn't have the additional permissions required:


extensions.registerSchemaAPI("downloads", "downloads", (extension, context) => {
      ....
      /* Additional permission needed: "download.open" */
      open(downloadId) {
        if (!extension.hasPermission("downloads.open")) {
          throw new context.cloneScope.Error("Permission denied because 'downloads.open' permission is missing.");
        }

        ....
      },

      /* Additional permission needed: "download.shelf" */
      setShelfEnabled(enabled) {
        if (!extension.hasPermission("downloads.shelf")) {
          throw new context.cloneScope.Error("Permission denied because 'downloads.shelf' permission is missing.");
        }

        ...
      },

::: toolkit/components/extensions/schemas/downloads.json
@@ +554,5 @@
> +          }
> +        ]
> +      },
> +      {
> +        "name": "showDefaultFolder",

unsupported field missing on showDefaultFolder API method

::: toolkit/components/extensions/schemas/manifest.json
@@ +116,5 @@
>            {
>              "type": "string",
>              "enum": [
>                "alarms",
> +              "downloads",

the values of Permissions defined in the head of |downloads.json| seems to be correctly merged to this list of choices, so it seems that there's no need to add "download" here.

::: toolkit/components/extensions/test/mochitest/test_ext_downloads.html
@@ +13,5 @@
> +<script type="text/javascript">
> +"use strict";
> +
> +function backgroundScript() {
> +  browser.test.assertTrue(!!chrome.downloads, "`downloads` API is present.");

no methods will be currently defined in chrome.downloads (because they are all marked as unsupported right now) but the enums should be correctly defined and injected based on the schema, maybe we can check that they are correctly defined as expected.

@@ +20,5 @@
> +
> +let extensionData = {
> +  background: "(" + backgroundScript.toString() + ")()",
> +  manifest: {
> +    permissions: ["downloads"],

we should probably check that "downloads.open" and "downloads.shelf" are accepted as valid permissions (e.g. an extension which asks these additional permissions should not raise any error that would prevent it to start)

but in my own opinion we can plan to do it in a followup (e.g. the additional permissions test case), and it is possible that we will end up to not supporting "downloads.shelf" because we do not have this piece of UI on Firefox.

Comment 3

[Andrew Swan [:aswan]]

Attachment: boilerplate for chrome.downloads

Comment 4

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

Comment on attachment 8716376 [details] [diff] [review]
boilerplate for chrome.downloads

Review of attachment 8716376 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thanks Andrew

lgtm

Before landing the patch subject message should be in the form: "Bug NUMBER - DESCRIPTION. r=REVIEWER"
e.g. "Bug 1245678 - [webext] add downloads API schema and implementation boilerplate. r=rpl"

Follow a couple of very small nits that we can tweak before landing this first stone of the downloads API:

::: toolkit/components/extensions/ext-downloads.js
@@ +9,5 @@
> +
> +extensions.registerSchemaAPI("downloads", "downloads", (extension, context) => {
> +  return {
> +    downloads: {
> +      // when we do open(), check for additional downloads.open permission

Tweak the inline comment (common m-c convention): capitalize the first letter and end the comment with a dot '.'
e.g. // When we do |downloads.open()|, check for the additional "downloads.open" permission.

@@ +11,5 @@
> +  return {
> +    downloads: {
> +      // when we do open(), check for additional downloads.open permission
> +      open(downloadId) {
> +        if (!extension.hasPermission("downloads.open")) {

I would put this example snippet in the above comment and remove any code from this two "not yet implemented" methods (or even better, we can remove the entire two functions and leave all the information that we need just in the comment)

@@ +17,5 @@
> +        }
> +        throw new context.cloneScope.Error("not yet implemented");
> +      },
> +
> +      setShelfEnabled(enabled) {

as "downloads.shelf" is a different permission, it is probably better if we explicitly add a note about it in the above comment and remove this function definition.
(just to prevent us to forget about it if we decide to leave this unsupported but we change our mind in the future)

::: toolkit/components/extensions/test/mochitest/test_ext_downloads.html
@@ +25,5 @@
> +                          "`downloads.State` enum is present.");
> +  browser.test.notifyPass("downloads tests");
> +}
> +
> +let extensionData = {

I doubt that this |extensionData| and the |backgroundScript| above will be reused in other tests (they seems to be specific to this particular test case), 
in that case we can define this two objects inside |test_downloads| generator function below.

@@ +32,5 @@
> +    permissions: ["downloads", "downloads.open", "downloads.shelf"],
> +  },
> +};
> +
> +add_task(function* test_downloads() {

How about a more specific name? e.g. test_downloads_api_namespace_and_permissions

Comment 5

[Andrew Swan [:aswan]]

Attachment: [webext] add downloads API schema and implementation boilerplate

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da3b7f111d16

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/da3b7f111d16