1262250 - Add a default property to the web extension schema for function parameters

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

This would be a property that can be added to a parameter, the value of which would be used as the default value of the parameter if no value is passed in.

An example of its usage would be to add a `defaultValue` of `{}` to the `createData` parameter of `browser.windows.create` [1] so that a user could omit the `createData` parameter but a populated object would still be provided to the `create` function.

[1]https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/schemas/windows.json#284

Comment 1

[Bob Silverberg [:bsilverberg]]

It looks like, according to the spec [1], maybe this property should be called `default` and not `defaultValue`.

What do you think, Kris, and do you agree that this would apply to `parameter` nodes?

[1] http://json-schema.org/latest/json-schema-validation.html#anchor101

Comment 2

[Bob Silverberg [:bsilverberg]]

I'm going to take this for now, but prioritize it below the test coverage bugs that I'm trying to complete over the next 2 weeks.

Comment 3

[Bob Silverberg [:bsilverberg]]

Adding bug 1225715 as a dependency so I remember to check that out when I come back to work on this.

Comment 4

[Kris Maglione [:kmag]]

Sounds right.

Comment 5

[Bob Silverberg [:bsilverberg]]

Andy suggested I hold off on this as there's a possibility we may do something different with schemas in the future.

Comment 6

[Kris Maglione [:kmag]]

I don't think there's any reason to hold of on this. The only hard plans we have for the schemas at the moment is to upgrade to the v4 spec, which this matches.

Comment 7

[Andy McKay]

It's not a priority or a blocker though, also the possibility of WebIDL schema at some point.

Comment 8

[Kris Maglione [:kmag]]

It solves a lot of problems, though, and simplifies some code.

Even if we do wind up going with WebIDL at some point, I don't think it's going to be any time soon, and we'll still need the JSON schemas for native.js.

Comment 9

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 10

[Bob Silverberg [:bsilverberg]]

Hmm, I implemented this as `defaultValue`, but then went back and saw my comment that the spec lists it as simply `default`. I think I may have had an issue with that when implementing it, but I'll try changing my patch to use `default` instead.

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review79162

I think this could be simplified a bit by unifiying the default argument handling with the unspecified-optional-args-get-set-to-null handling.  That is, since optional parameters now have a "default" property, set it to null if it isn't specified in the schema, then when you're actually creating fixedArgs, just insert the default rather than checking for default and setting to null if it doesn't exist.

