[Extensions Bindings] Add a bridge to use current custom bindings

[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

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

[Devlin, 2016-12-16 01:44:13]

Description was changed from

==========
Module system stuff
BUG=
==========

to

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

We also pull out some common testing classes
(TestV8ExtensionConfiguration and StringSourceMap) for use in testing.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

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

[Devlin, 2016-12-16 01:46:15]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-12-16 01:46:21]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-12-16 01:46:27]

Patchset #1 (id:40001) has been deleted

[Devlin, 2016-12-16 01:46:38]

Patchset #1 (id:60001) has been deleted

[Devlin, 2016-12-16 01:50:22]

Patchset #1 (id:80001) has been deleted

[Devlin, 2016-12-16 01:55:48]

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

[Devlin, 2016-12-16 01:55:49]

Heya folks, mind taking a look?  I think this CL is the one that proves that
what we've been working on actually, well, works. :)  We can use APIs!

Next steps:
- There's a bit of a medium tail on things related to this system itself. In
particular, implemented choices on argument specs, implementing the other JS
hooks (updateArguments[Pre|Post]Validate, customCallback), filtered events, the
optimizations for which there are todo's, etc.  And, of course, generating our
api signatures.
- Beginning to convert the custom bindings files.  Long term, we don't want any
JS custom bindings, but I think there's >100 files there.  There are a few we
want to convert pretty soon - like messaging, which is used in almost every
extension, is exposed to web pages, etc.  Others are probably less important,
and are probably not blocking.
- Perf testing!  Right now, there's very little around extension performance
testing.  But we need to make sure we're making improvements here.  (I'm fairly
confidant we are after the medium-tail above is fixed, but we need to a) prove
that, b) find out how much.)

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_bindings_system.h (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_bindings_system.h:62: v8::Local<v8::Object>*
hooks_interface_out);
I don't particularly love this interface, but unfortunately our current bindings
rely on having the API object ready as well (even though they perhaps
shouldn't).  This was the cleanest way I could find overall, and it seems to
keep everything minimally intrusive.  But I'm open to other suggestions. :)

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ar...
File extensions/renderer/argument_spec.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ar...
extensions/renderer/argument_spec.cc:51: const base::ListValue* choices =
nullptr;
This is needed for the chrome.test API to work, but I want to implement it in
another CL since this one's already pretty large.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/st...
File extensions/renderer/string_source_map.h (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/st...
extensions/renderer/string_source_map.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
This file and test_v8_extension_configuration are pretty much just moving stuff
- lemme know if you'd prefer it in a separate CL, and I'm happy to oblige. :)

[jbroman, 2016-12-16 19:00:50]

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/BU...
File extensions/renderer/BUILD.gn (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/BU...
extensions/renderer/BUILD.gn:187: "string_source_map.cc",
Shouldn't these be in the unit_tests target? It seems a shame to ship these in
prod Chrome if they aren't needed.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_binding.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_binding.cc:299: v8::Local<v8::Object>
APIBinding::GetJSHooksInterface(
nit: spelling changes from "Hook" to "Hooks"; why?

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.cc:37: v8::Local<v8::Context> context =
isolate->GetCallingContext();
GetCallingContext is deprecated; did you want GetCurrentContext?

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.h (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.h:41: v8::Global<v8::Value> api_object_;
I'm not confident about the lifetime of this and the below global. In
particular, this cycle looks uncollectable to me:

APIBindingBridge -> api object -> ??? -> APIBindingBridge

Thus keeping the whole context alive if there's any references from the API
object (or anything it references, including its creation context). Either we
need to make certain these cycles cannot happen (which is hard to do when author
script can mess with things), or that these references are definitely cleared,
or that these references occur in the V8 heap where the GC can see them.

One way to do the last would be to store them as a property of the wrapper
(which is how gin::Timer works, for instance). You can store a property with a
private symbol as a key (so script could never get access to that symbol), and
then retrieve it from the wrapper if you need it.

If this is already safe for some reason that's not obvious to me, it definitely
merits documentation.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/mo...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/mo...
extensions/renderer/module_system.cc:673: return
LoadModuleWithNativeAPIBridge(module_name, v8::Local<v8::Value>());
Any reason to pass an empty local here and then override it later on? Could just
put

return LoadModuleWithNativeAPIBridge(module_name, v8::Undefined(GetIsolate()));

here, and then not assume the Local<Value> is non-empty?

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/st...
File extensions/renderer/string_source_map.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/st...
extensions/renderer/string_source_map.cc:29: CHECK_EQ(0u, sources_.count(name))
<< "Module " << name << " not found";
Isn't the opposite the problem, that the module _was_ already found in
|sources_|?

[Devlin, 2016-12-16 20:31:09]

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

[Devlin, 2016-12-16 20:31:17]

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/BU...
File extensions/renderer/BUILD.gn (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/BU...
extensions/renderer/BUILD.gn:187: "string_source_map.cc",
On 2016/12/16 19:00:49, jbroman wrote:
> Shouldn't these be in the unit_tests target? It seems a shame to ship these in
> prod Chrome if they aren't needed.

Whoops!  Yes, thanks.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_binding.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_binding.cc:299: v8::Local<v8::Object>
APIBinding::GetJSHooksInterface(
On 2016/12/16 19:00:49, jbroman wrote:
> nit: spelling changes from "Hook" to "Hooks"; why?

Accident; synchronized.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.cc:37: v8::Local<v8::Context> context =
isolate->GetCallingContext();
On 2016/12/16 19:00:49, jbroman wrote:
> GetCallingContext is deprecated; did you want GetCurrentContext?

Done.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.h (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.h:41: v8::Global<v8::Value> api_object_;
On 2016/12/16 19:00:49, jbroman wrote:
> I'm not confident about the lifetime of this and the below global. In
> particular, this cycle looks uncollectable to me:
> 
> APIBindingBridge -> api object -> ??? -> APIBindingBridge
> 
> Thus keeping the whole context alive if there's any references from the API
> object (or anything it references, including its creation context). Either we
> need to make certain these cycles cannot happen (which is hard to do when
author
> script can mess with things), or that these references are definitely cleared,
> or that these references occur in the V8 heap where the GC can see them.
> 
> One way to do the last would be to store them as a property of the wrapper
> (which is how gin::Timer works, for instance). You can store a property with a
> private symbol as a key (so script could never get access to that symbol), and
> then retrieve it from the wrapper if you need it.
> 
> If this is already safe for some reason that's not obvious to me, it
definitely
> merits documentation.

Thanks for catching this!  I definitely still haven't learned all the ins and
outs of v8 lifetime, so I appreciate the skeptical eye. :)  I've adopted the
private property approach.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/mo...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/mo...
extensions/renderer/module_system.cc:673: return
LoadModuleWithNativeAPIBridge(module_name, v8::Local<v8::Value>());
On 2016/12/16 19:00:50, jbroman wrote:
> Any reason to pass an empty local here and then override it later on? Could
just
> put
> 
> return LoadModuleWithNativeAPIBridge(module_name,
v8::Undefined(GetIsolate()));
> 
> here, and then not assume the Local<Value> is non-empty?

Works for me. Done.

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/st...
File extensions/renderer/string_source_map.cc (right):

https://codereview.chromium.org/2575173002/diff/100001/extensions/renderer/st...
extensions/renderer/string_source_map.cc:29: CHECK_EQ(0u, sources_.count(name))
<< "Module " << name << " not found";
On 2016/12/16 19:00:50, jbroman wrote:
> Isn't the opposite the problem, that the module _was_ already found in
> |sources_|?

Copy-paste code strikes again!  It was this way in module_system_test.cc, too. 
Fixed.

[commit-bot: I haz the power, 2016-12-16 20:32:46]

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-16 22:04:55]

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

[commit-bot: I haz the power, 2016-12-16 22:04:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2016-12-16 22:26:06]

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

[commit-bot: I haz the power, 2016-12-16 22:27:03]

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-17 00:26:21]

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

[commit-bot: I haz the power, 2016-12-17 00:26:22]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2016-12-17 00:50:54]

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

[commit-bot: I haz the power, 2016-12-17 00:51:42]

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-17 02:56:49]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2016-12-20 20:51:35]

lgtm; the lifetime now looks simpler to me

Please rebase to remove the changes that have already landed, and update CL
description accordingly.

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.cc (right):

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.cc:64: v8::Local<v8::Value>
hook_interface;
The naming is a little funny here, with kHookObjectKey corresponding to
hook_interface, rather than hook_object.

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.cc:69: return;
nit: It'd be pretty surprising for these private properties to go away. I'd at
least NOTREACHED if that happens, or possibly just use ToLocalChecked.

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.h (right):

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.h:39: v8::Local<v8::Function> function);
I assume it's documented elsewhere what parameters this function takes, what the
hooks do, etc.? I admit that I get a little lost in this, not having familiarity
with the shape of it.

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system_unittest.cc:282: " 
api.apiFunctions.setHandleRequest('queryState',\n"
Do we need more exhaustive coverage than this one test, or is there coverage
elsewhere? Currently a bunch of parameters are set up but I don't see tests
covering them.

[Devlin, 2016-12-20 21:33:07]

Description was changed from

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

We also pull out some common testing classes
(TestV8ExtensionConfiguration and StringSourceMap) for use in testing.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

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

to

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

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

[Devlin, 2016-12-20 22:20:20]

lazyboy@, wanna take a look?

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.cc (right):

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.cc:64: v8::Local<v8::Value>
hook_interface;
On 2016/12/20 20:51:34, jbroman wrote:
> The naming is a little funny here, with kHookObjectKey corresponding to
> hook_interface, rather than hook_object.

s/kHookObjectKey/kHookInterfaceKey

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.cc:69: return;
On 2016/12/20 20:51:34, jbroman wrote:
> nit: It'd be pretty surprising for these private properties to go away. I'd at
> least NOTREACHED if that happens, or possibly just use ToLocalChecked.

ToLocalChecked()'ing.

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_binding_bridge.h (right):

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_bridge.h:39: v8::Local<v8::Function> function);
On 2016/12/20 20:51:35, jbroman wrote:
> I assume it's documented elsewhere what parameters this function takes, what
the
> hooks do, etc.? I admit that I get a little lost in this, not having
familiarity
> with the shape of it.

