[Extension Bindings] Add JS custom hook support

[Extension Bindings] Add JS custom hook support

Add the ability to register a custom hook from JS. This is the next step
towards being able to use our many, many custom_bindings.js files for
existing APIs.

Add a test for the same.

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

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

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

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

Follows https://codereview.chromium.org/2552343006/; mind taking a look when you
can?

[jbroman, 2016-12-13 22:12:41]

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
File extensions/renderer/api_binding_hooks.cc (right):

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:43: void
SetHandleRequest(gin::Arguments* arguments) {
There's nothing unusual going on here, so you don't need to use gin::Arguments
here. (This is one nice thing about gin::Wrappable.)

void SetHandleRequest(v8::Isolate* isolate, const std::string& method_name,
v8::Local<v8::Function> handler) {
}

It will also handle throwing an error if this signature doesn't match.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:58: js_hooks_[qualified_method_name] =
You already have a reference to this object a few minutes up. Wouldn't this
work:

entry.Reset(isolate, handler);

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:116: CHECK(hooks_value->IsObject());
It'd be pretty bad for this to not be a v8::Object; the implementation of
gin::CreateHandle is pretty simple and already assumes v8::Object internally. Do
we need this check? (V8 already puts a debug-only check in the cast below.)

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:119: data->hook_interfaces[api_name] =
Up to your preference, but you can use Reset() here to avoid spelling out
v8::Global<v8::Object> here.

data->hook_interfaces[api_name].Reset(context->GetIsolate(), hooks_object);

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:134: run_js.Run(function, context,
v8_args.size(), v8_args.data());
Do these functions ever need the ability to throw exceptions or return values?
Currently, this doesn't seem to permit either.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:184: return base::Bind(&RunJsFunction,
run_js_,
I don't see whether I commented on this previously, but does the caller always
immediately invoke the callback? From here it's harder to tell whether this is
safe or not. e.g. you pass a Local<Context>, which is safe as long as the
callback is only invoked within the HandleScope that created it.

Might it be clearer if the function just did the thing, rather than making a
callback?

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:197: if (!v8::Script::Compile(context,
source).ToLocal(&script))
nit: Do you want to also pass in a ScriptOrigin with a resource name? It make
exception tracebacks easier to read.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:203: v8::Local<v8::Object> api_hooks =
GetJSHookInterface(api_name, context);
I can't find the definition of this function. Should it be
CreateJSHookInterface?

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:205: run_js_.Run(function, context, 1,
args);
nit: I'd prefer arraysize(args) to 1, in case arguments are added in the future.

[Devlin, 2016-12-14 02:02:09]

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

[Devlin, 2016-12-14 02:02:14]

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
File extensions/renderer/api_binding_hooks.cc (right):

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:43: void
SetHandleRequest(gin::Arguments* arguments) {
On 2016/12/13 22:12:40, jbroman wrote:
> There's nothing unusual going on here, so you don't need to use gin::Arguments
> here. (This is one nice thing about gin::Wrappable.)
> 
> void SetHandleRequest(v8::Isolate* isolate, const std::string& method_name,
> v8::Local<v8::Function> handler) {
> }
> 
> It will also handle throwing an error if this signature doesn't match.

Nifty, done.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:58: js_hooks_[qualified_method_name] =
On 2016/12/13 22:12:41, jbroman wrote:
> You already have a reference to this object a few minutes up. Wouldn't this
> work:
> 
> entry.Reset(isolate, handler);

I was definitely going to do that, and apparently got distracted halfway through
my thought.  Thanks for catching this.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:116: CHECK(hooks_value->IsObject());
On 2016/12/13 22:12:40, jbroman wrote:
> It'd be pretty bad for this to not be a v8::Object; the implementation of
> gin::CreateHandle is pretty simple and already assumes v8::Object internally.
Do
> we need this check? (V8 already puts a debug-only check in the cast below.)

Removed.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:119: data->hook_interfaces[api_name] =
On 2016/12/13 22:12:40, jbroman wrote:
> Up to your preference, but you can use Reset() here to avoid spelling out
> v8::Global<v8::Object> here.
> 
> data->hook_interfaces[api_name].Reset(context->GetIsolate(), hooks_object);

Works for me, done.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:134: run_js.Run(function, context,
v8_args.size(), v8_args.data());
On 2016/12/13 22:12:40, jbroman wrote:
> Do these functions ever need the ability to throw exceptions or return values?
> Currently, this doesn't seem to permit either.

Yes, it doesn't currently.  It will need to in the future.  I've added a TODO.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:184: return base::Bind(&RunJsFunction,
run_js_,
On 2016/12/13 22:12:41, jbroman wrote:
> I don't see whether I commented on this previously, but does the caller always
> immediately invoke the callback? From here it's harder to tell whether this is
> safe or not. e.g. you pass a Local<Context>, which is safe as long as the
> callback is only invoked within the HandleScope that created it.
> 
> Might it be clearer if the function just did the thing, rather than making a
> callback?

Much cleaner now; good idea!

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:197: if (!v8::Script::Compile(context,
source).ToLocal(&script))
On 2016/12/13 22:12:40, jbroman wrote:
> nit: Do you want to also pass in a ScriptOrigin with a resource name? It make
> exception tracebacks easier to read.

sg; done.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:203: v8::Local<v8::Object> api_hooks =
GetJSHookInterface(api_name, context);
On 2016/12/13 22:12:40, jbroman wrote:
> I can't find the definition of this function. Should it be
> CreateJSHookInterface?

Errm, yes.  Done.

https://codereview.chromium.org/2563093002/diff/1/extensions/renderer/api_bin...
extensions/renderer/api_binding_hooks.cc:205: run_js_.Run(function, context, 1,
args);
On 2016/12/13 22:12:40, jbroman wrote:
> nit: I'd prefer arraysize(args) to 1, in case arguments are added in the
future.

Done.

[commit-bot: I haz the power, 2016-12-14 02:02:51]

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-14 02:59:43]

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

[commit-bot: I haz the power, 2016-12-14 02:59:44]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-12-14 16:28:28]

lgtm

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

https://codereview.chromium.org/2563093002/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.cc:28: ~JSHookInterface() override {}
super-nit: This destructor is implied; you don't need to explicitly define it.

https://codereview.chromium.org/2563093002/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.cc:80: v8::Isolate* isolate = nullptr;
super-nit: This is explicitly initialized in the constructor, so it's a little
surprising to see it initialized here.

https://codereview.chromium.org/2563093002/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.cc:111:
v8::Local<v8::Object>::Cast(hooks_value);
super-nit: Can combine this with the previous line (and using Local::As makes it
more concise):

v8::Local<v8::Object> hooks_object = hooks.ToV8().As<v8::Object>();

[Devlin, 2016-12-14 16:53:34]

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

[Devlin, 2016-12-14 16:53:53]

lazyboy@, wanna take a look?

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

https://codereview.chromium.org/2563093002/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.cc:28: ~JSHookInterface() override {}
On 2016/12/14 16:28:28, jbroman wrote:
> super-nit: This destructor is implied; you don't need to explicitly define it.

Done.

https://codereview.chromium.org/2563093002/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.cc:80: v8::Isolate* isolate = nullptr;
On 2016/12/14 16:28:27, jbroman wrote:
> super-nit: This is explicitly initialized in the constructor, so it's a little
> surprising to see it initialized here.

Personally, I prefer initializing all POD in the declaration if any are, because
otherwise you have to keep track of which thing is initialized where, rather
than just being able to look at the declaration and know with certainty that
everything is safe.  But, given the declaration and definition are together and
that this is the only POD member, I can see why it looks weird. :)  Un-init'd.