::: toolkit/components/extensions/Schemas.jsm:1488
(Diff revision 2)
> +          if (parameter.default) {
> +            fixedArgs[parameterIndex] = parameter.default;
> +          } else {

Is this actually desired?  I'm not that well acquainted with this code but it looks like it is executed when there is an explicit null or undefined passed into the function.  why would we override in that case?

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js:481
(Diff revision 2)
>    root.testing.foo(null, true);
> -  verify("call", "testing", "foo", [null, true]);
> +  verify("call", "testing", "foo", [99, true]);

Ah, this is the test corresponding to the previous comment.  This does not seem like desirable behavior to me (same with the following case)

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review79162

I'm not really following this. The second block of code that deals with the default value is only executed when the type of the argument does not match the type of the parameter, which is why the default handling is in two places. I wouldn't be surprised if this could be more streamlined, but given the code that's there now I'm not sure how to do that. Can you be a bit more specific about how you see it being improved?

> Is this actually desired?  I'm not that well acquainted with this code but it looks like it is executed when there is an explicit null or undefined passed into the function.  why would we override in that case?

I wasn't sure how we should handle this myself, so when I was first working on this code (several weeks ago), I asked Kris about it, and he said that he would like the default to override an explicit null or undefined. If you disagree then I guess we should have a debate about it. I suppose there's an argument to be made that if we are providing a default value then we do not intend for the parameter to ever have a value of null or undefined.

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review79162

I meant that you could store null instead of undefined in the default property and then get rid of the ifs on lines 1471 and 1488 and just set the fixed arg to paramaters[].default

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review79224

::: toolkit/components/extensions/Schemas.jsm:1459
(Diff revision 2)
>  
>    checkParameters(args, context) {
>      let fixedArgs = [];
>  
>      // First we create a new array, fixedArgs, that is the same as
> -    // |args| but with null values in place of omitted optional
> +    // |args| but with null values and/or default values

this is unnecessarily confusing, i think saying "with default values" is fine (null being the default value for a default value :) )

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review79162

Ah, I finally understand. I made this change in the latest commit.

> I wasn't sure how we should handle this myself, so when I was first working on this code (several weeks ago), I asked Kris about it, and he said that he would like the default to override an explicit null or undefined. If you disagree then I guess we should have a debate about it. I suppose there's an argument to be made that if we are providing a default value then we do not intend for the parameter to ever have a value of null or undefined.

I saw you discussed this with Kris on IRC and I believe this is the behaviour we are going to keep, so I'm marking this issue as dropped.

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80212

::: toolkit/components/extensions/Schemas.jsm:1295
(Diff revision 3)
>  
>          parameters.push({
> -          type: Schemas.parseSchema(param, path, ["name", "optional"]),
> +          type: Schemas.parseSchema(param, path, ["name", "optional", "default"]),
>            name: param.name,
>            optional: param.optional == null ? isCallback : param.optional,
> +          default: param.default == null ? undefined : param.default,

Why convert null to undefined here?  I think you're just changing it back to null down on line 1474 but I don't understand why you don't just take `param.default` here and be done with it.

::: toolkit/components/extensions/Schemas.jsm:1474
(Diff revision 3)
>          return false;
>        }
>  
>        let parameter = this.parameters[parameterIndex];
>        if (parameter.optional) {
> -        // Try skipping it.
> +        parameter.default = parameter.default || null;

I don't think this is right, if there is a default value of 0 or false, this will turn it into null.  In any case, why are you setting parameter.default at all here and not just reading it?

::: toolkit/components/extensions/Schemas.jsm:1487
(Diff revision 3)
>          return false;
>        }
>  
>        let arg = args[argIndex];
>        if (!parameter.type.checkBaseType(getValueBaseType(arg))) {
>          if (parameter.optional && (arg === null || arg === undefined)) {

Can we add a comment here explaining the behavior and also the fact that it is for compatibility with Chrome.

Comment 19

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80350

::: toolkit/components/extensions/Schemas.jsm:1474
(Diff revision 4)
>          return false;
>        }
>  
>        let parameter = this.parameters[parameterIndex];
>        if (parameter.optional) {
> -        // Try skipping it.
> +        fixedArgs[parameterIndex] = parameter.default;

why did you remove the comment?

::: toolkit/components/extensions/Schemas.jsm:1515
(Diff revision 4)
>        this.throwError(context, "Incorrect argument types");
>      }
>  
>      // Now we normalize (and fully type check) all non-omitted arguments.
>      fixedArgs = fixedArgs.map((arg, parameterIndex) => {
> -      if (arg === null) {
> +      if (arg === null || arg === undefined) {

why is this needed?

Comment 21

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80212

> Why convert null to undefined here?  I think you're just changing it back to null down on line 1474 but I don't understand why you don't just take `param.default` here and be done with it.

Good point, thanks.

> I don't think this is right, if there is a default value of 0 or false, this will turn it into null.  In any case, why are you setting parameter.default at all here and not just reading it?

I had to do this because of a check below that only checks for `null`, and not `undefined`, but by changing that check I was able to also change this as you suggest.

> Can we add a comment here explaining the behavior and also the fact that it is for compatibility with Chrome.

TBH, I'm not sure I understand why we've decided to do this, nor what it has to do with Chrome compatibility. How would you comment on it?

Comment 22

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 23

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80350

> why did you remove the comment?

It didn't seem particularly useful, and not even accurate anymore, now that we're dealing with default values.

> why is this needed?

It's not, anymore. :)

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80372

::: toolkit/components/extensions/Schemas.jsm:1489
(Diff revision 5)
>        let arg = args[argIndex];
>        if (!parameter.type.checkBaseType(getValueBaseType(arg))) {
> +        // For Chrome compatibility, use the default value if
> +        // null or undefined are explicitly passed as arguments
>          if (parameter.optional && (arg === null || arg === undefined)) {
> -          fixedArgs[parameterIndex] = null;
> +          fixedArgs[parameterIndex] = parameter.default;

We need to clone this value, so any callees that change it don't wind up changing it for future calls, too.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80372

> We need to clone this value, so any callees that change it don't wind up changing it for future calls, too.

(And please add a test to make sure that that's the case)

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review80350

> It didn't seem particularly useful, and not even accurate anymore, now that we're dealing with default values.

I think there's room for improvement but I think it remains both accurate and useful.
This is the code that deals with optional parameters that are omitted from the list of actual arguments, specifically the step where we try skipping an optional formal parameter.

Comment 27

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 28

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 29

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

aswan, I made a change to address parameters for events, and added a test for that as well. Do you mind taking another look for a final r?

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review81460

::: toolkit/components/extensions/Schemas.jsm:1476
(Diff revision 7)
>          return false;
>        }
>  
>        let parameter = this.parameters[parameterIndex];
>        if (parameter.optional) {
> -        // Try skipping it.
> +        // Set the optional parameter to the default for now

I prefer the original comment here.

::: toolkit/components/extensions/Schemas.jsm:1490
(Diff revision 7)
>        }
>  
>        let arg = args[argIndex];
>        if (!parameter.type.checkBaseType(getValueBaseType(arg))) {
> +        // For Chrome compatibility, use the default value if
> +        // null or undefined are explicitly passed as arguments

to be pedantic: "if null or undefined is explicitly passed but is not a valid argument in this position"

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js:90
(Diff revision 7)
>         name: "foo",
>         type: "function",
>         parameters: [
> -         {name: "arg1", type: "integer", optional: true},
> +         {name: "arg1", type: "integer", optional: true, default: 99},
>           {name: "arg2", type: "boolean", optional: true},
> +         {name: "arg3", type: "object", optional: true, properties: {

can you leave this out here and create a separate dedicated schema in testDefaults() below?  (the fact that this is so spread out is a little confusing)

Comment 31

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

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

Comment 32

[Andrew Swan [:aswan]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review81544

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js:1377
(Diff revisions 7 - 8)
>    do_check_eq(root.testing.prop3.sub_foo(), "overwritten again");
>    do_check_eq(countProp3SubFoo, 2);
>  });
>  
>  
> +let defaultsJson = [

this can move into the task below

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js:1378
(Diff revisions 7 - 8)
>    do_check_eq(countProp3SubFoo, 2);
>  });
>  
>  
> +let defaultsJson = [
> +  {namespace: "defaultsJson",

weird indentation here

Comment 33

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8793343 [details]
Bug 1262250 - Add a defaultValue property to the web extension schema,

https://reviewboard.mozilla.org/r/80096/#review81544

> this can move into the task below

True, it can, although I did it like this because it is consistent with the rest of the file, e.g., [1] and [2].

[1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js#809
[2] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js#976

> weird indentation here

As above, this is true, but I did it like this because it is consistent with the rest of the file, e.g., [1] and [2].

[1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js#810
[2] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js#977

Comment 34

[Bob Silverberg [:bsilverberg]]

Try looks good, requesting check in.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b73442fc24c
Add a defaultValue property to the web extension schema, r=aswan

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b73442fc24c