binidngs: Make callInternalFunction return MaybeLocal

Source/core/inspector/JavaScriptCallFrame.cpp

Index: Source/core/inspector/JavaScriptCallFrame.cpp
diff --git a/Source/core/inspector/JavaScriptCallFrame.cpp b/Source/core/inspector/JavaScriptCallFrame.cpp
index 5369ca566f83786ef2ad1a3894bc225247aad332..052b1880e6bb27f10e6c86c31de7166833d86d1e 100644
--- a/Source/core/inspector/JavaScriptCallFrame.cpp
+++ b/Source/core/inspector/JavaScriptCallFrame.cpp
@@ -69,10 +69,10 @@ int JavaScriptCallFrame::callV8FunctionReturnInt(const char* name) const
     v8::Context::Scope contextScope(m_debuggerContext.newLocal(m_isolate));
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, name)));
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate);
-    if (result.IsEmpty() || !result->IsInt32())
+    v8::Local<v8::Value> result;
+    if (!V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocal(&result) || !result->IsInt32())
         return 0;
-    return result->Int32Value();
+    return result.As<v8::Int32>()->Value();
 }
 
 String JavaScriptCallFrame::callV8FunctionReturnString(const char* name) const
@@ -81,7 +81,7 @@ String JavaScriptCallFrame::callV8FunctionReturnString(const char* name) const
     v8::Context::Scope contextScope(m_debuggerContext.newLocal(m_isolate));
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, name)));
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate);
+    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked();

[haraken, 2015/03/25 12:05:22]

Ditto. I guess this callInternalFunction can return an empty handle.

[bashi, 2015/03/27 00:39:50]

Yes. If we have better option than crash (e.g. return an empty string), it would
be great. yurys@, WDYT?

[yurys, 2015/03/27 12:07:24]

@aanrey would know better if his place is allowed to throw when called e.g. from
JavaScriptCallFrame::stepInPositions

[bashi, 2015/03/31 05:46:08]

Changed to return String() since this function seems to have the same semantics
of callV8FunctionReturnInt, which returns 0 on failure.

     return toCoreStringWithUndefinedOrNullCheck(result);
 }
 
@@ -124,7 +124,7 @@ v8::Local<v8::Value> JavaScriptCallFrame::scopeChain() const
 {
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "scopeChain")));
-    v8::Local<v8::Array> scopeChain = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate));
+    v8::Local<v8::Array> scopeChain = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked());
     v8::Local<v8::Array> result = v8::Array::New(m_isolate, scopeChain->Length());
     for (uint32_t i = 0; i < scopeChain->Length(); i++)
         result->Set(i, scopeChain->Get(i));
@@ -135,7 +135,7 @@ int JavaScriptCallFrame::scopeType(int scopeIndex) const
 {
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "scopeType")));
-    v8::Local<v8::Array> scopeType = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate));
+    v8::Local<v8::Array> scopeType = v8::Local<v8::Array>::Cast(V8ScriptRunner::callInternalFunction(func, callFrame, 0, 0, m_isolate).ToLocalChecked());
     return scopeType->Get(scopeIndex)->Int32Value();
 }
 
@@ -174,15 +174,14 @@ ScriptValue JavaScriptCallFrame::evaluateWithExceptionDetails(ScriptState* scrip
         scopeExtension.isEmpty() ? v8::Handle<v8::Value>::Cast(v8::Undefined(m_isolate)) : scopeExtension.v8Value()
     };
     v8::TryCatch tryCatch;
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(evalFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate);
-
     v8::Local<v8::Object> wrappedResult = v8::Object::New(m_isolate);
-    if (tryCatch.HasCaught()) {
-        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), tryCatch.Exception());
-        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "exceptionDetails"), createExceptionDetails(m_isolate, tryCatch.Message()));
-    } else {
+    v8::Local<v8::Value> result;
+    if (V8ScriptRunner::callInternalFunction(evalFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocal(&result)) {
         wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), result);

[haraken, 2015/03/25 12:05:22]

Can we replace the v8::String::NewFromUtf8 with v8AtomicString in follow-up CLs?

Just to confirm: Our plan is to replace v8::String::NewFromUtf8 (except the ones
in devtools) with v8AtomicString, right?

[yurys, 2015/03/25 12:29:25]

This is exactly one of the places in devtools code that is going to be moved
along with the rest of debugger instrumentation so I'd rather you replaced this
calls with ToLocalChecked or something like that. BTW, would it make sense to
have something like ToLocalOrCrash?

[Yuki, 2015/03/25 12:34:01]

ToLocalChecked crashes when V8_ENABLE_CHECKS is defined at compile time.

Or, do you want ToLocalOrCrash to crash regardless of V8_ENABLE_CHECKS?

[bashi, 2015/03/27 00:39:50]

Let's use ToLocalChecked() as this is going to be moved out.

[yurys, 2015/03/27 12:07:24]

Current implementation with ToLocal call here looks fine to me, it is equivalent
to what it did before.

         wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "exceptionDetails"), v8::Undefined(m_isolate));
+    } else {
+        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "result"), tryCatch.Exception());
+        wrappedResult->Set(v8::String::NewFromUtf8(m_isolate, "exceptionDetails"), createExceptionDetails(m_isolate, tryCatch.Message()));
     }
     return ScriptValue(scriptState, wrappedResult);
 }
@@ -192,7 +191,7 @@ v8::Local<v8::Value> JavaScriptCallFrame::restart()
     v8::Local<v8::Object> callFrame = m_callFrame.newLocal(m_isolate);
     v8::Local<v8::Function> restartFunction = v8::Local<v8::Function>::Cast(callFrame->Get(v8AtomicString(m_isolate, "restart")));
     v8::Debug::SetLiveEditEnabled(m_isolate, true);
-    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(restartFunction, callFrame, 0, 0, m_isolate);
+    v8::Local<v8::Value> result = V8ScriptRunner::callInternalFunction(restartFunction, callFrame, 0, 0, m_isolate).ToLocalChecked();
     v8::Debug::SetLiveEditEnabled(m_isolate, false);
     return result;
 }
@@ -207,7 +206,7 @@ ScriptValue JavaScriptCallFrame::setVariableValue(ScriptState* scriptState, int
         v8String(m_isolate, variableName),
         newValue.v8Value()
     };
-    return ScriptValue(scriptState, V8ScriptRunner::callInternalFunction(setVariableValueFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate));
+    return ScriptValue(scriptState, V8ScriptRunner::callInternalFunction(setVariableValueFunction, callFrame, WTF_ARRAY_LENGTH(argv), argv, m_isolate).ToLocalChecked());

[yurys, 2015/03/27 12:07:24]

In this case we may well fail to change local variable and the exception should
be propagated to the calling code (see 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
So empty handle is valid here.

FYI: Using ScriptValue in this class (as well as in many other places in
inspector code) just hampers reading this code. When it was introduced we had to
work both with V8 and JSC and it made perfect sense then. In Blink we use single
VM and there are no plans for adding another so using Handles directly would be
preferable. I'll try to update this code as it will be necessary for extracting
it anyways.

[bashi, 2015/03/31 05:46:08]

Thanks. Replaced with ToLocal(). In blink, we decided to propagate MaybeLocal
when a method returns a result of v8 Maybe API directly (so
V8ScriptRunner::callInternalFunction returns Maybe). It would make sense to have
this method return MaybeLocal as well, but I didn't do that as inspector code
would have a different policy.

 }
 
 v8::Local<v8::Object> JavaScriptCallFrame::createExceptionDetails(v8::Isolate* isolate, v8::Local<v8::Message> message)