extensions: Use V8 Maybe APIs in NativeHandler

extensions: Use V8 Maybe APIs in NativeHandler

Also make NativeHandler::NewInstance() return MaybeLocal<Object>
because it can fail while running V8 APIs.

BUG=479065
Committed: https://crrev.com/61ca3c7b545104ac3edda633a80a62808d281b23
Cr-Commit-Position: refs/heads/master@{#336302}

[bashi, 2015-06-19 08:45:24]

bashi@chromium.org changed reviewers:
+ kalman@chromium.org

[bashi, 2015-06-19 08:45:24]

PTAL?

[not at google - send to devlin, 2015-06-19 17:23:55]

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
extensions/renderer/binding_generating_native_handler.cc:20:
v8::MaybeLocal<v8::Object> BindingGeneratingNativeHandler::NewInstance() {
NewInstance doesn't run any third-party code so it should be safe to return a
Local and CHECK that it's never a Maybe value. If our internal code throws an
exception we're screwed. It would make for a nicer interface.

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
extensions/renderer/binding_generating_native_handler.cc:30:
v8::Local<v8::Context> context = isolate->GetCurrentContext();
Use the context() property rather than the current context?

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/module_...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:568: LOG(ERROR) << "Cannot instanciate
native for requireNative(" + native_name +
"instantiate", and NOTREACHED(), but might be moot. Also use << not +.

[bashi, 2015-06-23 00:10:49]

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
extensions/renderer/binding_generating_native_handler.cc:20:
v8::MaybeLocal<v8::Object> BindingGeneratingNativeHandler::NewInstance() {
On 2015/06/19 17:23:54, kalman wrote:
> NewInstance doesn't run any third-party code so it should be safe to return a
> Local and CHECK that it's never a Maybe value. If our internal code throws an
> exception we're screwed. It would make for a nicer interface.

Done.

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
extensions/renderer/binding_generating_native_handler.cc:30:
v8::Local<v8::Context> context = isolate->GetCurrentContext();
On 2015/06/19 17:23:54, kalman wrote:
> Use the context() property rather than the current context?

BindingGeneratingNativeHandler doesn't have context() property. Get a context
from ModuleSystem? We need to expose context() if so.

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/module_...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:568: LOG(ERROR) << "Cannot instanciate
native for requireNative(" + native_name +
On 2015/06/19 17:23:55, kalman wrote:
> "instantiate", and NOTREACHED(), but might be moot. Also use << not +.

Removed because NewInstance() returns Local now.

[not at google - send to devlin, 2015-06-23 00:36:48]

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
extensions/renderer/binding_generating_native_handler.cc:30:
v8::Local<v8::Context> context = isolate->GetCurrentContext();
On 2015/06/23 00:10:49, bashi1 wrote:
> On 2015/06/19 17:23:54, kalman wrote:
> > Use the context() property rather than the current context?
> 
> BindingGeneratingNativeHandler doesn't have context() property. Get a context
> from ModuleSystem? We need to expose context() if so.

Ah right. It should be pretty straightforward to change
BindingGeneratingNativeHandler to be constructed with a ScriptContext instead of
a ModuleSystem.

[bashi, 2015-06-23 00:49:41]

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/1/extensions/renderer/binding...
extensions/renderer/binding_generating_native_handler.cc:30:
v8::Local<v8::Context> context = isolate->GetCurrentContext();
On 2015/06/23 00:36:48, kalman wrote:
> On 2015/06/23 00:10:49, bashi1 wrote:
> > On 2015/06/19 17:23:54, kalman wrote:
> > > Use the context() property rather than the current context?
> > 
> > BindingGeneratingNativeHandler doesn't have context() property. Get a
context
> > from ModuleSystem? We need to expose context() if so.
> 
> Ah right. It should be pretty straightforward to change
> BindingGeneratingNativeHandler to be constructed with a ScriptContext instead
of
> a ModuleSystem.

Done. Thanks for the suggestion!

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

Last round. Sorry about the length of this review. It's very tricky code and I
needed to refresh my memory.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:20:
v8::Local<v8::Object> BindingGeneratingNativeHandler::NewInstance() {
// 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.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:24:
v8::Local<v8::String> v8_api_name;
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.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:27:
!ToV8String(isolate, bind_to_.c_str(), &v8_bind_to)) {
Perhaps add a ToV8String() variation that takes a std::string?

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:32:
v8::Local<v8::Context> v8_context = context_->v8_context();
// require('binding');

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:35:
->ToObject(v8_context)
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)?

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:41:
v8::Local<v8::Value> binding_value;
// require('binding').Binding;

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:44:
!binding_value->ToObject(v8_context).ToLocal(&binding)) {
Is |binding_value| guaranteed to be an Object? What happens if it isn't?
Likewise below with |binding_instance_value|.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:49:
v8::Local<v8::Value> create_binding_value;
// require('Binding').Binding.create;

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:58:
v8::Local<v8::Value> argv[] = {v8_api_name};
// require('Binding').Binding.create(api_name);

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:69:
v8::Local<v8::Value> generate_value;
// require('Binding').Binding.create(api_name).generate;

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:77:
v8::Local<v8::Object> object = v8::Object::New(isolate);
// require('Binding').Binding.create(api_name).generate();

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:85: if
(!SetProperty(v8_context, object, v8_bind_to, compiled_schema)) {
// var result = {};
// result[bind_to] = ...;

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:89: return
scope.Escape(object);
// var result = {};
// result[bind_to] = ...;
// return result;

[bashi, 2015-06-24 00:04:33]

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:20:
v8::Local<v8::Object> BindingGeneratingNativeHandler::NewInstance() {
On 2015/06/23 17:26:22, kalman wrote:
> // 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.

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:24:
v8::Local<v8::String> v8_api_name;
On 2015/06/23 17:26:23, kalman wrote:
> 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.

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:27:
!ToV8String(isolate, bind_to_.c_str(), &v8_bind_to)) {
On 2015/06/23 17:26:23, kalman wrote:
> Perhaps add a ToV8String() variation that takes a std::string?

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:35:
->ToObject(v8_context)
On 2015/06/23 17:26:22, kalman wrote:
> 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)?

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?

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:44:
!binding_value->ToObject(v8_context).ToLocal(&binding)) {
On 2015/06/23 17:26:23, kalman wrote:
> Is |binding_value| guaranteed to be an Object? What happens if it isn't?
> Likewise below with |binding_instance_value|.

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.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:49:
v8::Local<v8::Value> create_binding_value;
On 2015/06/23 17:26:22, kalman wrote:
> // require('Binding').Binding.create;

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:58:
v8::Local<v8::Value> argv[] = {v8_api_name};
On 2015/06/23 17:26:22, kalman wrote:
> // require('Binding').Binding.create(api_name);

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:69:
v8::Local<v8::Value> generate_value;
On 2015/06/23 17:26:23, kalman wrote:
> // require('Binding').Binding.create(api_name).generate;

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:77:
v8::Local<v8::Object> object = v8::Object::New(isolate);
On 2015/06/23 17:26:23, kalman wrote:
> // require('Binding').Binding.create(api_name).generate();

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:85: if
(!SetProperty(v8_context, object, v8_bind_to, compiled_schema)) {
On 2015/06/23 17:26:23, kalman wrote:
> // var result = {};
> // result[bind_to] = ...;

Done.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:89: return
scope.Escape(object);
On 2015/06/23 17:26:22, kalman wrote:
> // var result = {};
> // result[bind_to] = ...;
> // return result;

Done.

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

lgtm assuming the change is easy to make.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:35:
->ToObject(v8_context)
On 2015/06/24 00:04:32, bashi1 wrote:
> On 2015/06/23 17:26:22, kalman wrote:
> > 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)?
> 
> 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?

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.

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:44:
!binding_value->ToObject(v8_context).ToLocal(&binding)) {
On 2015/06/24 00:04:32, bashi1 wrote:
> On 2015/06/23 17:26:23, kalman wrote:
> > Is |binding_value| guaranteed to be an Object? What happens if it isn't?
> > Likewise below with |binding_instance_value|.
> 
> 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.

ok, cool.

https://codereview.chromium.org/1192763002/diff/60001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/60001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:46: //
require('binding');
I would have preferred for each of these comments at the start of the blocks
(i.e. before the variable declarations that relate to them), not after. Easier
to scan through to see what is happening.

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

Thank you for reviewing. I'd like you to have the last look (sorry!)

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/40001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:35:
->ToObject(v8_context)
On 2015/06/24 00:34:56, kalman wrote:
> On 2015/06/24 00:04:32, bashi1 wrote:
> > On 2015/06/23 17:26:22, kalman wrote:
> > > 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)?
> > 
> > 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?
> 
> 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.

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

https://codereview.chromium.org/1192763002/diff/60001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/60001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:46: //
require('binding');
On 2015/06/24 00:34:56, kalman wrote:
> I would have preferred for each of these comments at the start of the blocks
> (i.e. before the variable declarations that relate to them), not after. Easier
> to scan through to see what is happening.

Done.

[not at google - send to devlin, 2015-06-24 16:43:03]

lgtm

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:61: //
require('Binding').Binding.create;
For all, this should be require('binding') not require('Binding') (note
capitalisation).

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:92: //
require('Binding').Binding.create(api_name).generate();
Heh, stepping back here, one day I might come back to this and just add a
'generateForNativeHandler' method to the 'Binding' module, it would be so much
simpler. require('binding').generateForNativeHandler(api_name). One day!

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/mod...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:205: v8_module_name);
This should fit on 1 line; run "git cl format" before submitting.

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:563: return v8::MaybeLocal<v8::Object>();
This should probably be a DCHECK since the code is only hit during testing. No
need to fail gracefully.

[bashi, 2015-06-26 00:32:36]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-06-26 00:32:37]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1192763002/#ps100001
(title: " ")

[bashi, 2015-06-26 00:32:46]

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/bin...
File extensions/renderer/binding_generating_native_handler.cc (right):

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:61: //
require('Binding').Binding.create;
On 2015/06/24 16:43:03, kalman wrote:
> For all, this should be require('binding') not require('Binding') (note
> capitalisation).

Done.

https://codereview.chromium.org/1192763002/diff/80001/extensions/renderer/bin...
extensions/renderer/binding_generating_native_handler.cc:92: //
require('Binding').Binding.create(api_name).generate();
On 2015/06/24 16:43:03, kalman wrote:
> Heh, stepping back here, one day I might come back to this and just add a
> 'generateForNativeHandler' method to the 'Binding' module, it would be so much
> simpler. require('binding').generateForNativeHandler(api_name). One day!

:)

[commit-bot: I haz the power, 2015-06-26 00:34:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192763002/100001

[commit-bot: I haz the power, 2015-06-26 00:40:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-26 00:41:10]

Patchset 6 (id:??) landed as
https://crrev.com/61ca3c7b545104ac3edda633a80a62808d281b23
Cr-Commit-Position: refs/heads/master@{#336302}