[Blink] Modify SuspendableScriptExecutor to take a v8::Function

third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp

Index: third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
diff --git a/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp b/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
index 9a3dabcbff7b81727e33907ca7147941bd8686e8..43ab0d7dd2c0423ed06f39ac39e2389f28f7937f 100644
--- a/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
+++ b/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
@@ -6,6 +6,7 @@
 
 #include "bindings/core/v8/ScriptController.h"
 #include "bindings/core/v8/ScriptSourceCode.h"
+#include "bindings/core/v8/V8PersistentValueVector.h"
 #include "core/dom/Document.h"
 #include "core/frame/LocalFrame.h"
 #include "platform/UserGestureIndicator.h"
@@ -16,28 +17,116 @@
 
 namespace blink {
 
+namespace {
+
+class WebScriptExecutor : public GarbageCollectedFinalized<WebScriptExecutor>, public SuspendableScriptExecutor::Executor {

[haraken, 2016/10/03 02:30:09]

WebScriptExecutor should not inherit from GarbageCollectedFinalized, because
SuspendableScriptExecutor::Executor already inherits from GarbageCollected.

Just make SuspendableScriptExecutor::Executor inherit from
GarbageCollectedFinalized. And make WebScriptExecutor not inherit from
GarbageCollectedFinalized.

[Devlin, 2016/10/03 20:53:30]

Done.

+public:
+    WebScriptExecutor(const HeapVector<ScriptSourceCode>& sources, int worldID, int extensionGroup, bool userGesture);
+
+    Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+        visitor->trace(m_sources);
+        SuspendableScriptExecutor::Executor::trace(visitor);
+    }
+
+private:
+    HeapVector<ScriptSourceCode> m_sources;
+    int m_worldID;
+    int m_extensionGroup;
+    bool m_userGesture;
+};
+
+WebScriptExecutor::WebScriptExecutor(const HeapVector<ScriptSourceCode>& sources, int worldID, int extensionGroup, bool userGesture)
+    : m_sources(sources)
+    , m_worldID(worldID)
+    , m_extensionGroup(extensionGroup)
+    , m_userGesture(userGesture)
+{
+}
+
+Vector<v8::Local<v8::Value>> WebScriptExecutor::execute(LocalFrame* frame)
+{
+    std::unique_ptr<UserGestureIndicator> indicator;
+    if (m_userGesture)
+        indicator = wrapUnique(new UserGestureIndicator(DefinitelyProcessingNewUserGesture));
+
+    Vector<v8::Local<v8::Value>> results;
+    if (m_worldID) {
+        frame->script().executeScriptInIsolatedWorld(m_worldID, m_sources, m_extensionGroup, &results);
+    } else {
+        v8::Local<v8::Value> scriptValue = frame->script().executeScriptInMainWorldAndReturnValue(m_sources.first());
+        results.append(scriptValue);
+    }
+
+    return results;
+}
+
+class V8FunctionExecutor : public GarbageCollectedFinalized<V8FunctionExecutor>, public SuspendableScriptExecutor::Executor {

[haraken, 2016/10/03 02:30:09]

Ditto.

[Devlin, 2016/10/03 20:53:30]

Done.

+public:
+    V8FunctionExecutor(v8::Isolate*, v8::Local<v8::Function>, v8::Local<v8::Value> receiver, int argc, v8::Local<v8::Value> argv[]);
+
+    Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+
+private:
+    ScopedPersistent<v8::Function> m_function;
+    ScopedPersistent<v8::Value> m_receiver;
+    V8PersistentValueVector<v8::Value> m_args;
+};
+
+V8FunctionExecutor::V8FunctionExecutor(v8::Isolate* isolate, v8::Local<v8::Function> function, v8::Local<v8::Value> receiver, int argc, v8::Local<v8::Value> argv[])
+    : m_function(isolate, function)
+    , m_receiver(isolate, receiver)
+    , m_args(isolate)
+{
+    m_args.ReserveCapacity(argc);
+    for (int i = 0; i < argc; ++i)
+        m_args.Append(argv[i]);
+}
+
+Vector<v8::Local<v8::Value>> V8FunctionExecutor::execute(LocalFrame* frame)
+{
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    Vector<v8::Local<v8::Value>> results;
+    v8::Local<v8::Value> singleResult;
+    size_t numArgs = m_args.Size();
+    v8::Local<v8::Value> args[numArgs];
+    for (size_t i = 0; i < numArgs; ++i)
+        args[i] = m_args.Get(i);
+    if (V8ScriptRunner::callFunction(m_function.newLocal(isolate), frame->document(), m_receiver.newLocal(isolate), numArgs, static_cast<v8::Local<v8::Value>*>(args), toIsolate(frame)).ToLocal(&singleResult))
+        results.append(singleResult);
+    return results;
+}
+
+} // namespace
+
 void SuspendableScriptExecutor::createAndRun(LocalFrame* frame, int worldID, const HeapVector<ScriptSourceCode>& sources, int extensionGroup, bool userGesture, WebScriptExecutionCallback* callback)
 {
-    SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, worldID, sources, extensionGroup, userGesture, callback);
+    SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new WebScriptExecutor(sources, worldID, extensionGroup, userGesture));
+    executor->run();
+}
+
+void SuspendableScriptExecutor::createAndRun(LocalFrame* frame, v8::Isolate* isolate, v8::Local<v8::Function> function, v8::Local<v8::Value> receiver, int argc, v8::Local<v8::Value> argv[], WebScriptExecutionCallback* callback)
+{
+    SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, new V8FunctionExecutor(isolate, function, receiver, argc, argv));

