extensions: Use V8 Maybe APIs in NativeHandler

extensions/renderer/binding_generating_native_handler.cc

 // Copyright 2014 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/binding_generating_native_handler.h"
 
-#include "extensions/renderer/module_system.h"
+#include "extensions/renderer/script_context.h"
+#include "extensions/renderer/v8_helpers.h"
 
 namespace extensions {
 
+using namespace v8_helpers;
+
 BindingGeneratingNativeHandler::BindingGeneratingNativeHandler(
-    ModuleSystem* module_system,
+    ScriptContext* context,
     const std::string& api_name,
     const std::string& bind_to)
-    : module_system_(module_system), api_name_(api_name), bind_to_(bind_to) {}
+    : context_(context), api_name_(api_name), bind_to_(bind_to) {}
 
 v8::Local<v8::Object> BindingGeneratingNativeHandler::NewInstance() {

[not at google - send to devlin, 2015/06/23 17:26:22]

// This long sequence of commands effectively runs the JavaScript code,
// such that result[bind_to] is the compiled schema for |api_name|:
//
//   var result = {};
//   result[bind_to] = require('binding').Binding.create(api_name).generate();
//   return result;
//
// Unfortunately using the v8 APIs makes that quite verbose.
// Each stage is marked with the code it executes.

[bashi, 2015/06/24 00:04:32]

Done.

-  v8::Isolate* isolate = module_system_->GetIsolate();
+  v8::Isolate* isolate = context_->isolate();
   v8::EscapableHandleScope scope(isolate);
-  v8::Local<v8::Object> binding_module =
-      module_system_->Require("binding")->ToObject(isolate);
-  v8::Local<v8::Object> binding =
-      binding_module->Get(v8::String::NewFromUtf8(isolate, "Binding"))
-          ->ToObject(isolate);
+
+  v8::Local<v8::String> v8_api_name;

[not at google - send to devlin, 2015/06/23 17:26:23]

See above, I think these stages should be associated with the JavaScript code
they execute. Sorry about making you do this! But with the increased verbosity I
think it would make it much easier to read. Here,

// Convert |api_name| and |bind_to| into their v8::Strings to pass
// through the v8 APIs.

[bashi, 2015/06/24 00:04:32]

Done.

+  v8::Local<v8::String> v8_bind_to;
+  if (!ToV8String(isolate, api_name_.c_str(), &v8_api_name) ||
+      !ToV8String(isolate, bind_to_.c_str(), &v8_bind_to)) {

[not at google - send to devlin, 2015/06/23 17:26:23]

Perhaps add a ToV8String() variation that takes a std::string?

[bashi, 2015/06/24 00:04:32]

Done.

+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
+
+  v8::Local<v8::Context> v8_context = context_->v8_context();

[not at google - send to devlin, 2015/06/23 17:26:23]

// require('binding');

+  v8::Local<v8::Object> binding_module;
+  if (!context_->module_system()->Require("binding")
+           ->ToObject(v8_context)

[not at google - send to devlin, 2015/06/23 17:26:22]

Hm, Require() should really return a v8::Object because that is the contract of
commonjs - it returns the export object. Could you change ModuleSystem::Require
to return a v8::Object (with whatever CHECKs are appropriate)?

[bashi, 2015/06/24 00:04:32]

Hmm, ModuleSystem::Require() returns what ModuleSystem::RequireForJsInner()
returns, and it seems that it could return undefined in some cases. Should we
really change the return value type? If we want to return a v8::Object, I think
we should return MaybeLocal<v8::Object> because it enforces call-sites to do
empty checks. WDYT?

[not at google - send to devlin, 2015/06/24 00:34:56]

I see... maybe because require can fail? A Maybe<Object> makes sense then... I
hope that there aren't any call sites to change other than this one. It doesn't
look like it.

[bashi, 2015/06/24 03:30:44]

Done. I needed to change some other places. RequireFunction now returns
MaybeLocal. Could you take another look?

+           .ToLocal(&binding_module)) {
+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
+
+  v8::Local<v8::Value> binding_value;

[not at google - send to devlin, 2015/06/23 17:26:22]

// require('binding').Binding;

+  v8::Local<v8::Object> binding;
+  if (!GetProperty(v8_context, binding_module, "Binding", &binding_value) ||
+      !binding_value->ToObject(v8_context).ToLocal(&binding)) {

[not at google - send to devlin, 2015/06/23 17:26:23]

Is |binding_value| guaranteed to be an Object? What happens if it isn't?
Likewise below with |binding_instance_value|.

[bashi, 2015/06/24 00:04:32]

It depends on what "require('binding').Binding" returns.

My understanding is that ToObject() does what the spec says.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-toobject

If ToObject().ToLocal() succeeds there is no exception and |binding| is
guaranteed to be an object.

[not at google - send to devlin, 2015/06/24 00:34:56]

ok, cool.

+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
+
+  v8::Local<v8::Value> create_binding_value;

[not at google - send to devlin, 2015/06/23 17:26:22]

// require('Binding').Binding.create;

[bashi, 2015/06/24 00:04:32]

Done.

+  if (!GetProperty(v8_context, binding, "create", &create_binding_value) ||
+      !create_binding_value->IsFunction()) {
+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
   v8::Local<v8::Function> create_binding =
-      binding->Get(v8::String::NewFromUtf8(isolate, "create"))
-          .As<v8::Function>();
-  v8::Local<v8::Value> argv[] = {
-      v8::String::NewFromUtf8(isolate, api_name_.c_str())};
-  v8::Local<v8::Object> binding_instance =
-      create_binding->Call(binding, arraysize(argv), argv)->ToObject(isolate);
-  v8::Local<v8::Function> generate =
-      binding_instance->Get(v8::String::NewFromUtf8(isolate, "generate"))
-          .As<v8::Function>();
+      create_binding_value.As<v8::Function>();
+
+  v8::Local<v8::Value> argv[] = {v8_api_name};

[not at google - send to devlin, 2015/06/23 17:26:22]

// require('Binding').Binding.create(api_name);

[bashi, 2015/06/24 00:04:32]

Done.

+  v8::Local<v8::Value> binding_instance_value;
+  v8::Local<v8::Object> binding_instance;
+  if (!CallFunction(v8_context, create_binding, binding, arraysize(argv), argv,
+                    &binding_instance_value) ||
+      !binding_instance_value->ToObject(v8_context)
+           .ToLocal(&binding_instance)) {
+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
+
+  v8::Local<v8::Value> generate_value;

[not at google - send to devlin, 2015/06/23 17:26:23]

// require('Binding').Binding.create(api_name).generate;

[bashi, 2015/06/24 00:04:32]

Done.

+  if (!GetProperty(v8_context, binding_instance, "generate", &generate_value) ||
+      !generate_value->IsFunction()) {
+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
+  v8::Local<v8::Function> generate = generate_value.As<v8::Function>();
+
   v8::Local<v8::Object> object = v8::Object::New(isolate);

[not at google - send to devlin, 2015/06/23 17:26:23]

// require('Binding').Binding.create(api_name).generate();

[bashi, 2015/06/24 00:04:32]

Done.

-  v8::Local<v8::Value> compiled_schema =
-      generate->Call(binding_instance, 0, NULL);
-  if (!compiled_schema.IsEmpty()) {
-    object->Set(v8::String::NewFromUtf8(isolate, bind_to_.c_str()),
-                compiled_schema);
+  v8::Local<v8::Value> compiled_schema;
+  if (!CallFunction(v8_context, generate, binding_instance, 0, nullptr,
+                    &compiled_schema)) {
+    NOTREACHED();
+    return v8::Local<v8::Object>();
+  }
+
+  if (!SetProperty(v8_context, object, v8_bind_to, compiled_schema)) {

[not at google - send to devlin, 2015/06/23 17:26:23]

// var result = {};
// result[bind_to] = ...;

[bashi, 2015/06/24 00:04:32]

Done.

+    NOTREACHED();
+    return v8::Local<v8::Object>();
   }
   return scope.Escape(object);

[not at google - send to devlin, 2015/06/23 17:26:22]

// var result = {};
// result[bind_to] = ...;
// return result;

[bashi, 2015/06/24 00:04:32]

Done.

 }
 
 }  // namespace extensions