Refactor Worklet class to use ScriptResource

third_party/WebKit/Source/modules/worklet/Worklet.cpp

Index: third_party/WebKit/Source/modules/worklet/Worklet.cpp
diff --git a/third_party/WebKit/Source/modules/worklet/Worklet.cpp b/third_party/WebKit/Source/modules/worklet/Worklet.cpp
index 54993701b26d3318a46beafde5caadf43c95419a..dcd238f63df2984197f13907a3d6b3e3b2184aa7 100644
--- a/third_party/WebKit/Source/modules/worklet/Worklet.cpp
+++ b/third_party/WebKit/Source/modules/worklet/Worklet.cpp
@@ -5,17 +5,22 @@
 #include "modules/worklet/Worklet.h"
 
 #include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "bindings/core/v8/ScriptSourceCode.h"
 #include "bindings/core/v8/V8Binding.h"
-#include "bindings/core/v8/WorkerOrWorkletScriptController.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/ExceptionCode.h"
-#include "core/inspector/InspectorInstrumentation.h"
+#include "core/fetch/FetchInitiatorTypeNames.h"
+#include "core/frame/LocalFrame.h"
+#include "core/loader/DocumentLoader.h"
+#include "core/loader/FrameFetchContext.h"
 #include "core/workers/WorkletGlobalScopeProxy.h"
+#include "modules/worklet/WorkletScriptLoader.h"
 
 namespace blink {
 
-Worklet::Worklet(ExecutionContext* executionContext)
+Worklet::Worklet(ExecutionContext* executionContext, LocalFrame* frame)

[yhirano, 2016/07/29 02:19:23]

Can you tell me the relationship between |frame| and |executionContext|? Does
|frame->document()| always equal to |executionContext|? If so, can we omit the
|executionContext| parameter? Otherwise, when will they be different each other?

[haraken, 2016/07/29 08:26:35]

It should be. If the two are different, you're doing a wrong thing :)

So you should be able to remove either of the ExecutionContext or LocalFrame.

[Gleb Lanbin, 2016/07/29 19:23:14]

Done.

     : ActiveDOMObject(executionContext)
+    , m_fetcher(frame->loader().documentLoader()->fetcher())
 {
 }
 
@@ -26,67 +31,45 @@ ScriptPromise Worklet::import(ScriptState* scriptState, const String& url)
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(SyntaxError, "'" + url + "' is not a valid URL."));
     }
 
-    // TODO(ikilpatrick): Perform upfront CSP checks once we decide on a
-    // CSP-policy for worklets.
+    ResourceRequest resourceRequest(scriptURL);
+    resourceRequest.setRequestContext(WebURLRequest::RequestContextScript);
+    FetchRequest request(resourceRequest, FetchInitiatorTypeNames::internal);
+    ScriptResource* resource = ScriptResource::fetch(request, fetcher());
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
-    m_resolvers.append(resolver);
-
     ScriptPromise promise = resolver->promise();
-
-    // TODO(ikilpatrick): WorkerScriptLoader will need to be extended to allow
-    // module loading support. For now just fetch a 'classic' script.
-
-    // NOTE: WorkerScriptLoader may synchronously invoke its callbacks
-    // (resolving the promise) before we return it.
-    m_scriptLoaders.append(WorkerScriptLoader::create());
-    m_scriptLoaders.last()->loadAsynchronously(*getExecutionContext(), scriptURL, DenyCrossOriginRequests,
-        getExecutionContext()->securityContext().addressSpace(),
-        bind(&Worklet::onResponse, wrapPersistent(this), WTF::unretained(m_scriptLoaders.last().get())),
-        bind(&Worklet::onFinished, wrapPersistent(this), WTF::unretained(m_scriptLoaders.last().get()), wrapPersistent(resolver)));
-
+    if (resource) {
+        m_resources.append(resource);
+        WorkletScriptLoader* workletLoader = WorkletScriptLoader::create(resolver, this);
+        resource->addClient(workletLoader);

[yhirano, 2016/07/29 02:19:23]

resource -> workerLoader is a weak reference (actually, held as a raw pointer).
I think |workerLoader| is held by no one, right?

[yhirano, 2016/07/29 02:19:23]

You need to remove the client manually from the resource before the client gets
invalid. Inheriting from ResourceOwner is the simplest way to address it, I
guess.

[Gleb Lanbin, 2016/07/29 19:23:14]

Done. Thanks!

[Gleb Lanbin, 2016/07/29 19:23:14]

Done.

+    } else {
+        resolver->reject(DOMException::create(NetworkError));
+    }
     return promise;
 }
 
-void Worklet::onResponse(WorkerScriptLoader* scriptLoader)
-{
-    InspectorInstrumentation::didReceiveScriptResponse(getExecutionContext(), scriptLoader->identifier());
-}
-
-void Worklet::onFinished(WorkerScriptLoader* scriptLoader, ScriptPromiseResolver* resolver)
+void Worklet::notifyFinished(Resource* resource)
 {
-    if (scriptLoader->failed()) {
-        resolver->reject(DOMException::create(NetworkError));
-    } else {
-        // TODO(ikilpatrick): Worklets don't have the same error behaviour
-        // as workers, etc. For a SyntaxError we should reject, however if
-        // the script throws a normal error, resolve. For now just resolve.
-        workletGlobalScopeProxy()->evaluateScript(scriptLoader->script(), scriptLoader->url());
-        InspectorInstrumentation::scriptImported(getExecutionContext(), scriptLoader->identifier(), scriptLoader->script());
-        resolver->resolve();
-    }
-
-    size_t index = m_scriptLoaders.find(scriptLoader);
+    workletGlobalScopeProxy()->evaluateScript(ScriptSourceCode(toScriptResource(resource)));
 
-    ASSERT(index != kNotFound);
-    ASSERT(m_resolvers[index] == resolver);
-
-    m_scriptLoaders.remove(index);
-    m_resolvers.remove(index);
+    size_t index = m_resources.find(resource);
+    DCHECK(index != kNotFound);

[yhirano, 2016/07/29 02:19:23]

DCHECK_NE

[Gleb Lanbin, 2016/07/29 19:23:14]

Done.

+    DCHECK(m_resources[index] == resource);

[yhirano, 2016/07/29 02:19:23]

DCHECK_EQ

[Gleb Lanbin, 2016/07/29 19:23:14]

Done.

+    m_resources.remove(index);
 }
 
 void Worklet::stop()
 {
     workletGlobalScopeProxy()->terminateWorkletGlobalScope();
-
-    for (auto scriptLoader : m_scriptLoaders) {
-        scriptLoader->cancel();
+    for (const auto& resource : m_resources) {
+        resource->loader()->cancel();
     }
 }
 
 DEFINE_TRACE(Worklet)
 {
-    visitor->trace(m_resolvers);
+    visitor->trace(m_resources);
+    visitor->trace(m_fetcher);
     ActiveDOMObject::trace(visitor);
 }