[Extensions Bindings] Add support for updateArgumentsPreValidate

extensions/renderer/api_binding_unittest.cc

Index: extensions/renderer/api_binding_unittest.cc
diff --git a/extensions/renderer/api_binding_unittest.cc b/extensions/renderer/api_binding_unittest.cc
index 114915c6b91620193f65293d266d0f6fc3fd778d..a333a9264e7cf029e965c69ce678ff5025143c67 100644
--- a/extensions/renderer/api_binding_unittest.cc
+++ b/extensions/renderer/api_binding_unittest.cc
@@ -196,7 +196,7 @@ TEST_F(APIBindingUnittest, Test) {
   APIBinding binding(
       "test", *functions, nullptr, nullptr,
       base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
-      base::MakeUnique<APIBindingHooks>(binding::RunJSFunction()), &refs);
+      base::MakeUnique<APIBindingHooks>(binding::RunJSFunctionSync()), &refs);
   EXPECT_TRUE(refs.empty());
 
   v8::HandleScope handle_scope(isolate());
@@ -297,7 +297,7 @@ TEST_F(APIBindingUnittest, TypeRefsTest) {
   APIBinding binding(
       "test", *functions, types.get(), nullptr,
       base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
-      base::MakeUnique<APIBindingHooks>(binding::RunJSFunction()), &refs);
+      base::MakeUnique<APIBindingHooks>(binding::RunJSFunctionSync()), &refs);
   EXPECT_EQ(2u, refs.size());
   EXPECT_TRUE(base::ContainsKey(refs, "refObj"));
   EXPECT_TRUE(base::ContainsKey(refs, "refEnum"));
@@ -343,7 +343,7 @@ TEST_F(APIBindingUnittest, RestrictedAPIs) {
   APIBinding binding(
       "test", *functions, nullptr, nullptr,
       base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
-      base::MakeUnique<APIBindingHooks>(binding::RunJSFunction()), &refs);
+      base::MakeUnique<APIBindingHooks>(binding::RunJSFunctionSync()), &refs);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = ContextLocal();
@@ -386,7 +386,7 @@ TEST_F(APIBindingUnittest, TestEventCreation) {
   APIBinding binding(
       "test", *functions, nullptr, events.get(),
       base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
-      base::MakeUnique<APIBindingHooks>(binding::RunJSFunction()), &refs);
+      base::MakeUnique<APIBindingHooks>(binding::RunJSFunctionSync()), &refs);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = ContextLocal();
@@ -422,7 +422,7 @@ TEST_F(APIBindingUnittest, TestDisposedContext) {
   APIBinding binding(
       "test", *functions, nullptr, nullptr,
       base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
-      base::MakeUnique<APIBindingHooks>(binding::RunJSFunction()), &refs);
+      base::MakeUnique<APIBindingHooks>(binding::RunJSFunctionSync()), &refs);
   EXPECT_TRUE(refs.empty());
 
   v8::HandleScope handle_scope(isolate());
@@ -451,16 +451,18 @@ TEST_F(APIBindingUnittest, TestCustomHooks) {
 
   // Register a hook for the test.oneString method.
   auto hooks = base::MakeUnique<APIBindingHooks>(
-      base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle));
   bool did_call = false;
   auto hook = [](bool* did_call, const APISignature* signature,
-                 gin::Arguments* arguments,
+                 v8::Local<v8::Context> context,
+                 std::vector<v8::Local<v8::Value>>* arguments,
                  const ArgumentSpec::RefMap& ref_map) {
     *did_call = true;
-    EXPECT_EQ(1, arguments->Length());
-    std::string argument;
-    EXPECT_TRUE(arguments->GetNext(&argument));
-    EXPECT_EQ("foo", argument);
+    if (arguments->size() != 1u) {
+      EXPECT_EQ(1u, arguments->size());
+      return APIBindingHooks::RequestResult::HANDLED;
+    }
+    EXPECT_EQ("foo", gin::V8ToString(arguments->at(0)));
     return APIBindingHooks::RequestResult::HANDLED;
   };
   hooks->RegisterHandleRequest("test.oneString", base::Bind(hook, &did_call));
@@ -494,7 +496,7 @@ TEST_F(APIBindingUnittest, TestCustomHooks) {
 TEST_F(APIBindingUnittest, TestJSCustomHook) {
   // Register a hook for the test.oneString method.
   auto hooks = base::MakeUnique<APIBindingHooks>(
-      base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle));
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = ContextLocal();
@@ -552,4 +554,67 @@ TEST_F(APIBindingUnittest, TestJSCustomHook) {
   ExpectPass(binding_object, "obj.stringAndInt('foo', 42);", "['foo',42]");
 }
 
+// Tests the updateArgumentsPreValidate hook.

[jbroman, 2017/01/02 19:46:38]

It looks like it's legal for this hook to throw an exception. Should we at least
have a unit test for the case where it does?

[Devlin, 2017/01/04 17:57:02]

Done.

+TEST_F(APIBindingUnittest, TestUpdateArgumentsPreValidate) {
+  // Register a hook for the test.oneString method.
+  auto hooks = base::MakeUnique<APIBindingHooks>(
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle));
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+  const char kRegisterHook[] =
+      "(function(hooks) {\n"
+      "  hooks.setUpdateArgumentsPreValidate('oneString', function() {\n"
+      "    this.requestArguments = Array.from(arguments);\n"
+      "    if (this.requestArguments[0] === true)\n"
+      "      return ['hooked']\n"
+      "    return this.requestArguments\n"
+      "  });\n"
+      "})";
+  v8::Local<v8::String> source_string =
+      gin::StringToV8(isolate(), kRegisterHook);
+  v8::Local<v8::String> source_name =
+      gin::StringToV8(isolate(), "custom_hook");
+  hooks->RegisterJsSource(
+      v8::Global<v8::String>(isolate(), source_string),
+      v8::Global<v8::String>(isolate(), source_name));
+
+  std::unique_ptr<base::ListValue> functions = ListValueFromString(kFunctions);
+  ASSERT_TRUE(functions);
+  ArgumentSpec::RefMap refs;
+
+  APIBinding binding(
+      "test", *functions, nullptr, nullptr,
+      base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
+      std::move(hooks), &refs);
+  EXPECT_TRUE(refs.empty());
+
+  APIEventHandler event_handler(
+      base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+  v8::Local<v8::Object> binding_object = binding.CreateInstance(
+      context, isolate(), &event_handler, base::Bind(&AllowAllAPIs));
+
+  // Call the method with a hook. Since the hook updates arguments before
+  // validation, we should be able to pass in invalid arguments and still
+  // have the hook called.
+  ExpectFailure(binding_object, "obj.oneString(false);", kError);
+  // TODO(devlin): Add a utility method to retrieve a stringified property from
+  // an object; we do this enough.
+  std::unique_ptr<base::Value> response_args =
+      GetBaseValuePropertyFromObject(context->Global(), context,
+                                     "requestArguments");
+  ASSERT_TRUE(response_args);
+  EXPECT_EQ("[false]", ValueToString(*response_args));
+
+  ExpectPass(binding_object, "obj.oneString(true);", "['hooked']");
+  response_args =
+      GetBaseValuePropertyFromObject(context->Global(), context,
+                                     "requestArguments");
+  ASSERT_TRUE(response_args);
+  EXPECT_EQ("[true]", ValueToString(*response_args));
+
+  // Other methods, like stringAndInt(), should behave normally.
+  ExpectPass(binding_object, "obj.stringAndInt('foo', 42);", "['foo',42]");
+}
+
 }  // namespace extensions