[Extensions Bindings] Allow custom hooks to return values, throw

[Extensions Bindings] Allow custom hooks to return values, throw

Custom hooks need the ability to return results synchronously and
throw exceptions in the case of bad invocations. Update native
bindings to allow for hooks to do this, and add a number of tests.

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

[Devlin, 2016-12-29 17:53:11]

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

[commit-bot: I haz the power, 2016-12-29 17:53:19]

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, 2016-12-29 18:29:10]

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

[commit-bot: I haz the power, 2016-12-29 18:29:11]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-12-29 18:34:25]

Description was changed from

==========
Return value
BUG=
==========

to

==========
[Extensions Bindings] Allow custom hooks to return values, throw

Custom hooks need the ability to return results synchronously and
throw exceptions in the case of bad invocations. Update native
bindings to allow for hooks to do this, and add a number of tests.

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

[Devlin, 2016-12-29 18:34:34]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-12-29 18:37:35]

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

[Devlin, 2016-12-29 18:37:36]

Heya, please take a look.  Technically depends on
https://codereview.chromium.org/2598123002/, but it's a small enough dependency
that I think we can do the reviews in parallel.

Cheers!

[jbroman, 2017-01-02 20:05:46]

Largely looks good to me; I'll have a closer look at the unit tests once my
comments here and the other CL are settled.

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

https://codereview.chromium.org/2609553003/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding.cc:228: if (!return_value.IsEmpty())
Do we expect this to ever be IsEmpty is a case that shouldn't be a DCHECK
failure (i.e. a programming failure)?

Every JS function returns a value, even if it's |undefined|.

https://codereview.chromium.org/2609553003/diff/20001/extensions/renderer/api...
File extensions/renderer/api_binding_hooks.h (right):

https://codereview.chromium.org/2609553003/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.h:80: v8::Local<v8::Value>* return_value);
nit: This argument list is getting a little long. WDYT of at wrapping this up
with the result enum:

struct RequestResult {
  enum ResultCode { HANDLED, INVALID_INVOCATION, ... };
  ResultCode code;
  v8::Local<v8::Value> value;  // if code == HANDLED
};

[Devlin, 2017-01-04 18:38:07]

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

https://codereview.chromium.org/2609553003/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding.cc:228: if (!return_value.IsEmpty())
On 2017/01/02 20:05:46, jbroman wrote:
> Do we expect this to ever be IsEmpty is a case that shouldn't be a DCHECK
> failure (i.e. a programming failure)?
> 
> Every JS function returns a value, even if it's |undefined|.

In the case of a native (c++) handler hook without a return value, I'd expect
this to happen.

https://codereview.chromium.org/2609553003/diff/20001/extensions/renderer/api...
File extensions/renderer/api_binding_hooks.h (right):

https://codereview.chromium.org/2609553003/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.h:80: v8::Local<v8::Value>* return_value);
On 2017/01/02 20:05:46, jbroman wrote:
> nit: This argument list is getting a little long. WDYT of at wrapping this up
> with the result enum:
> 
> struct RequestResult {
>   enum ResultCode { HANDLED, INVALID_INVOCATION, ... };
>   ResultCode code;
>   v8::Local<v8::Value> value;  // if code == HANDLED
> };

Hmm... it makes it seem a little more verbose to me, but I don't feel too
strongly.  I've done this in the latest patch - if you prefer it, let's keep it.

[jbroman, 2017-01-04 21:57:49]

lgtm

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding_hooks.h (right):

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.h:44: ~RequestResult();
Is the [chromium-style] plugin forcing these to be out-of-line? If not, then the
first can be inline and the other two are implied (v8::Local is trivial to copy
and destroy -- it's just a pointer internally).

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.h:81: // runs it. Returns the result of
running the hook, if any. Populates
If the |return_value| argument is removed, then this last bit of comment can go
away.

[lazyboy, 2017-01-04 23:22:45]

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding_unittest.cc (right):

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_unittest.cc:809: EXPECT_EQ(1u,
arguments->size());
Is this line correct given the condition on line above this? Same for line 464
in this file.

[Devlin, 2017-01-04 23:29:29]

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding_hooks.h (right):

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.h:44: ~RequestResult();
On 2017/01/04 21:57:49, jbroman wrote:
> Is the [chromium-style] plugin forcing these to be out-of-line? If not, then
the
> first can be inline and the other two are implied (v8::Local is trivial to
copy
> and destroy -- it's just a pointer internally).

Yeah, this was the result of being bullied by [chromium-style].

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.h:81: // runs it. Returns the result of
running the hook, if any. Populates
On 2017/01/04 21:57:49, jbroman wrote:
> If the |return_value| argument is removed, then this last bit of comment can
go
> away.

Whoops, done.

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding_unittest.cc (right):

https://codereview.chromium.org/2609553003/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding_unittest.cc:809: EXPECT_EQ(1u,
arguments->size());
On 2017/01/04 23:22:45, lazyboy wrote:
> Is this line correct given the condition on line above this? Same for line 464
> in this file.

It is correct-ish - these should be ASSERTs since the test will crash otherwise,
but ASSERTs mess with the return type of the function.  I've added a comment to
this effect.

[lazyboy, 2017-01-05 00:15:17]

lgtm

[Devlin, 2017-01-06 19:46:54]

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

[commit-bot: I haz the power, 2017-01-06 19:47:43]

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

[Devlin, 2017-01-06 20:36:28]

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

[Devlin, 2017-01-06 20:36:31]

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

[Devlin, 2017-01-06 20:36:32]

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

[commit-bot: I haz the power, 2017-01-06 20:36:44]

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-01-06 20:45:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483734991934820,
"parent_rev": "8c9857238e62da4de34c8b02458418916c8a77d0", "commit_rev":
"3a066a5d5906e16b1a9281dd6d987f8defd1a87f"}

[commit-bot: I haz the power, 2017-01-06 20:45:31]

Description was changed from

==========
[Extensions Bindings] Allow custom hooks to return values, throw

Custom hooks need the ability to return results synchronously and
throw exceptions in the case of bad invocations. Update native
bindings to allow for hooks to do this, and add a number of tests.

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

to

==========
[Extensions Bindings] Allow custom hooks to return values, throw

Custom hooks need the ability to return results synchronously and
throw exceptions in the case of bad invocations. Update native
bindings to allow for hooks to do this, and add a number of tests.

BUG=653596

Review-Url: https://codereview.chromium.org/2609553003
Cr-Commit-Position: refs/heads/master@{#442037}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a066a5d5906e16b1a9281dd6d98...
==========

[commit-bot: I haz the power, 2017-01-06 20:45:32]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3a066a5d5906e16b1a9281dd6d98...