Added more of a comment.

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2575173002/diff/160001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system_unittest.cc:282: " 
api.apiFunctions.setHandleRequest('queryState',\n"
On 2016/12/20 20:51:35, jbroman wrote:
> Do we need more exhaustive coverage than this one test, or is there coverage
> elsewhere? Currently a bunch of parameters are set up but I don't see tests
> covering them.

Made this test more thorough.  Note also that a lot of this (though not all) is
tested quite well by the e2e test added.

[lazyboy, 2016-12-21 19:49:58]

lgtm

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/ap...
File extensions/renderer/api_binding.h (right):

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/ap...
extensions/renderer/api_binding.h:101: // The registered hooks for this API.
Null if there are no registered custom
This can't be null anymore?

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/mo...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/mo...
extensions/renderer/module_system.cc:750: api_bridge,
context_->safe_builtins()->GetArray(),
nit: api_bridge should go before safe builtin comment on line 749?
Also I'd add a comment here saying this is exposed as "apiBridge" in JS so it's
easier to grep.

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system_unittest.cc:330: property =
(optional nit) This pattern of pulling a property and matching its string value
seems repetitive.
maybe add a lambda: auto has_property_with_value = [&context](string&
orperty_name, string& property_value) { ...  }

[Devlin, 2016-12-21 20:38:56]

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

[commit-bot: I haz the power, 2016-12-21 20:39:41]

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-21 21:22:19]

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