https://codereview.chromium.org/2563093002/diff/20001/extensions/renderer/api...
extensions/renderer/api_binding_hooks.cc:111:
v8::Local<v8::Object>::Cast(hooks_value);
On 2016/12/14 16:28:27, jbroman wrote:
> super-nit: Can combine this with the previous line (and using Local::As makes
it
> more concise):
> 
> v8::Local<v8::Object> hooks_object = hooks.ToV8().As<v8::Object>();

Done.

[commit-bot: I haz the power, 2016-12-14 16:54:04]

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-14 17:49:01]

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

[commit-bot: I haz the power, 2016-12-14 17:49:02]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-12-14 19:13:50]

lgtm

[Devlin, 2016-12-14 19:19:35]

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

[Devlin, 2016-12-14 19:19:36]

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

[commit-bot: I haz the power, 2016-12-14 19:20:25]

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-14 19:28:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481743175772780,
"parent_rev": "0ad7a50543469b20638bff2607dcfd6e7ae5554a", "commit_rev":
"a758be039180b1fc0ed25d916fb2af0212514623"}

[commit-bot: I haz the power, 2016-12-14 19:28:39]

Description was changed from

==========
[Extension Bindings] Add JS custom hook support

Add the ability to register a custom hook from JS. This is the next step
towards being able to use our many, many custom_bindings.js files for
existing APIs.

Add a test for the same.

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

to

==========
[Extension Bindings] Add JS custom hook support

Add the ability to register a custom hook from JS. This is the next step
towards being able to use our many, many custom_bindings.js files for
existing APIs.

Add a test for the same.

BUG=653596

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

[commit-bot: I haz the power, 2016-12-14 19:28:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-14 19:31:09]

Description was changed from

==========
[Extension Bindings] Add JS custom hook support

Add the ability to register a custom hook from JS. This is the next step
towards being able to use our many, many custom_bindings.js files for
existing APIs.

Add a test for the same.

BUG=653596

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

to

==========
[Extension Bindings] Add JS custom hook support

Add the ability to register a custom hook from JS. This is the next step
towards being able to use our many, many custom_bindings.js files for
existing APIs.

Add a test for the same.

BUG=653596

Committed: https://crrev.com/e59cdacff65172969c9ce74dd5f8f6b8d8b63ce9
Cr-Commit-Position: refs/heads/master@{#438588}
==========

[commit-bot: I haz the power, 2016-12-14 19:31:10]

Patchset 3 (id:??) landed as
https://crrev.com/e59cdacff65172969c9ce74dd5f8f6b8d8b63ce9
Cr-Commit-Position: refs/heads/master@{#438588}