[Extensions Bindings] Add APIBindingsSystem

[Extensions Bindings] Add APIBindingsSystem

Add an APIBindingsSystem class that contains the necessary bits and
pieces to construct the extension API bindings (the request handler,
reference map, and collection of APIBinding objects). This object can
vend API instances for a given API name, notifies when an API method is
called, and fields any callbacks into the request handler.

Add tests for the same, including one which demonstrates how we might
set this up in the real world, i.e. using actual Extension APIs.

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

[Devlin, 2016-10-21 17:46:48]

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

[commit-bot: I haz the power, 2016-10-21 17:47:33]

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-10-21 19:32:54]

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

[commit-bot: I haz the power, 2016-10-21 19:32:56]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-10-21 19:36:01]

Description was changed from

==========
wip

BUG=
==========

to

==========
[Extensions Bindings] Add APIBindingsSystem

Add an APIBindingsSystem class that contains the necessary bits and
pieces to construct the extension API bindings (the request handler,
reference map, and collection of APIBinding objects). This object can
vend API instances for a given API name, notifies when an API method is
called, and fields any callbacks into the request handler.

Add tests for the same, including one which demonstrates how we might
set this up in the real world, i.e. using actual Extension APIs.

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

[Devlin, 2016-10-21 19:37:15]

Patchset #2 (id:20001) has been deleted

[Devlin, 2016-10-21 19:37:20]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-10-21 19:37:53]

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

[Devlin, 2016-10-21 19:37:54]

lazyboy@ and jbroman@, mind taking a look?

[jbroman, 2016-10-21 22:53:59]

getting on a plane shortly, but here's a few thoughts

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.cc:51: : static_cast<const
base::ListValue&>(base::ListValue()),
Is this cast necessary? I'd have expected it to compile just fine without the
cast.

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.cc:61: auto request =
base::MakeUnique<Request>();
nit: Will the non-test |send_request_| actually need to take ownership of one of
these? I'd have expected it to only need a const ref (in which case this object
could just be stack-allocated). The test one could make a copy or use a mock
without imposing another per-request allocation.

(OTOH, given the number of other allocations going on at present, this one
doesn't add much cost.)

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.h:41: using GetAPISchemaMethod =
I take it you expect the real implementation to be more complicated than a map
lookup? Otherwise I'd just expect a map (or a reference thereto) to be passed
instead.

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.h:55: APIRequestHandler*
request_handler() { return request_handler_.get(); }
nit: It seems a little asymmetric that users of this object send requests using
a callback provided to this class, but to respond they have to get the
APIRequestHandler*; this class kind of hides APIRequestHandler but kind of
doesn't.

Why not have APIBindingsSystem take the CallJSFunction (or similar) directly,
and then have it just have APIRequestHandler as a class member. It seems like it
would simplify the use of this class (from a Law-of-Demeter-ish perspective).

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec.cc:96: // Enum entries come in two versions: a
list of possible strings, and
Could this and its unit test be in another CL? It doesn't seem related to the
other changes.

[Devlin, 2016-10-22 00:49:15]

I'll rebase and upload a new PS once I pull out the argument spec changes into
another cl.

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.cc:51: : static_cast<const
base::ListValue&>(base::ListValue()),
On 2016/10/21 22:53:59, jbroman wrote:
> Is this cast necessary? I'd have expected it to compile just fine without the
> cast.

I think this is one of those times that the compiler isn't *quite* smart enough.
 It first tries to determine the result of the ternary, which is either a const
base::ListValue& (*type_definitions) or a base::ListValue& (the
base::ListValue()).  Since it can't, it throws "incompatible operand types". 
Eventually, I'm sure clang will be smart enough to check where we're using the
result (which can only be a const base::ListValue&), but we're sadly not there
yet.

We could hypothetically also do:
base::ListValue empty_args;
return base::MakeUnique<>(..., type_definitions ? *type_definitions :
empty_args)
but then we have to alloc empty_args in all cases.  (Fun side note: It's very
strange that this compiles, since it more-or-less the same values as what we
have now without the cast.  A local guru's hypothesis is that this is because,
in the empty_args case, both operands are L-Values.)

Option 3, we could change the APIBinding ctor to take a const base::ListValue*
and just pass in type_definitions directly.

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.cc:61: auto request =
base::MakeUnique<Request>();
On 2016/10/21 22:53:59, jbroman wrote:
> nit: Will the non-test |send_request_| actually need to take ownership of one
of
> these? I'd have expected it to only need a const ref (in which case this
object
> could just be stack-allocated). The test one could make a copy or use a mock
> without imposing another per-request allocation.
> 
> (OTOH, given the number of other allocations going on at present, this one
> doesn't add much cost.)

It's a little dependent on how everything shakes out.  If we eventually replace
Request with ExtensionHostMsg_Request_Params, this is more doable.  Otherwise,
the ability to Swap() the storage of the list value would be pretty beneficial,
which is what I'm thinking right now.  A non-const & would be pretty helpful
here, but against style.

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.h:41: using GetAPISchemaMethod =
On 2016/10/21 22:53:59, jbroman wrote:
> I take it you expect the real implementation to be more complicated than a map
> lookup? Otherwise I'd just expect a map (or a reference thereto) to be passed
> instead.

You can see the current "real" implementation in the
APIBindingsSystemTestWithRealAPI. Basically, it boils down to a fetch from the
ExtensionAPI object.  Looking down the road to where everything will be
generated, it's possible this could just be a map, but that would make testing
with real APIs much harder at the moment.

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.h:55: APIRequestHandler*
request_handler() { return request_handler_.get(); }
On 2016/10/21 22:53:59, jbroman wrote:
> nit: It seems a little asymmetric that users of this object send requests
using
> a callback provided to this class, but to respond they have to get the
> APIRequestHandler*; this class kind of hides APIRequestHandler but kind of
> doesn't.
> 
> Why not have APIBindingsSystem take the CallJSFunction (or similar) directly,
> and then have it just have APIRequestHandler as a class member. It seems like
it
> would simplify the use of this class (from a Law-of-Demeter-ish perspective).

I was a little torn on this.  This works, but means that we expose a
"CompleteRequest" wrapper in APIBindingsSystem.  But since you're echoing my
thoughts about this being a little weird, it seems like it might be worth it. :)
 Done.

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec.cc:96: // Enum entries come in two versions: a
list of possible strings, and
On 2016/10/21 22:53:59, jbroman wrote:
> Could this and its unit test be in another CL? It doesn't seem related to the
> other changes.