[commit-bot: I haz the power, 2016-12-21 21:22:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-12-21 21:42:03]

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

[commit-bot: I haz the power, 2016-12-21 21:42:53]

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-21 22:31:22]

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

[commit-bot: I haz the power, 2016-12-21 22:31:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2016-12-21 23:35:50]

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

[commit-bot: I haz the power, 2016-12-21 23:36:16]

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-22 00:55:53]

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

[commit-bot: I haz the power, 2016-12-22 00:55:54]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-12-22 01:49:10]

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/ap...
File extensions/renderer/api_binding.h (right):

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/ap...
extensions/renderer/api_binding.h:101: // The registered hooks for this API.
Null if there are no registered custom
On 2016/12/21 19:49:58, lazyboy wrote:
> This can't be null anymore?

Whoops - missed this one.  Done.

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/mo...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/mo...
extensions/renderer/module_system.cc:750: api_bridge,
context_->safe_builtins()->GetArray(),
On 2016/12/21 19:49:58, lazyboy wrote:
> nit: api_bridge should go before safe builtin comment on line 749?
> Also I'd add a comment here saying this is exposed as "apiBridge" in JS so
it's
> easier to grep.

Done.

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2575173002/diff/180001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system_unittest.cc:330: property =
On 2016/12/21 19:49:58, lazyboy wrote:
> (optional nit) This pattern of pulling a property and matching its string
value
> seems repetitive.
> maybe add a lambda: auto has_property_with_value = [&context](string&
> orperty_name, string& property_value) { ...  }

