[Extensions Bindings] Allow for argument validation without conversion

extensions/renderer/api_signature.cc

Index: extensions/renderer/api_signature.cc
diff --git a/extensions/renderer/api_signature.cc b/extensions/renderer/api_signature.cc
index 0edca2f9bf199545c6b3e8f5ec9c65f140e221ca..e82759baaf8bc3cadf0eb76ab1959272e24d8532 100644
--- a/extensions/renderer/api_signature.cc
+++ b/extensions/renderer/api_signature.cc
@@ -21,12 +21,48 @@ APISignature::APISignature(const base::ListValue& specification) {
 
 APISignature::~APISignature() {}
 
-std::unique_ptr<base::ListValue> APISignature::ParseArguments(
+bool APISignature::ParseArgumentsToV8(gin::Arguments* arguments,

[jbroman, 2016/12/22 20:01:34]

Higher level thought on this (that may cancel some of my previous ones): it's
unfortunate that we have to have this much code that knows about both paths
explicitly (or at all, but we're stuck with that).

WDYT about instead making ParseArgumentsImpl a template that hides as much of
the difference as possible?

https://gist.github.com/jeremyroman/9c6445fcf988a88e9440c275f2471afe is a sketch
at what that might look like.

I'm not super attached to it, but it does trim out all of these functions having
two different kinds of out arguments and having to null check them all over the
place.

WDYT?

[Devlin, 2016/12/22 22:35:04]

Interesting idea!  I like it a lot, for the most part.  I've tweaked it a bit,
though.

In the next step, we need to support the hook updateArgumentsPreValidate(),
which, as the name implies, allows a hook to manipulate the raw arguments passed
to it prior to any validation.  This causes some messiness in a few areas, and
one of them is that it means that parsing arguments might not always take a
gin::Arguments.  In order to clean some of it up, I found it useful to have a
helper class assist in the argument parsing.

So, back to this patch (the rest will be more clear once I can upload it) - the
idioms you suggested work well with having that helper class, and just
subclassing it differently for V8 vs JSON parsing, so I've done that here rather
than causing even more churn once the next patch is ready.  I think this still
addresses the main concerns you had.  (It's also a bit small in binary size,
since we don't have to templatize the rather-complicated ParseArgument(s).)

Let me know what you think. :)

+                                      const ArgumentSpec::RefMap& type_refs,
+                                      std::vector<v8::Local<v8::Value>>* v8_out,
+                                      std::string* error) const {
+  return ParseArgumentsImpl(arguments, type_refs, v8_out, nullptr, nullptr,
+                            error);
+}
+
+bool APISignature::ParseArgumentsToJSON(
+    gin::Arguments* arguments,
+    const ArgumentSpec::RefMap& type_refs,
+    std::unique_ptr<base::ListValue>* json_out,
+    v8::Local<v8::Function>* callback_out,
+    std::string* error) const {
+  DCHECK(json_out);
+  DCHECK(callback_out);
+  return ParseArgumentsImpl(arguments, type_refs, nullptr, json_out,
+                            callback_out, error);
+}
+
+bool APISignature::ParseArgumentsImpl(
     gin::Arguments* arguments,
     const ArgumentSpec::RefMap& type_refs,
+    std::vector<v8::Local<v8::Value>>* v8_out,
+    std::unique_ptr<base::ListValue>* json_out,
     v8::Local<v8::Function>* callback_out,
     std::string* error) const {
-  auto results = base::MakeUnique<base::ListValue>();
+  DCHECK(error);
+  DCHECK_EQ(!!json_out, !!callback_out);

[jbroman, 2016/12/22 20:01:34]

I found this a little tough to read. Is this equivalent to:

if (json_out)
  DCHECK(callback_out);
else
  DCHECK(v8_out);

This is pretty close to what's already being DCHECKed below, so you could just
add a DCHECK(callback_out) to the json_out branch below.

[Devlin, 2016/12/22 22:35:04]

Moot, I think.

+  DCHECK_NE(!!v8_out, !!json_out);
+
+  std::unique_ptr<base::ListValue> json_results;
+  // Only create the base::Values and convert if we need to; otherwise just
+  // look at the v8 values.
+  if (json_out)
+    json_results = base::MakeUnique<base::ListValue>();
+  // Adding v8 values to a vector is cheap enough that we don't need to do it
+  // conditionally.
+  // TODO(devlin): This can also result in creating null values (for absent
+  // optional parameters). Is that expensive enough to warrant making this
+  // conditional?
+  std::vector<v8::Local<v8::Value>> v8_results;
 
   // TODO(devlin): This is how extension APIs have always determined if a
   // function has a callback, but it seems a little silly. In the long run (once
@@ -41,56 +77,77 @@ std::unique_ptr<base::ListValue> APISignature::ParseArguments(
   size_t end_size =
       signature_has_callback ? signature_.size() - 1 : signature_.size();
   for (size_t i = 0; i < end_size; ++i) {
-    std::unique_ptr<base::Value> parsed =
-        ParseArgument(*signature_[i], context, arguments, type_refs, error);
-    if (!parsed)
-      return nullptr;
-    results->Append(std::move(parsed));
+    if (!ParseArgument(*signature_[i], context, arguments, type_refs,
+                       &v8_results, json_results.get(), error)) {
+      return false;
+    }
   }
 
   v8::Local<v8::Function> callback_value;
   if (signature_has_callback &&
       !ParseCallback(arguments, *signature_.back(), error, &callback_value)) {
-    return nullptr;
+    return false;
   }
 
   if (!arguments->PeekNext().IsEmpty())
-    return nullptr;  // Extra arguments aren't allowed.
+    return false;  // Extra arguments aren't allowed.
+
+  if (json_out) {
+    if (!callback_value.IsEmpty())
+      *callback_out = callback_value;
+    *json_out = std::move(json_results);
+  } else {
+    DCHECK(v8_out);
+    if (!callback_value.IsEmpty())
+      v8_results.push_back(callback_value);
+    v8_out->swap(v8_results);
+  }
 
-  *callback_out = callback_value;
-  return results;
+  return true;
 }
 
-std::unique_ptr<base::Value> APISignature::ParseArgument(
-    const ArgumentSpec& spec,
-    v8::Local<v8::Context> context,
-    gin::Arguments* arguments,
-    const ArgumentSpec::RefMap& type_refs,
-    std::string* error) const {
+bool APISignature::ParseArgument(const ArgumentSpec& spec,
+                                 v8::Local<v8::Context> context,
+                                 gin::Arguments* arguments,
+                                 const ArgumentSpec::RefMap& type_refs,
+                                 std::vector<v8::Local<v8::Value>>* v8_out,
+                                 base::ListValue* json_out,
+                                 std::string* error) const {
   v8::Local<v8::Value> value = arguments->PeekNext();
   if (value.IsEmpty() || value->IsNull() || value->IsUndefined()) {
     if (!spec.optional()) {
       *error = "Missing required argument: " + spec.name();
-      return nullptr;
+      return false;
     }
     // This is safe to call even if |arguments| is at the end (which can happen
     // if n optional arguments are omitted at the end of the signature).
     arguments->Skip();
-    return base::Value::CreateNullValue();
+
+    v8_out->push_back(v8::Null(context->GetIsolate()));
+    if (json_out)
+      json_out->Append(base::Value::CreateNullValue());
+    return true;
   }
 
-  std::unique_ptr<base::Value> result =
-      spec.ConvertArgument(context, value, type_refs, error);
-  if (!result) {
+  std::unique_ptr<base::Value> json_arg;
+  if (!spec.ParseArgument(context, value, type_refs,
+                          json_out ? &json_arg : nullptr, error)) {
     if (!spec.optional()) {
       *error = "Missing required argument: " + spec.name();
-      return nullptr;
+      return false;
     }
-    return base::Value::CreateNullValue();
+
+    v8_out->push_back(v8::Null(context->GetIsolate()));

[jbroman, 2016/12/22 20:01:34]

It's strange that |v8_out| here is unconditionally null, and doesn't match how
the caller behaves.

[Devlin, 2016/12/22 22:35:04]

Moot, I think.

+    if (json_out)
+      json_out->Append(base::Value::CreateNullValue());
+    return true;
   }
 
+  v8_out->push_back(value);
+  if (json_out)
+    json_out->Append(std::move(json_arg));
   arguments->Skip();
-  return result;
+  return true;
 }
 
 bool APISignature::ParseCallback(gin::Arguments* arguments,