[Extensions Bindings] Allow for argument validation without conversion

extensions/renderer/api_signature.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_signature.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "gin/arguments.h"
 
 namespace extensions {
 
 APISignature::APISignature(const base::ListValue& specification) {
   signature_.reserve(specification.GetSize());
   for (const auto& value : specification) {
     const base::DictionaryValue* param = nullptr;
     CHECK(value->GetAsDictionary(&param));
     signature_.push_back(base::MakeUnique<ArgumentSpec>(*param));
   }
 }
 
 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 {
-  auto results = base::MakeUnique<base::ListValue>();
+  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 {
+  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
   // signatures are generated), it probably makes sense to indicate this
   // differently.
   bool signature_has_callback =
       !signature_.empty() &&
       signature_.back()->type() == ArgumentType::FUNCTION;
 
   v8::Local<v8::Context> context = arguments->isolate()->GetCurrentContext();
 
   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.
 
-  *callback_out = callback_value;
-  return results;
+  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);
+  }
+
+  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,
                                  const ArgumentSpec& callback_spec,
                                  std::string* error,
                                  v8::Local<v8::Function>* callback_out) const {
   v8::Local<v8::Value> value = arguments->PeekNext();
   if (value.IsEmpty() || value->IsNull() || value->IsUndefined()) {
     if (!callback_spec.optional()) {
       *error = "Missing required argument: " + callback_spec.name();
       return false;
     }
     arguments->Skip();
     return true;
   }
 
   if (!value->IsFunction()) {
     *error = "Argument is wrong type: " + callback_spec.name();
     return false;
   }
 
   *callback_out = value.As<v8::Function>();
   arguments->Skip();
   return true;
 }
 
 }  // namespace extensions