Sure, I'll pull them out.  They're still needed here for the real schemas to
parse, but I'll rebase once I have the other CL up.

[Devlin, 2016-10-22 00:55:21]

On 2016/10/22 00:49:15, Devlin wrote:
> I'll rebase and upload a new PS once I pull out the argument spec changes into
> another cl.

Rebased. :)

[Devlin, 2016-10-26 00:19:51]

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

[commit-bot: I haz the power, 2016-10-26 00:20:38]

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-10-26 02:48:13]

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

[commit-bot: I haz the power, 2016-10-26 02:48:14]

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

[Devlin, 2016-10-26 20:17:03]

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.cc:51: : static_cast<const
base::ListValue&>(base::ListValue()),
On 2016/10/22 00:49:15, Devlin wrote:
> On 2016/10/21 22:53:59, jbroman wrote:
> > Is this cast necessary? I'd have expected it to compile just fine without
the
> > cast.
> 
> I think this is one of those times that the compiler isn't *quite* smart
enough.
>  It first tries to determine the result of the ternary, which is either a
const
> base::ListValue& (*type_definitions) or a base::ListValue& (the
> base::ListValue()).  Since it can't, it throws "incompatible operand types". 
> Eventually, I'm sure clang will be smart enough to check where we're using the
> result (which can only be a const base::ListValue&), but we're sadly not there
> yet.
> 
> We could hypothetically also do:
> base::ListValue empty_args;
> return base::MakeUnique<>(..., type_definitions ? *type_definitions :
> empty_args)
> but then we have to alloc empty_args in all cases.  (Fun side note: It's very
> strange that this compiles, since it more-or-less the same values as what we
> have now without the cast.  A local guru's hypothesis is that this is because,
> in the empty_args case, both operands are L-Values.)
> 
> Option 3, we could change the APIBinding ctor to take a const base::ListValue*
> and just pass in type_definitions directly.

FYI, I decided option 3 here was best.

[jbroman, 2016-10-31 14:16:58]

Back from vacation. This looks reasonable, lgtm.

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

https://codereview.chromium.org/2438623002/diff/40001/extensions/renderer/api...
extensions/renderer/api_bindings_system.cc:51: : static_cast<const
base::ListValue&>(base::ListValue()),
On 2016/10/26 at 20:17:03, Devlin wrote:
> On 2016/10/22 00:49:15, Devlin wrote:
> > On 2016/10/21 22:53:59, jbroman wrote:
> > > Is this cast necessary? I'd have expected it to compile just fine without
the
> > > cast.
> > 
> > I think this is one of those times that the compiler isn't *quite* smart
enough.
> >  It first tries to determine the result of the ternary, which is either a
const
> > base::ListValue& (*type_definitions) or a base::ListValue& (the
> > base::ListValue()).  Since it can't, it throws "incompatible operand types".

