Enable module scripts in ScriptLoader

third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp

Index: third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
index fda3bcb4b64e543789bca503174aa168bc90f3d9..894917f010dc48896fc4b0bcceecd3aa2275f00f 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
@@ -6,6 +6,7 @@
 
 #include "bindings/core/v8/V8Binding.h"
 #include "core/dom/Modulator.h"
+#include "core/dom/ModuleScript.h"
 #include "core/dom/ScriptModuleResolver.h"
 
 namespace blink {
@@ -55,13 +56,35 @@ ScriptValue ScriptModule::Instantiate(ScriptState* script_state) {
   return ScriptValue();
 }
 
-void ScriptModule::Evaluate(ScriptState* script_state) {
+// https://html.spec.whatwg.org/#run-a-module-script
+void ScriptModule::Evaluate(ScriptState* script_state,
+                            const ModuleScript* moduleScript) {
   v8::Isolate* isolate = script_state->GetIsolate();
   v8::TryCatch try_catch(isolate);
   try_catch.SetVerbose(true);
   v8::Local<v8::Value> result;
+
+  // 3. "If s's instantiation state is "errored", then report the exception

[hiroshige, 2017/04/12 19:57:05]

Where should this logic be placed?
(Note that this logic is not tested and thus significant changes might be done
later, but I'd like to start the discussion of the place of this logic)

I temporarily put this logic here, because
- this step require |isolate|, and I uses |isolate| at Line 62 here.
- In the classic script case, ClassicScript::RunScript() is implemented totally
in bindings/core/v8, so having this logic here in bindings/core/v8 might be
consistent.

An alternative is to place this in ModuleScript, as we already get ScriptState
there via ToScriptStateForMainWorld(frame), and we can get isolate.

But this makes me think about more general question:
Which script state and isolate should we use in module scripts?
I expect it is Modulator.
So I expect ModuleScript should have a direct reference to Modulator, removing
the following comments in ModuleScript.h:
// Note: A "module script"'s "setttings object" is ommitted, as we currently
// always have access to the corresponding Modulator when operating on a
// ModuleScript instance.

(also related to https://github.com/whatwg/html/issues/2469)

[kouhei (in TOK), 2017/04/13 02:28:36]

I'd put this logic in ModulatorImpl
This path is also fine for me. I don't have a strong opinion here and trust your
judgement :)

[hiroshige, 2017/04/13 23:53:15]

Splitted the ModulatorImpl change into
https://codereview.chromium.org/2819733002, and making ModuleScript to have
Modulator reference in https://codereview.chromium.org/2823483002.

+  //     given by s's instantiation error for s and abort these steps."
+  if (moduleScript->InstantiationState() ==
+      ModuleInstantiationState::kErrored) {
+    V8ThrowException::ThrowException(
+        isolate, moduleScript->CreateInstantiationError(isolate));
+    return;
+  }
+
+  // 4. "Assert: s's instantiation state is "instantiated" (and thus its
+  //     module record is not null)."
+  DCHECK_EQ(moduleScript->InstantiationState(),
+            ModuleInstantiationState::kInstantiated);
+
+  // 5. "Let record be s's module record."
+  const ScriptModule& record = moduleScript->Record();
+  DCHECK(!record.IsNull());
+
+  // 6.--9.?
   if (!V8Call(
-          V8ScriptRunner::EvaluateModule(module_->NewLocal(isolate),
+          V8ScriptRunner::EvaluateModule(record.module_->NewLocal(isolate),
                                          script_state->GetContext(), isolate),
           result, try_catch)) {
     // TODO(adamk): report error