bindings: Use Maybe version of Call()

bindings: Use Maybe version of Call()

This CL also makes following functions return MaybeLocal
as they return results of Call().
- V8ScriptRunner::callFunction()
- ScriptController::callFunction()

BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194181

[bashi, 2015-04-17 07:39:24]

Patchset #1 (id:1) has been deleted

[bashi, 2015-04-17 09:21:52]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[bashi, 2015-04-17 09:21:53]

PTAL?

[Yuki, 2015-04-17 09:59:24]

lgtm

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ErrorHandler.cpp:60: v8::Local<v8::Value> returnValue;
Move this line just before you really need returnValue variable.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp:71:
v8::Local<v8::Value>
V8WorkerGlobalScopeEventListener::callListenerFunction(ScriptState* scriptState,
v8::Local<v8::Value> jsEvent, Event* event)
Is it guaranteed that this function returns an empty handle iff an exception is
thrown?

Even in that case, I vaguely think this function should return MaybeLocal...

It would be helpful to have a comment in .h file about how to call this function
and how to check and handle errors.

We might want a TODO comment like WebLocalFrameImpl.

[haraken, 2015-04-18 23:10:47]

LGTM

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/NPV8Object.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/NPV8Object.cpp:317: v8::Local<v8::Value> resultObject;

Move this down to line 327.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ErrorHandler.cpp:78: if
(scriptState->executionContext()->isWorkerGlobalScope())

Nit: I'd add { } since the clause is long.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:496: v8::MaybeLocal<v8::Value> result
= function->Call(receiver, argc, args);

I'm a bit confused about this. Why don't we need to pass
isolate->GetCurrentContext() here? A MaybeLocal version of the V8 API must take
a context, mustn't it?

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/modules...
File Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp
(right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/modules...
Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp:69:
v8::TryCatch exceptionCatcher;

Is this v8::TryCatch still needed?

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:908: if
(frame()->script().callFunction(function, receiver, argc,
static_cast<v8::Local<v8::Value>*>(argv)).ToLocal(&result))

Nit: I'd prefer using the following programming pattern for consistency.

if (!...)
  return ...;  // Fail
return ...;  // Success

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:908: if
(frame()->script().callFunction(function, receiver, argc,
static_cast<v8::Local<v8::Value>*>(argv)).ToLocal(&result))

Ditto.

[bashi, 2015-04-21 00:10:42]

Patchset #2 (id:40001) has been deleted

[bashi, 2015-04-21 00:15:40]

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/NPV8Object.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/NPV8Object.cpp:317: v8::Local<v8::Value> resultObject;
On 2015/04/18 23:10:46, haraken wrote:
> 
> Move this down to line 327.

Done.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ErrorHandler.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ErrorHandler.cpp:60: v8::Local<v8::Value> returnValue;
On 2015/04/17 09:59:23, Yuki wrote:
> Move this line just before you really need returnValue variable.

Done.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ErrorHandler.cpp:78: if
(scriptState->executionContext()->isWorkerGlobalScope())
On 2015/04/18 23:10:47, haraken wrote:
> 
> Nit: I'd add { } since the clause is long.
> 

Done.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:496: v8::MaybeLocal<v8::Value> result
= function->Call(receiver, argc, args);
On 2015/04/18 23:10:47, haraken wrote:
> 
> I'm a bit confused about this. Why don't we need to pass
> isolate->GetCurrentContext() here? A MaybeLocal version of the V8 API must
take
> a context, mustn't it?
> 

Thanks for the catch! I forgot to pass isolate->GetCurrentContext().

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp:71:
v8::Local<v8::Value>
V8WorkerGlobalScopeEventListener::callListenerFunction(ScriptState* scriptState,
v8::Local<v8::Value> jsEvent, Event* event)
On 2015/04/17 09:59:24, Yuki wrote:
> Is it guaranteed that this function returns an empty handle iff an exception
is
> thrown?
> 
> Even in that case, I vaguely think this function should return MaybeLocal...
> 
> It would be helpful to have a comment in .h file about how to call this
function
> and how to check and handle errors.
> 
> We might want a TODO comment like WebLocalFrameImpl.

Chatted offline. It's not easy to make |callListenerFunction| return MaybeLocal
because it can fail without exception and I can't come up with an appropriate
exception to throw on failure. Added a comment on
V8AbstractEventListener::callListenerFunction to describe this could return an
empty handle.

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/modules...
File Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp
(right):

https://codereview.chromium.org/1061503005/diff/20001/Source/bindings/modules...
Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp:69:
v8::TryCatch exceptionCatcher;
On 2015/04/18 23:10:47, haraken wrote:
> 
> Is this v8::TryCatch still needed?

I guess so. IIUC, it swallows an exception.

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:908: if
(frame()->script().callFunction(function, receiver, argc,
static_cast<v8::Local<v8::Value>*>(argv)).ToLocal(&result))
On 2015/04/18 23:10:47, haraken wrote:
> 
> Nit: I'd prefer using the following programming pattern for consistency.
> 
> if (!...)
>   return ...;  // Fail
> return ...;  // Success

Done.

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:908: if
(frame()->script().callFunction(function, receiver, argc,
static_cast<v8::Local<v8::Value>*>(argv)).ToLocal(&result))
On 2015/04/18 23:10:47, haraken wrote:
> 
> Ditto.

Sorry, I couldn't find what you want to say here. Putting braces?

[haraken, 2015-04-21 04:38:54]

Still LGTM

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1061503005/diff/20001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:908: if
(frame()->script().callFunction(function, receiver, argc,
static_cast<v8::Local<v8::Value>*>(argv)).ToLocal(&result))
On 2015/04/21 00:15:40, bashi1 wrote:
> On 2015/04/18 23:10:47, haraken wrote:
> > 
> > Ditto.
> 
> Sorry, I couldn't find what you want to say here. Putting braces?

Ignore the comment :)

[bashi, 2015-04-21 23:30:48]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-04-21 23:30:48]

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

[commit-bot: I haz the power, 2015-04-21 23:31:22]

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

[commit-bot: I haz the power, 2015-04-21 23:35:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-21 23:35:46]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52925)

[bashi, 2015-04-22 00:54:35]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-04-22 00:54:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1061503005/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-04-22 00:54:58]

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

[commit-bot: I haz the power, 2015-04-22 01:00:03]

Committed patchset #3 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194181