[Extensions Bindings] Consider argument type more in signature parsing

[Extensions Bindings] Consider argument type more in signature parsing

Right now, when parsing arguments, we look at whether or not an argument
matches the expected argument, and, if it does not and the argument is
optional, we advance to the next argument. This results in strange
error messages when in certain circumstances. For instance, consider a
signature:
optional object<prop: string>, optional function

If we pass {prop: 2} to this signature, we'd first compare against
the object parameter, find it doesn't match, and then advance to
comparing against the function argument. This resulted in an error
message focusing on the function argument, rather than the object
argument.

To address this, check the types of the arguments first. If the type
matches the expected type, assume that the argument is meant to match.
This way, when we see the first argument is an object, we parse it
against the object parameter and give appropriate errors.

This can potentially cause issues when there is an ambiguous signature
based on argument types (e.g., optional object, optional object, and
provided a single argument - which object to match against?), but these
ambiguous signatures are already disallowed unless the API implements
custom handling.

BUG=653596
Review-Url: https://codereview.chromium.org/2947223003
Cr-Commit-Position: refs/heads/master@{#482118}
Committed: https://chromium.googlesource.com/chromium/src/+/d701d8e63d28192afbdbe9d0d65569a4b994fd51

[Devlin, 2017-06-21 22:08:45]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 22:09:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-06-21 23:00:35]

Description was changed from

==========
check type first
BUG=
==========

to

==========
[Extensions Bindings] Consider argument type more in signature parsing

Right now, when parsing arguments, we look at whether or not an argument
matches the expected argument, and, if it does not and the argument is
optional, we advance to the next argument. This results in strange
error messages when in certain circumstances. For instance, consider a
signature:
optional object<prop: string>, optional function

If we pass {prop: 2} to this signature, we'd first compare against
the object parameter, find it doesn't match, and then advance to
comparing against the function argument. This resulted in an error
message focusing on the function argument, rather than the object
argument.

To address this, check the types of the arguments first. If the type
matches the expected type, assume that the argument is meant to match.
This way, when we see the first argument is an object, we parse it
against the object parameter and give appropriate errors.

This can potentially cause issues when there is an ambiguous signature
based on argument types (e.g., optional object, optional object, and
provided a single argument - which object to match against?), but these
ambiguous signatures are already disallowed unless the API implements
custom handling.

BUG=653596
==========

[Devlin, 2017-06-21 23:07:39]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-06-21 23:07:39]

Hey folks, mind taking a look?

Sorry for the long CL description... it's a bit of a subtle change.

https://codereview.chromium.org/2947223003/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/window_open/argument_overflow/test.js
(right):

https://codereview.chromium.org/2947223003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/window_open/argument_overflow/test.js:10:
var nativeBindingsError = 'Invalid type: expected integer, found number.';
Concretely, this CL changes the error from:

Error in invocation of windows.create(optional object createData, optional
function callback): Error at parameter 'callback': Invalid type: expected
function, found object.

to

Error in invocation of windows.create(optional object createData, optional
function callback): Error at parameter 'createData': Error at property 'left':
Invalid type: expected integer, found number.

[commit-bot: I haz the power, 2017-06-22 00:58:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 00:58:46]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-06-22 15:55:28]

lgtm from a JS side of things; I don't know enough about extension APIs to be
able to confirm that this is consistent with how callers expect to use APIs

https://codereview.chromium.org/2947223003/diff/20001/extensions/renderer/arg...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2947223003/diff/20001/extensions/renderer/arg...
extensions/renderer/argument_spec.cc:317: *error = api_errors::InvalidChoice();
Not related to this CL, but just noticed: we swallow the error messages of the
individual choices, which might be useful in clarifying why none of them
matched. Not sure how much this matters in practice.

[Devlin, 2017-06-22 16:05:17]

