[Extensions Bindings] Add APIBindingsSystem

extensions/renderer/api_bindings_system.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/renderer/api_bindings_system.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/values.h"
+#include "extensions/renderer/api_binding.h"
+#include "extensions/renderer/api_request_handler.h"
+
+namespace extensions {
+
+APIBindingsSystem::Request::Request() {}
+APIBindingsSystem::Request::~Request() {}
+
+APIBindingsSystem::APIBindingsSystem(
+    std::unique_ptr<APIRequestHandler> request_handler,
+    const GetAPISchemaMethod& get_api_schema,
+    const SendRequestMethod& send_request)
+    : request_handler_(std::move(request_handler)),
+      get_api_schema_(get_api_schema),
+      send_request_(send_request) {}
+
+APIBindingsSystem::~APIBindingsSystem() {}
+
+v8::Local<v8::Object> APIBindingsSystem::CreateAPIInstance(
+    const std::string& api_name,
+    v8::Local<v8::Context> context,
+    v8::Isolate* isolate) {
+  std::unique_ptr<APIBinding>& binding = api_bindings_[api_name];
+  if (!binding)
+    binding = CreateNewAPIBinding(api_name);
+  return binding->CreateInstance(context, isolate);
+}
+
+std::unique_ptr<APIBinding> APIBindingsSystem::CreateNewAPIBinding(
+    const std::string& api_name) {
+  const base::DictionaryValue& api_schema = get_api_schema_.Run(api_name);
+
+  const base::ListValue* function_definitions = nullptr;
+  CHECK(api_schema.GetList("functions", &function_definitions));
+  const base::ListValue* type_definitions = nullptr;
+  // Type definitions might not exist for the given API.
+  api_schema.GetList("types", &type_definitions);
+
+  return base::MakeUnique<APIBinding>(
+      api_name, *function_definitions,
+      type_definitions ? *type_definitions
+                       : static_cast<const base::ListValue&>(base::ListValue()),

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

Is this cast necessary? I'd have expected it to compile just fine without the
cast.

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

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.

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

FYI, I decided option 3 here was best.

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

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).

+      base::Bind(&APIBindingsSystem::OnAPICall, base::Unretained(this)),
+      &type_reference_map_);
+}
+
+void APIBindingsSystem::OnAPICall(const std::string& name,
+                                  std::unique_ptr<base::ListValue> arguments,
+                                  v8::Isolate* isolate,
+                                  v8::Local<v8::Context> context,
+                                  v8::Local<v8::Function> callback) {
+  auto request = base::MakeUnique<Request>();

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

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.)

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

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.

+  if (!callback.IsEmpty()) {
+    request->request_id =
+        request_handler_->AddPendingRequest(isolate, callback, context);
+  }
+  request->arguments = std::move(arguments);
+  request->method_name = name;
+
+  send_request_.Run(std::move(request));
+}
+
+}  // namespace extensions