Allow omitting optional parameters for Extensions API functions

Allow omitting optional parameters for Extensions API functions

BUG=29215
TEST=Existing browser tests that call API functions with omitted parameters. Still need to add browser tests and possibly some unit tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124898

[Aaron Boodman, 2012-02-06 01:30:52]

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/extens...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/extens...
chrome/renderer/extensions/chrome_v8_context.cc:64:
->Set(v8::String::New(kValidateAPI), v8::True());
Formatting should be:

v8::Local<v8::Object>::Cast(hidden)->Set(
    ... rest indented four ...

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:38:
chromeHidden.fullSignature = function(name, schemas) {
Function names should be a verb phrase, like getFullSignature(). Also, reading
the name and comments for this, I'm not sure what it does. Can you clarify the
naming?

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:51:
chromeHidden.matchCandidate = function(args, schemas) {
argumentsMatchSignature()?

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:59:
validator.validate(args[i], schemas[i]);
This does not do the right thing. It will check all the details of schemas[i]
and come up with wrong answers for the edge cases we talked about in the thread.

We only want to consider the coarse type here, like string/int/boolean/etc. You
can use the JSONSchemaValidator.getSchemaType() method.

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:65:
chromeHidden.getCandidates = function(schemas) {
Can we call this something other than 'candidate'. Maybe
getOverloadedSignatures() or something?

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:102: return
normalized_args;
Hm. This doesn't seem right. We never throw the validation errors.

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:531: try {
I think it would be a bit cleaner to just have validate() return true/false to
indicate whether it was able to find a signature.

Or alternately, separate out finding the signature from validate() entirely. So
this code would look like:

var matchedSignature =
chromeHidden.getFunctionSignature(this.definition.parameters, args);
if (!matchedSignature)
  throw new Error("Invocation doesn't match...");

chromeHidden.validate(args, matchedSignature);

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/resour...
chrome/renderer/resources/extensions/schema_generated_bindings.js:535:
chromeHidden.fullSignature(this.name,
Can we also include the signature that was actually called in the error message?

[Matt Perry, 2012-02-06 21:34:57]

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/extens...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://chromiumcodereview.appspot.com/9317072/diff/19/chrome/renderer/extens...
chrome/renderer/extensions/chrome_v8_context.cc:64:
->Set(v8::String::New(kValidateAPI), v8::True());
On 2012/02/06 01:30:52, Aaron Boodman wrote:
> Formatting should be:
> 
> v8::Local<v8::Object>::Cast(hidden)->Set(
>     ... rest indented four ...

btw, the reason for this is that Chrome style is to wrap with the operator at
the end of the preceding line. also acceptable is 
...Cast(hidden)->
    Set(...)

[Matt Tytel, 2012-02-15 06:09:50]

Cleaned the code and refactored and the code now reports better error messages.
Note that this implementation doesn't support optional parameters for callbacks,
thought it's easy to fix.

[Matt Tytel, 2012-02-15 06:11:27]

The code expanded a bit more than I thought after supporting these error
messages. Should this move to another file?

[Matt Perry, 2012-02-16 00:31:08]

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:70: //
Generate all possible signatures for a given API function scehma.
schema*

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:80:
signatures.push(signature);
could do this more simply as signatures.push([schemes[0]].concat(remaining[i]))

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:101:
chromeHidden.matchSignature = function(args, schemas) {
The implementation here is somewhat wasteful. You generate all possible
signatures in advance, when you could check them recursively against args on the
fly. That said, I'm not sure it matters enough to worry about.

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:179: return
name +"(" + type_names.join(", ") + ")";
space before "("

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:187: return
name +"(" + type_names.join(", ") + ")";
space before "("

http://codereview.chromium.org/9317072/diff/10007/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:190: // Finds
the correct signature for the given arguments
end sentence with .

[Matt Tytel, 2012-02-16 04:35:02]

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:101:
chromeHidden.matchSignature = function(args, schemas) {
I think this a code efficiency vs code simplicity issue. Here I'm reusing the
code from the ambiguity check (isSchemaAmbiguous) to avoid duplicating code.

[Matt Perry, 2012-02-16 19:21:29]

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:101:
chromeHidden.matchSignature = function(args, schemas) {
On 2012/02/16 04:35:02, mtytel wrote:
> I think this a code efficiency vs code simplicity issue. Here I'm reusing the
> code from the ambiguity check (isSchemaAmbiguous) to avoid duplicating code.

Yep. I think you made the right tradeoff here.

[Aaron Boodman, 2012-02-16 21:36:54]

This is great stuff. Just naming nits.

+rafaelw, kalman fyi

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:35:
chromeHidden.getAllTypesForSchema = function(schema) {
Button a function on chromeHidden basically exports it to all the other
javascript files that make up the bindings layer in this context. It looks like
that isn't needed here, so you should just make these plain functions, like
forEach, above.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:46:
schema_types.push(validator.types[schema['$ref']].type);
What if it has choices?

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:109:
chromeHidden.isSchemaAmbiguous = function(schema) {
The terms in use here are getting a bit overloaded and confusing.

A 'schema' is pretty ambiguous since it is hierarchical. It could represent an
entire API, just one function, a single parameter, etc.

How about terms like:

* definedSignature: the signature with optionals.
* resolution: the process to choose one of the candidates.
* candidateSignatures: the list of signatures without optionals.
* resolvedSignature: the result of resolution.

If you ever have to use the word schema, it might be best to qualify it, like
parameterSchema, or namespaceSchema, to make it more clear which one you're
talking about.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:583: //
Validate API for ambiguity only in DEBUG mode.
Can you add this here please:

TODO(aa): It would be best to run this in a unit test, but in order to do that
we would need to better factor this code so that it didn't depend on so much
v8::Extension machinery.

[not at google - send to devlin, 2012-02-17 00:47:10]

This is super cool. I'm not planning on reviewing this, but I have a general
comment:

This is a lot of high-density code, some of which deals with schema validation
and some of which deals with the normalisation of arguments for when the
generated functions are called. The latter definitely belongs in SGB.js, but can
the former be factored away into json_schema.js somehow? E.g. a method like
isSchemaAmbiguous sounds like it belongs in json_schema.js.

A very nice benefit of code in json_schema.js is that it can be
pseudo-unittested -- see chrome/test/data/extensions/json_schema_test.js.
Another is that I think the code would be easier to follow.

(Also CC'ing calamity@ who's writing a JSON schema based code generator for C++
-- I don't think that this change will affect it since the normalisation process
is purely a preprocessing type thing before anything reaches the browser).

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:188: // Finds
the correct signature for the given arguments.
I think this comment (and function name) is not quite right. Something along the
lines of "padMissingOptionalArguments" or something -- the matching/validating
is just a sideeffect of that process.

[Aaron Boodman, 2012-02-17 00:53:09]

On Thu, Feb 16, 2012 at 4:47 PM,  <kalman@chromium.org> wrote:
> This is super cool. I'm not planning on reviewing this, but I have a general
> comment:
>
> This is a lot of high-density code, some of which deals with schema
> validation
> and some of which deals with the normalisation of arguments for when the
> generated functions are called. The latter definitely belongs in SGB.js, but
> can
> the former be factored away into json_schema.js somehow? E.g. a method like
> isSchemaAmbiguous sounds like it belongs in json_schema.js.
>
> A very nice benefit of code in json_schema.js is that it can be
> pseudo-unittested -- see chrome/test/data/extensions/json_schema_test.js.
> Another is that I think the code would be easier to follow.

I agree with this comment. I had meant to say something to this effect as well.

- a

[Matt Tytel, 2012-02-24 01:29:27]

Moved some functions over to json_schema.js.
I only moved functions that deal with one schema object and left functions that
handle lists of schema objects (i.e. function signatures) in
schema_generated_bindings.js.

Tests are coming soon.

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:70: //
Generate all possible signatures for a given API function scehma.
On 2012/02/16 00:31:08, Matt Perry wrote:
> schema*

Done.

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:80:
signatures.push(signature);
On 2012/02/16 00:31:08, Matt Perry wrote:
> could do this more simply as
signatures.push([schemes[0]].concat(remaining[i]))

Done.

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:179: return
name +"(" + type_names.join(", ") + ")";
On 2012/02/16 00:31:08, Matt Perry wrote:
> space before "("

Done.

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:187: return
name +"(" + type_names.join(", ") + ")";
On 2012/02/16 00:31:08, Matt Perry wrote:
> space before "("

Done.

https://chromiumcodereview.appspot.com/9317072/diff/10007/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:190: // Finds
the correct signature for the given arguments
On 2012/02/16 00:31:08, Matt Perry wrote:
> end sentence with .

Done.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:35:
chromeHidden.getAllTypesForSchema = function(schema) {
On 2012/02/16 21:36:54, Aaron Boodman wrote:
> Button a function on chromeHidden basically exports it to all the other
> javascript files that make up the bindings layer in this context. It looks
like
> that isn't needed here, so you should just make these plain functions, like
> forEach, above.

Done.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:46:
schema_types.push(validator.types[schema['$ref']].type);
On 2012/02/16 21:36:54, Aaron Boodman wrote:
> What if it has choices?

Done. I don't think any have any choices or link to any other refs but might as
well check.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:109:
chromeHidden.isSchemaAmbiguous = function(schema) {
On 2012/02/16 21:36:54, Aaron Boodman wrote:
> The terms in use here are getting a bit overloaded and confusing.
> 
> A 'schema' is pretty ambiguous since it is hierarchical. It could represent an
> entire API, just one function, a single parameter, etc.
> 
> How about terms like:
> 
> * definedSignature: the signature with optionals.
> * resolution: the process to choose one of the candidates.
> * candidateSignatures: the list of signatures without optionals.
> * resolvedSignature: the result of resolution.
> 
> If you ever have to use the word schema, it might be best to qualify it, like
> parameterSchema, or namespaceSchema, to make it more clear which one you're
> talking about.

Done.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:188: // Finds
the correct signature for the given arguments.
On 2012/02/17 00:47:11, kalman wrote:
> I think this comment (and function name) is not quite right. Something along
the
> lines of "padMissingOptionalArguments" or something -- the matching/validating
> is just a sideeffect of that process.

Done.

https://chromiumcodereview.appspot.com/9317072/diff/8008/chrome/renderer/reso...
chrome/renderer/resources/extensions/schema_generated_bindings.js:583: //
Validate API for ambiguity only in DEBUG mode.
On 2012/02/16 21:36:54, Aaron Boodman wrote:
> Can you add this here please:
> 
> TODO(aa): It would be best to run this in a unit test, but in order to do that
> we would need to better factor this code so that it didn't depend on so much
> v8::Extension machinery.

Done.

[Matt Perry, 2012-02-24 02:00:12]

mostly LG, just some style issues

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/json_schema.js:500: * message.
Comment needs updating.

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:35: function
getSignatures(param_list) {
javascript style for variable names is paramList. Same goes for the rest of the
variables in this file.

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:143: return
name +" (" + type_names.join(", ") + ")";
space before " ("

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:151: return
name +" (" + type_names.join(", ") + ")";
ditto

[Matt Tytel, 2012-02-24 02:29:35]

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
File chrome/renderer/resources/extensions/json_schema.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/json_schema.js:500: * message.
On 2012/02/24 02:00:12, Matt Perry wrote:
> Comment needs updating.

Done.

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:35: function
getSignatures(param_list) {
On 2012/02/24 02:00:12, Matt Perry wrote:
> javascript style for variable names is paramList. Same goes for the rest of
the
> variables in this file.

Done.

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:143: return
name +" (" + type_names.join(", ") + ")";
On 2012/02/24 02:00:12, Matt Perry wrote:
> space before " ("

Done. I misinterpreted your last comment on this and put the space right before
the ( instead of the ". I wondered why you wanted a space there.

https://chromiumcodereview.appspot.com/9317072/diff/20011/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:151: return
name +" (" + type_names.join(", ") + ")";
On 2012/02/24 02:00:12, Matt Perry wrote:
> ditto

Done.

[not at google - send to devlin, 2012-02-24 03:59:56]

Apologies for the second drive-by. Thanks for moving those functions to
json_schema.js -- is it possible to write some tests for them (in
json_schema_test.js)?

http://codereview.chromium.org/9317072/diff/25001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9317072/diff/25001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:34: //
Generate all possible signatures for a given API function schema.
i think these methods can be grouped with the other ones after the
chromeHidden.validator thing.

[Matt Perry, 2012-02-24 19:50:50]

lgtm

[Matt Tytel, 2012-02-29 04:36:37]

Added json_schema.js tests

https://chromiumcodereview.appspot.com/9317072/diff/25001/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/25001/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:34: //
Generate all possible signatures for a given API function schema.
On 2012/02/24 03:59:56, kalman wrote:
> i think these methods can be grouped with the other ones after the
> chromeHidden.validator thing.

Done.

[Matt Tytel, 2012-03-01 08:04:33]

Fixed a problem with accepting null arguments and renamed some variables.

https://chromiumcodereview.appspot.com/9317072/diff/43004/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/43004/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:611:
apiFunction.name != "extension.sendRequest" &&
Note the small hack here. 
extension.sendRequest is ambiguous and I'm not entirely sure how to best ignore
it. (updateArgumentsPreValidate handles the ambiguity)

[Aaron Boodman, 2012-03-01 08:14:25]

still lgtm

https://chromiumcodereview.appspot.com/9317072/diff/43004/chrome/renderer/res...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

https://chromiumcodereview.appspot.com/9317072/diff/43004/chrome/renderer/res...
chrome/renderer/resources/extensions/schema_generated_bindings.js:611:
apiFunction.name != "extension.sendRequest" &&
On 2012/03/01 08:04:33, Matt Tytel wrote:
> Note the small hack here. 
> extension.sendRequest is ambiguous and I'm not entirely sure how to best
ignore
> it. (updateArgumentsPreValidate handles the ambiguity)

Just add a comment in the code explaining why the special case. 

Separating the branches might improve clarity:

if (chromeHidden.validateAPI && isDefinedsignatureAmbiguous(...)) {
  // Because blah blah blah..
  if (apiFunction.name != "...") {
  }
  throw ...
}

[Matt Tytel, 2012-03-01 08:31:22]

Here's a better fix for the null arguments.

Also the new variable name changes are in here as well.
(I deleted the wrong patch set :/)

[Aaron Boodman, 2012-03-01 22:59:05]

still lgtm

http://codereview.chromium.org/9317072/diff/40002/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/json_schema.js (right):

http://codereview.chromium.org/9317072/diff/40002/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/json_schema.js:149:
chromeHidden.JSONSchemaValidator.prototype.getAllTypesForSchema = function(
Nit: I usually see the "function(" on the next line.

http://codereview.chromium.org/9317072/diff/40002/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/9317072/diff/40002/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:35:
chromeHidden.schemaValidator = new chromeHidden.JSONSchemaValidator();
It isn't necessary to put this on chromeHidden. It can just be a variable at
this level.

[clintstaley, 2012-03-02 03:57:21]

LGTM

[Sam Kerner (Chrome), 2012-03-02 19:36:08]

lgtm

LGTM

[commit-bot: I haz the power, 2012-03-03 23:02:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/9317072/45015

[commit-bot: I haz the power, 2012-03-03 23:02:07]

Can't apply patch for file chrome/common/extensions/api/experimental.app.json.
While running patch -p1 --forward --force;
patching file chrome/common/extensions/api/experimental.app.json
Hunk #1 FAILED at 71.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/api/experimental.app.json.rej

[commit-bot: I haz the power, 2012-03-03 23:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/9317072/51001

[commit-bot: I haz the power, 2012-03-04 14:51:26]

Change committed as 124898