https://codereview.chromium.org/2947223003/diff/20001/extensions/renderer/arg...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2947223003/diff/20001/extensions/renderer/arg...
extensions/renderer/argument_spec.cc:317: *error = api_errors::InvalidChoice();
On 2017/06/22 15:55:27, jbroman wrote:
> Not related to this CL, but just noticed: we swallow the error messages of the
> individual choices, which might be useful in clarifying why none of them
> matched. Not sure how much this matters in practice.

Yes, we do.  This CL actually improved that slightly.  For instance, if we have
a choice between string|integer and the extension passes a bool, we'll now say
"expected string|integer, found boolean" (whereas before we'd say "invalid
choice").  But we don't give extensive information once we get further down,
because it could be hard.  For instance, consider:

choices: object<prop1: string, prop2: bool> || object<prop1: bool> || boolean

and the extension passes an object<prop1: integer>

The error here is tricky.  We probably *don't* want to spell out each full
possibility (that would be too verbose), but we can't always pinpoint an exact
error (with a *lot* of complexity, we could probably deduce down to "expected
prop1 to be either a string or a bool", but that's hard).  And saying just
"expected object or bool" wouldn't work, because the developer did pass an
object.

So, overall, "invalid choice" seemed appropriate.  Note we will still point to
the exact argument (and potentially property or index), so the developer should
be able to see exactly where the error was and look up the docs.

[lazyboy, 2017-06-23 23:07:05]

lgtm

[Devlin, 2017-06-24 00:48:08]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 00:48:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 02:05:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-24 02:05:17]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-24 02:05:58]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2017-06-24 02:06:03]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-06-24 02:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2947223003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-24 02:06:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 02:11:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498269963456720,
"parent_rev": "64bddcd3d17a15e211a9b92f7e0a998d2713ecbb", "commit_rev":
"d701d8e63d28192afbdbe9d0d65569a4b994fd51"}

[commit-bot: I haz the power, 2017-06-24 02:11:22]

Description was changed from

==========
[Extensions Bindings] Consider argument type more in signature parsing

Right now, when parsing arguments, we look at whether or not an argument
matches the expected argument, and, if it does not and the argument is
optional, we advance to the next argument. This results in strange
error messages when in certain circumstances. For instance, consider a
signature:
optional object<prop: string>, optional function

If we pass {prop: 2} to this signature, we'd first compare against
the object parameter, find it doesn't match, and then advance to
comparing against the function argument. This resulted in an error
message focusing on the function argument, rather than the object
argument.

To address this, check the types of the arguments first. If the type
matches the expected type, assume that the argument is meant to match.
This way, when we see the first argument is an object, we parse it
against the object parameter and give appropriate errors.

This can potentially cause issues when there is an ambiguous signature
based on argument types (e.g., optional object, optional object, and
provided a single argument - which object to match against?), but these
ambiguous signatures are already disallowed unless the API implements
custom handling.

BUG=653596
==========

to

==========
[Extensions Bindings] Consider argument type more in signature parsing

Right now, when parsing arguments, we look at whether or not an argument
matches the expected argument, and, if it does not and the argument is
optional, we advance to the next argument. This results in strange
error messages when in certain circumstances. For instance, consider a
signature:
optional object<prop: string>, optional function

If we pass {prop: 2} to this signature, we'd first compare against
the object parameter, find it doesn't match, and then advance to
comparing against the function argument. This resulted in an error
message focusing on the function argument, rather than the object
argument.

To address this, check the types of the arguments first. If the type
matches the expected type, assume that the argument is meant to match.
This way, when we see the first argument is an object, we parse it
against the object parameter and give appropriate errors.

This can potentially cause issues when there is an ambiguous signature
based on argument types (e.g., optional object, optional object, and
provided a single argument - which object to match against?), but these
ambiguous signatures are already disallowed unless the API implements
custom handling.

BUG=653596

Review-Url: https://codereview.chromium.org/2947223003
Cr-Commit-Position: refs/heads/master@{#482118}
Committed:
https://chromium.googlesource.com/chromium/src/+/d701d8e63d28192afbdbe9d0d655...
==========

[commit-bot: I haz the power, 2017-06-24 02:11:23]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d701d8e63d28192afbdbe9d0d655...