[Extensions Bindings] Allow for argument validation without conversion

[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

We also need to add support for parsing arguments that are functions,
since we are now validating for requests with custom handlers (which
can have functions as arguments other than callbacks).

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

BUG=653596
Committed: https://crrev.com/6598b14ac84919777d8019054114b7b0aa6baec0
Cr-Commit-Position: refs/heads/master@{#440816}

[Devlin, 2016-12-19 21:14:16]

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

[commit-bot: I haz the power, 2016-12-19 21:14:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-12-19 21:22:55]

Description was changed from

==========
arg hooks
BUG=
==========

to

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

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

[commit-bot: I haz the power, 2016-12-19 22:43:48]

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

[commit-bot: I haz the power, 2016-12-19 22:43:48]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-12-19 23:09:37]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-12-19 23:31:50]

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

[Devlin, 2016-12-22 17:10:03]

friendly ping :)

[jbroman, 2016-12-22 20:01:34]

This largely looks good (though I wish we didn't have to have this forked
logic).

I mainly have a thought about whether we can extract out most of the
base::Value/v8::Value-specific bits.

Sorry for the review delay; it slipped past me until this morning (and then I
was busy with an interview).

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
extensions/renderer/api_signature.cc:24: bool
APISignature::ParseArgumentsToV8(gin::Arguments* arguments,
Higher level thought on this (that may cancel some of my previous ones): it's
unfortunate that we have to have this much code that knows about both paths
explicitly (or at all, but we're stuck with that).

WDYT about instead making ParseArgumentsImpl a template that hides as much of
the difference as possible?

https://gist.github.com/jeremyroman/9c6445fcf988a88e9440c275f2471afe is a sketch
at what that might look like.

I'm not super attached to it, but it does trim out all of these functions having
two different kinds of out arguments and having to null check them all over the
place.

WDYT?

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
extensions/renderer/api_signature.cc:52: DCHECK_EQ(!!json_out, !!callback_out);
I found this a little tough to read. Is this equivalent to:

if (json_out)
  DCHECK(callback_out);
else
  DCHECK(v8_out);

This is pretty close to what's already being DCHECKed below, so you could just
add a DCHECK(callback_out) to the json_out branch below.

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
extensions/renderer/api_signature.cc:140:
v8_out->push_back(v8::Null(context->GetIsolate()));
It's strange that |v8_out| here is unconditionally null, and doesn't match how
the caller behaves.

[Devlin, 2016-12-22 22:35:04]

On 2016/12/22 20:01:34, jbroman wrote:
> Sorry for the review delay; it slipped past me until this morning (and then I
> was busy with an interview).

No worries!  I figured it had just slipped through.

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
extensions/renderer/api_signature.cc:24: bool
APISignature::ParseArgumentsToV8(gin::Arguments* arguments,
On 2016/12/22 20:01:34, jbroman wrote:
> Higher level thought on this (that may cancel some of my previous ones): it's
> unfortunate that we have to have this much code that knows about both paths
> explicitly (or at all, but we're stuck with that).
> 
> WDYT about instead making ParseArgumentsImpl a template that hides as much of
> the difference as possible?
> 
> https://gist.github.com/jeremyroman/9c6445fcf988a88e9440c275f2471afe is a
sketch
> at what that might look like.
> 
> I'm not super attached to it, but it does trim out all of these functions
having
> two different kinds of out arguments and having to null check them all over
the
> place.
> 
> WDYT?

Interesting idea!  I like it a lot, for the most part.  I've tweaked it a bit,
though.

In the next step, we need to support the hook updateArgumentsPreValidate(),
which, as the name implies, allows a hook to manipulate the raw arguments passed
to it prior to any validation.  This causes some messiness in a few areas, and
one of them is that it means that parsing arguments might not always take a
gin::Arguments.  In order to clean some of it up, I found it useful to have a
helper class assist in the argument parsing.

So, back to this patch (the rest will be more clear once I can upload it) - the
idioms you suggested work well with having that helper class, and just
subclassing it differently for V8 vs JSON parsing, so I've done that here rather
than causing even more churn once the next patch is ready.  I think this still
addresses the main concerns you had.  (It's also a bit small in binary size,
since we don't have to templatize the rather-complicated ParseArgument(s).)

Let me know what you think. :)

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
extensions/renderer/api_signature.cc:52: DCHECK_EQ(!!json_out, !!callback_out);
On 2016/12/22 20:01:34, jbroman wrote:
> I found this a little tough to read. Is this equivalent to:
> 
> if (json_out)
>   DCHECK(callback_out);
> else
>   DCHECK(v8_out);
> 
> This is pretty close to what's already being DCHECKed below, so you could just
> add a DCHECK(callback_out) to the json_out branch below.

Moot, I think.

https://codereview.chromium.org/2583273002/diff/20001/extensions/renderer/api...
extensions/renderer/api_signature.cc:140:
v8_out->push_back(v8::Null(context->GetIsolate()));
On 2016/12/22 20:01:34, jbroman wrote:
> It's strange that |v8_out| here is unconditionally null, and doesn't match how
> the caller behaves.

Moot, I think.

[Devlin, 2016-12-23 00:56:41]

Description was changed from

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

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

to

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

We also need to add support for parsing arguments that are functions,
since we are now validating for requests with custom handlers (which
can have functions as arguments other than callbacks).

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

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

[jbroman, 2016-12-23 18:20:30]

lgtm

[Devlin, 2016-12-27 16:27:06]

lazyboy@, wanna take a look?

[lazyboy, 2016-12-27 23:48:44]

lgtm w/ an optional nit

https://codereview.chromium.org/2583273002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2583273002/diff/80001/extensions/renderer/api...
extensions/renderer/api_signature.cc:31: // resulting list as it goes.
nittynit: I find "as it goes" a bit confusing, hinting we need to call this
multiple times... or it describes internal implementation detail. The resulting
list is populated in one go, right? Maybe just avoid this part after the comma.

[Devlin, 2016-12-28 00:19:34]

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

[commit-bot: I haz the power, 2016-12-28 00:19:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-12-28 00:56:18]

https://codereview.chromium.org/2583273002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2583273002/diff/80001/extensions/renderer/api...
extensions/renderer/api_signature.cc:31: // resulting list as it goes.
On 2016/12/27 23:48:44, lazyboy wrote:
> nittynit: I find "as it goes" a bit confusing, hinting we need to call this
> multiple times... or it describes internal implementation detail. The
resulting
> list is populated in one go, right? Maybe just avoid this part after the
comma.

Sounds good to me; done.

[Devlin, 2016-12-28 00:56:22]

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

[Devlin, 2016-12-28 00:56:26]

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

[Devlin, 2016-12-28 00:56:26]

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/2583273002/#ps100001
(title: "lazyboy's")

[commit-bot: I haz the power, 2016-12-28 00:56:41]

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, 2016-12-28 01:07:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482886586410710,
"parent_rev": "befa0862948fda61d20835062c20c003254a3068", "commit_rev":
"5a49f8c1b51cfc60b2edfab5c79b963946a54ce9"}

[commit-bot: I haz the power, 2016-12-28 01:09:24]

Description was changed from

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

We also need to add support for parsing arguments that are functions,
since we are now validating for requests with custom handlers (which
can have functions as arguments other than callbacks).

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

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

to

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

We also need to add support for parsing arguments that are functions,
since we are now validating for requests with custom handlers (which
can have functions as arguments other than callbacks).

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

BUG=653596

Review-Url: https://codereview.chromium.org/2583273002
==========

[commit-bot: I haz the power, 2016-12-28 01:09:26]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2017-01-02 15:48:05]

Description was changed from

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

We also need to add support for parsing arguments that are functions,
since we are now validating for requests with custom handlers (which
can have functions as arguments other than callbacks).

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

BUG=653596

Review-Url: https://codereview.chromium.org/2583273002
==========

to

==========
[Extensions Bindings] Allow for argument validation without conversion

Currently, the argument validation and and argument conversion happens
in a single step. This is good for the case when we need the JSON
serialized arguments, but in the case of custom JS hooks, the work is
unnecessary. However, we still need to match arguments to a signature
in that case.

Add another method to allow for matching, but not converting, the
arguments, and use it when we have a handle request JS hook.  This is
in line with what the curent JS implementation does, and thus what our
custom bindings expect.

We also need to add support for parsing arguments that are functions,
since we are now validating for requests with custom handlers (which
can have functions as arguments other than callbacks).

Add a test that invoking a method with a custom binding with an invalid
invocation throws an error.

BUG=653596

Committed: https://crrev.com/6598b14ac84919777d8019054114b7b0aa6baec0
Cr-Commit-Position: refs/heads/master@{#440816}
==========

[commit-bot: I haz the power, 2017-01-02 15:48:07]

Patchset 5 (id:??) landed as
https://crrev.com/6598b14ac84919777d8019054114b7b0aa6baec0
Cr-Commit-Position: refs/heads/master@{#440816}