Closed Bug 1262250 Opened 6 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Iteration:
52.1 - Oct 3
Tracking Status
firefox52 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [schema] triaged)

Attachments

(1 file)

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
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
Flags: needinfo?(kmaglione+bmo)
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.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 18
Flags: blocking-webextensions-
Adding bug 1225715 as a dependency so I remember to check that out when I come back to work on this.
Depends on: 1225715
Blocks: 1261963
Sounds right.
Flags: needinfo?(kmaglione+bmo)
Andy suggested I hold off on this as there's a possibility we may do something different with schemas in the future.
Iteration: 48.3 - Apr 25 → ---
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.
It's not a priority or a blocker though, also the possibility of WebIDL schema at some point.
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.
Iteration: --- → 48.3 - Apr 25
Priority: -- → P3
Whiteboard: [schema] triaged
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.
Iteration: 48.3 - Apr 25 → 52.1 - Oct 3
Summary: Add a defaultValue property to the web extension schema → Add a default property to the web extension schema for function parameters
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)
Attachment #8793343 - Flags: review?(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'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 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 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 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 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.
Attachment #8793343 - Flags: review?(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?
Attachment #8793343 - Flags: review?(aswan) → review+
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 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 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 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 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 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?
Attachment #8793343 - Flags: review?(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)
Attachment #8793343 - Flags: review+
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
Attachment #8793343 - Flags: review?(aswan) → review+
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
Try looks good, requesting check in.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b73442fc24c
Add a defaultValue property to the web extension schema, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b73442fc24c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.