> > Eventually, I'm sure clang will be smart enough to check where we're using
the
> > result (which can only be a const base::ListValue&), but we're sadly not
there
> > yet.
> > 
> > We could hypothetically also do:
> > base::ListValue empty_args;
> > return base::MakeUnique<>(..., type_definitions ? *type_definitions :
> > empty_args)
> > but then we have to alloc empty_args in all cases.  (Fun side note: It's
very
> > strange that this compiles, since it more-or-less the same values as what we
> > have now without the cast.  A local guru's hypothesis is that this is
because,
> > in the empty_args case, both operands are L-Values.)
> > 
> > Option 3, we could change the APIBinding ctor to take a const
base::ListValue*
> > and just pass in type_definitions directly.
> 
> FYI, I decided option 3 here was best.

Ahh, I see what's going on here. Ordinarily this would be fine (with
std::string, for instance, it would work), because the compiler is smart enough
to convert both to const refs.

Unfortunately, it does so through copy-initialization (of the temporary), but
base::ListValue has a deleted copy constructor, so that fails. Not wanting to
bikeshed this more, that sgtm (and the original would be fine too).

[lazyboy, 2016-10-31 20:08:31]

lgtm with nits.

https://codereview.chromium.org/2438623002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_bindings_system.h (right):

https://codereview.chromium.org/2438623002/diff/80001/extensions/renderer/api...
extensions/renderer/api_bindings_system.h:65: // Handles a call into an API,
adds a pending request to the request_handler_,
nit: |request_handler_|, |send_request_|

https://codereview.chromium.org/2438623002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2438623002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_bindings_system_unittest.cc:188:
APIBindingsSystemTestBase::SetUp();
Better if this goes at the end of the func, after api_schemas_ is initialized,
then GetAPISchema() is guaranteed to always work.

[Devlin, 2016-10-31 20:40:12]

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

[commit-bot: I haz the power, 2016-10-31 20:40: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-10-31 22:03:37]

https://codereview.chromium.org/2438623002/diff/80001/extensions/renderer/api...
File extensions/renderer/api_bindings_system.h (right):

https://codereview.chromium.org/2438623002/diff/80001/extensions/renderer/api...
extensions/renderer/api_bindings_system.h:65: // Handles a call into an API,
adds a pending request to the request_handler_,
On 2016/10/31 20:08:31, lazyboy wrote:
> nit: |request_handler_|, |send_request_|

Done.

https://codereview.chromium.org/2438623002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2438623002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_bindings_system_unittest.cc:188:
APIBindingsSystemTestBase::SetUp();
On 2016/10/31 20:08:31, lazyboy wrote:
> Better if this goes at the end of the func, after api_schemas_ is initialized,
> then GetAPISchema() is guaranteed to always work.

Done.

[Devlin, 2016-10-31 22:03:42]

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

[Devlin, 2016-10-31 22:03:46]

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

[Devlin, 2016-10-31 22:03:47]

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

[commit-bot: I haz the power, 2016-10-31 22:04:08]

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-10-31 22:41:59]

Description was changed from

==========
[Extensions Bindings] Add APIBindingsSystem

Add an APIBindingsSystem class that contains the necessary bits and
pieces to construct the extension API bindings (the request handler,
reference map, and collection of APIBinding objects). This object can
vend API instances for a given API name, notifies when an API method is
called, and fields any callbacks into the request handler.

Add tests for the same, including one which demonstrates how we might
set this up in the real world, i.e. using actual Extension APIs.

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

to

==========
[Extensions Bindings] Add APIBindingsSystem

Add an APIBindingsSystem class that contains the necessary bits and
pieces to construct the extension API bindings (the request handler,
reference map, and collection of APIBinding objects). This object can
vend API instances for a given API name, notifies when an API method is
called, and fields any callbacks into the request handler.

Add tests for the same, including one which demonstrates how we might
set this up in the real world, i.e. using actual Extension APIs.

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

[commit-bot: I haz the power, 2016-10-31 22:42:00]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-10-31 22:45:49]

Description was changed from

==========
[Extensions Bindings] Add APIBindingsSystem

Add an APIBindingsSystem class that contains the necessary bits and
pieces to construct the extension API bindings (the request handler,
reference map, and collection of APIBinding objects). This object can
vend API instances for a given API name, notifies when an API method is
called, and fields any callbacks into the request handler.

Add tests for the same, including one which demonstrates how we might
set this up in the real world, i.e. using actual Extension APIs.

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

to

==========
[Extensions Bindings] Add APIBindingsSystem

Add an APIBindingsSystem class that contains the necessary bits and
pieces to construct the extension API bindings (the request handler,
reference map, and collection of APIBinding objects). This object can
vend API instances for a given API name, notifies when an API method is
called, and fields any callbacks into the request handler.

Add tests for the same, including one which demonstrates how we might
set this up in the real world, i.e. using actual Extension APIs.

BUG=653596

Committed: https://crrev.com/7616e8614f857f4366ca2127e9a7ea1fc2f48a6e
Cr-Commit-Position: refs/heads/master@{#428851}
==========

[commit-bot: I haz the power, 2016-10-31 22:45:50]

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