[Devlin, 2016/09/30 21:22:22]

Compiler error here (and on line 106): "error: member 'operator new' found in
multiple base classes of different types" - what should we be doing for garbage
collected on V8FunctionExecutor/WebScriptExecutor?

[haraken, 2016/10/03 02:30:09]

BTW, is SuspendableScriptExecutor used only on stack? Then it would be easier to
make it as STACK_ALLOCATED (instead of making it GarbageCollected).

[haraken, 2016/10/03 02:30:09]

See my comment in the above.

[Devlin, 2016/10/03 20:53:30]

SuspendableScriptExecutor is only used on the heap, unless I'm missing
something.  We create it here and it can finish either synchronously (in which
case it's freed right away) or asynchronously (in which case it's freed once it
executes).

[haraken, 2016/10/04 00:08:30]

My question is if you can write the code like:

  SuspendableScriptExecutor executor(...);
  executor->run();

Then SuspendableScriptExecutor is stack-allocated. You can mark it as
STACK_ALLOCATED and don't need to make it GarbageCollected.

[Devlin, 2016/10/04 00:14:26]

The SuspendableScriptExecutor needs to be able to live outside the current
scope, since the scripts may finish asynchronously (which is the reason to use
SuspendableScriptExecutor instead of just directly executing the script
immediately).  As far as I know, there's no way we can do that with a
stack-allocated object.

     executor->run();
 }
 
 void SuspendableScriptExecutor::contextDestroyed()
 {
     SuspendableTimer::contextDestroyed();
-    m_callback->completed(Vector<v8::Local<v8::Value>>());
+    if (m_callback)
+        m_callback->completed(Vector<v8::Local<v8::Value>>());
     dispose();
 }
 
-SuspendableScriptExecutor::SuspendableScriptExecutor(LocalFrame* frame, int worldID, const HeapVector<ScriptSourceCode>& sources, int extensionGroup, bool userGesture, WebScriptExecutionCallback* callback)
+SuspendableScriptExecutor::SuspendableScriptExecutor(LocalFrame* frame, WebScriptExecutionCallback* callback, Executor* executor)
     : SuspendableTimer(frame->document())
     , m_frame(frame)
-    , m_sources(sources)
     , m_callback(callback)
     , m_keepAlive(this)
-    , m_worldID(worldID)
-    , m_extensionGroup(extensionGroup)
-    , m_userGesture(userGesture)
+    , m_executor(executor)
 {
 }
 
@@ -65,27 +154,16 @@ void SuspendableScriptExecutor::run()
 
 void SuspendableScriptExecutor::executeAndDestroySelf()
 {
-    // after calling the destructor of object - object will be unsubscribed from
-    // resumed and contextDestroyed LifecycleObserver methods
-    std::unique_ptr<UserGestureIndicator> indicator;
-    if (m_userGesture)
-        indicator = wrapUnique(new UserGestureIndicator(DefinitelyProcessingNewUserGesture));
-
     v8::HandleScope scope(v8::Isolate::GetCurrent());
-    Vector<v8::Local<v8::Value>> results;
-    if (m_worldID) {
-        m_frame->script().executeScriptInIsolatedWorld(m_worldID, m_sources, m_extensionGroup, &results);
-    } else {
-        v8::Local<v8::Value> scriptValue = m_frame->script().executeScriptInMainWorldAndReturnValue(m_sources.first());
-        results.append(scriptValue);
-    }
+    Vector<v8::Local<v8::Value>> results = m_executor->execute(m_frame);
 
     // The script may have removed the frame, in which case contextDestroyed()
     // will have handled the disposal/callback.
     if (!m_frame->client())
         return;
 
-    m_callback->completed(results);
+    if (m_callback)
+        m_callback->completed(results);
     dispose();
 }
 
@@ -100,7 +178,6 @@ void SuspendableScriptExecutor::dispose()
 DEFINE_TRACE(SuspendableScriptExecutor)
 {
     visitor->trace(m_frame);
-    visitor->trace(m_sources);
     SuspendableTimer::trace(visitor);
 }