Yeah, I was debating even putting this in the util file.  Done here as a lambda
as you suggest for now; might move it in a followup.

[Devlin, 2016-12-22 01:49:17]

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

[Devlin, 2016-12-22 01:49:17]

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/2575173002/#ps240001
(title: ".")

[commit-bot: I haz the power, 2016-12-22 01:49:32]

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-22 01:55:42]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482371357075030,
"parent_rev": "e51b69e45dbd9ba3b27254ae187fb103bffb7020", "commit_rev":
"ed3b2d76836cf5a93f919c55eb33e63197047ca3"}

[commit-bot: I haz the power, 2016-12-22 01:56:18]

Description was changed from

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

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

to

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

BUG=653596

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

[commit-bot: I haz the power, 2016-12-22 01:56:19]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-12-22 02:00:15]

Description was changed from

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

BUG=653596

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

to

==========
[Extensions Bindings] Add a bridge to use current custom bindings

Even though, long-term, we want to move entirely away from our current
JS-style custom bindings, we need to support them in the short-term (so
that we aren't blocked on converting >100 files before moving to native
bindings as the default). This adds a bridge to be able to use current
custom bindings with the native bindings system.

The main step here is adding a bridge object. This is a gin::Wrappable
object that can be passed to the custom bindings instead of using
binding.generate() and allows custom bindings to register various hooks.

Also add an end-to-end test using custom bindings and the chrome.test
API. The chrome.test API is itself an extension API which relies on
custom bindings, so it's in many ways a good litmus test that this
works (on the downside - we needed the system in order to test the
system). In addition, add a more local test for custom bindings.

BUG=653596

Committed: https://crrev.com/7bf5512f524d82d6bb0a4e3dc6e56e2be9f0d2d8
Cr-Commit-Position: refs/heads/master@{#440310}
==========

[commit-bot: I haz the power, 2016-12-22 02:00:17]

Patchset 8 (id:??) landed as
https://crrev.com/7bf5512f524d82d6bb0a4e3dc6e56e2be9f0d2d8
Cr-Commit-Position: refs/heads/master@{#440310}