Skip to content

Commit

Permalink
Add v8::Local<v8::Module> argument to Instantiate
Browse files Browse the repository at this point in the history 
Made Instantiate, ModuleRequests and ModuleRequestPositions static
 to remove ModuleRecord.
And also, removed a HashMap from module_tree_linker_test
to remove ModuleRecord.

Bug: 991863
Change-Id: I15d09c7f2d17e3fc35b944f0bc421706b68c1568
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1743245
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Rika Fujimaki <rikaf@google.com>
Cr-Commit-Position: refs/heads/master@{#686309}
  • Loading branch information
@rika-fujimaki
rika-fujimaki authored and Commit Bot committed Aug 13, 2019
1 parent 5572b09 commit 5a8a3c3
Show file tree
Hide file tree
Showing 25 changed files with 230 additions and 160 deletions.
36 changes: 17 additions & 19 deletions third_party/blink/renderer/bindings/core/v8/module_record.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ ModuleRecord::ModuleRecord(v8::Isolate* isolate,

ModuleRecord::~ModuleRecord() = default;

ModuleRecord ModuleRecord::Compile(
v8::Local<v8::Module> ModuleRecord::Compile(
v8::Isolate* isolate,
const String& source,
const KURL& source_url,
Expand Down Expand Up @@ -90,7 +90,7 @@ ModuleRecord ModuleRecord::Compile(
.ToLocal(&module)) {
DCHECK(try_catch.HasCaught());
exception_state.RethrowV8Exception(try_catch.Exception());
return ModuleRecord();
return v8::Local<v8::Module>();
}
DCHECK(!try_catch.HasCaught());

Expand All @@ -100,23 +100,22 @@ ModuleRecord ModuleRecord::Compile(
isolate, cache_handler, produce_cache_options, module);
}

return ModuleRecord(isolate, module, source_url);
return module;
}

ScriptValue ModuleRecord::Instantiate(ScriptState* script_state,
v8::Local<v8::Module> record,

const KURL& source_url) {
// TODO(rikaf): Remove source_url_
DCHECK_EQ(source_url, source_url_);
v8::Isolate* isolate = script_state->GetIsolate();
v8::TryCatch try_catch(isolate);
try_catch.SetVerbose(true);

DCHECK(!IsNull());
DCHECK(!record.IsEmpty());
v8::Local<v8::Context> context = script_state->GetContext();
probe::ExecuteScript probe(ExecutionContext::From(script_state), source_url);
bool success;
if (!NewLocal(script_state->GetIsolate())
->InstantiateModule(context, &ResolveModuleCallback)
if (!record->InstantiateModule(context, &ResolveModuleCallback)
.To(&success) ||
!success) {
DCHECK(try_catch.HasCaught());
Expand Down Expand Up @@ -156,35 +155,34 @@ void ModuleRecord::ReportException(ScriptState* script_state,
V8ScriptRunner::ReportException(script_state->GetIsolate(), exception);
}

Vector<String> ModuleRecord::ModuleRequests(ScriptState* script_state) {
if (IsNull())
Vector<String> ModuleRecord::ModuleRequests(ScriptState* script_state,
v8::Local<v8::Module> record) {
if (record.IsEmpty())
return Vector<String>();

v8::Local<v8::Module> module = module_->NewLocal(script_state->GetIsolate());

Vector<String> ret;

int length = module->GetModuleRequestsLength();
int length = record->GetModuleRequestsLength();
ret.ReserveInitialCapacity(length);
for (int i = 0; i < length; ++i) {
v8::Local<v8::String> v8_name = module->GetModuleRequest(i);
v8::Local<v8::String> v8_name = record->GetModuleRequest(i);
ret.push_back(ToCoreString(v8_name));
}
return ret;
}

Vector<TextPosition> ModuleRecord::ModuleRequestPositions(
ScriptState* script_state) {
if (IsNull())
ScriptState* script_state,
v8::Local<v8::Module> record) {
if (record.IsEmpty())
return Vector<TextPosition>();
v8::Local<v8::Module> module = module_->NewLocal(script_state->GetIsolate());

Vector<TextPosition> ret;

int length = module->GetModuleRequestsLength();
int length = record->GetModuleRequestsLength();
ret.ReserveInitialCapacity(length);
for (int i = 0; i < length; ++i) {
v8::Location v8_loc = module->GetModuleRequestLocation(i);
v8::Location v8_loc = record->GetModuleRequestLocation(i);
ret.emplace_back(OrdinalNumber::FromZeroBasedInt(v8_loc.GetLineNumber()),
OrdinalNumber::FromZeroBasedInt(v8_loc.GetColumnNumber()));
}
Expand Down
13 changes: 9 additions & 4 deletions third_party/blink/renderer/bindings/core/v8/module_record.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class CORE_EXPORT ModuleRecord final {
DISALLOW_NEW();

public:
static ModuleRecord Compile(
static v8::Local<v8::Module> Compile(
v8::Isolate*,
const String& source,
const KURL& source_url,
Expand All @@ -87,14 +87,19 @@ class CORE_EXPORT ModuleRecord final {
ModuleRecord(v8::Isolate*, v8::Local<v8::Module>, const KURL&);

// Returns exception, if any.
ScriptValue Instantiate(ScriptState*, const KURL& source_url);
static ScriptValue Instantiate(ScriptState*,
v8::Local<v8::Module> record,
const KURL& source_url);

// Returns exception, if any.
ScriptValue Evaluate(ScriptState*) const;
static void ReportException(ScriptState*, v8::Local<v8::Value> exception);

Vector<String> ModuleRequests(ScriptState*);
Vector<TextPosition> ModuleRequestPositions(ScriptState*);
static Vector<String> ModuleRequests(ScriptState*,
v8::Local<v8::Module> record);
static Vector<TextPosition> ModuleRequestPositions(
ScriptState*,
v8::Local<v8::Module> record);

inline bool operator==(const blink::ModuleRecord& other) const;
bool operator!=(const blink::ModuleRecord& other) const {
Expand Down
108 changes: 67 additions & 41 deletions third_party/blink/renderer/bindings/core/v8/module_record_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,20 +86,20 @@ void ModuleRecordTestModulator::Trace(blink::Visitor* visitor) {
TEST(ModuleRecordTest, compileSuccess) {
V8TestingScope scope;
const KURL js_url("https://example.com/foo.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "export const a = 42;", js_url, js_url,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module.IsNull());
ASSERT_FALSE(module.IsEmpty());
}

TEST(ModuleRecordTest, compileFail) {
V8TestingScope scope;
const KURL js_url("https://example.com/foo.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "123 = 456", js_url, js_url, ScriptFetchOptions(),
TextPosition::MinimumPosition(), scope.GetExceptionState());
ASSERT_TRUE(module.IsNull());
ASSERT_TRUE(module.IsEmpty());
EXPECT_TRUE(scope.GetExceptionState().HadException());
}

Expand All @@ -109,15 +109,19 @@ TEST(ModuleRecordTest, equalAndHash) {
const KURL js_url_b("https://example.com/b.js");

ModuleRecord module_null;
ModuleRecord module_a = ModuleRecord::Compile(
v8::Local<v8::Module> local_module_a = ModuleRecord::Compile(
scope.GetIsolate(), "export const a = 'a';", js_url_a, js_url_a,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ModuleRecord module_a =
ModuleRecord(scope.GetIsolate(), local_module_a, js_url_a);
ASSERT_FALSE(module_a.IsNull());
ModuleRecord module_b = ModuleRecord::Compile(
v8::Local<v8::Module> local_module_b = ModuleRecord::Compile(
scope.GetIsolate(), "export const b = 'b';", js_url_b, js_url_b,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ModuleRecord module_b =
ModuleRecord(scope.GetIsolate(), local_module_b, js_url_b);
ASSERT_FALSE(module_b.IsNull());
Vector<char> module_deleted_buffer(sizeof(ModuleRecord));
ModuleRecord& module_deleted =
Expand Down Expand Up @@ -156,13 +160,13 @@ TEST(ModuleRecordTest, equalAndHash) {
TEST(ModuleRecordTest, moduleRequests) {
V8TestingScope scope;
const KURL js_url("https://example.com/foo.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "import 'a'; import 'b'; export const c = 'c';",
js_url, js_url, ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module.IsNull());
ASSERT_FALSE(module.IsEmpty());

auto requests = module.ModuleRequests(scope.GetScriptState());
auto requests = ModuleRecord::ModuleRequests(scope.GetScriptState(), module);
EXPECT_THAT(requests, testing::ContainerEq<Vector<String>>({"a", "b"}));
}

Expand All @@ -175,12 +179,13 @@ TEST(ModuleRecordTest, instantiateNoDeps) {
Modulator::SetModulator(scope.GetScriptState(), modulator);

const KURL js_url("https://example.com/foo.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "export const a = 42;", js_url, js_url,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module.IsNull());
ScriptValue exception = module.Instantiate(scope.GetScriptState(), js_url);
ASSERT_FALSE(module.IsEmpty());
ScriptValue exception =
ModuleRecord::Instantiate(scope.GetScriptState(), module, js_url);
ASSERT_TRUE(exception.IsEmpty());

EXPECT_EQ(0u, resolver->ResolveCount());
Expand All @@ -195,28 +200,33 @@ TEST(ModuleRecordTest, instantiateWithDeps) {
Modulator::SetModulator(scope.GetScriptState(), modulator);

const KURL js_url_a("https://example.com/a.js");
ModuleRecord module_a = ModuleRecord::Compile(
v8::Local<v8::Module> module_a = ModuleRecord::Compile(
scope.GetIsolate(), "export const a = 'a';", js_url_a, js_url_a,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module_a.IsNull());
resolver->PushModuleRecord(module_a);
ASSERT_FALSE(module_a.IsEmpty());
// TODO(rikaf) : Replace ModuleRecord with GCed
resolver->PushModuleRecord(
ModuleRecord(scope.GetIsolate(), module_a, js_url_a));

const KURL js_url_b("https://example.com/b.js");
ModuleRecord module_b = ModuleRecord::Compile(
v8::Local<v8::Module> module_b = ModuleRecord::Compile(
scope.GetIsolate(), "export const b = 'b';", js_url_b, js_url_b,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module_b.IsNull());
resolver->PushModuleRecord(module_b);
ASSERT_FALSE(module_b.IsEmpty());
// TODO(rikaf) : Replace ModuleRecord with GCed
resolver->PushModuleRecord(
ModuleRecord(scope.GetIsolate(), module_b, js_url_b));

const KURL js_url_c("https://example.com/c.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "import 'a'; import 'b'; export const c = 123;",
js_url_c, js_url_c, ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module.IsNull());
ScriptValue exception = module.Instantiate(scope.GetScriptState(), js_url_c);
ASSERT_FALSE(module.IsEmpty());
ScriptValue exception =
ModuleRecord::Instantiate(scope.GetScriptState(), module, js_url_c);
ASSERT_TRUE(exception.IsEmpty());

ASSERT_EQ(2u, resolver->ResolveCount());
Expand All @@ -233,27 +243,36 @@ TEST(ModuleRecordTest, EvaluationErrrorIsRemembered) {
Modulator::SetModulator(scope.GetScriptState(), modulator);

const KURL js_url_f("https://example.com/failure.js");
ModuleRecord module_failure = ModuleRecord::Compile(
v8::Local<v8::Module> module_failure = ModuleRecord::Compile(
scope.GetIsolate(), "nonexistent_function()", js_url_f, js_url_f,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module_failure.IsNull());
ASSERT_TRUE(
module_failure.Instantiate(scope.GetScriptState(), js_url_f).IsEmpty());
ASSERT_FALSE(module_failure.IsEmpty());
ASSERT_TRUE(ModuleRecord::Instantiate(scope.GetScriptState(), module_failure,
js_url_f)
.IsEmpty());
// TODO(rikaf): Replace module_failure_record with module_failure.
ModuleRecord module_failure_record =
ModuleRecord(scope.GetIsolate(), module_failure, js_url_f);
ScriptValue evaluation_error =
module_failure.Evaluate(scope.GetScriptState());
module_failure_record.Evaluate(scope.GetScriptState());
EXPECT_FALSE(evaluation_error.IsEmpty());

resolver->PushModuleRecord(module_failure);
resolver->PushModuleRecord(module_failure_record);

const KURL js_url_c("https://example.com/c.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "import 'failure'; export const c = 123;", js_url_c,
js_url_c, ScriptFetchOptions(), TextPosition::MinimumPosition(),
scope.GetExceptionState());
ASSERT_FALSE(module.IsNull());
ASSERT_TRUE(module.Instantiate(scope.GetScriptState(), js_url_c).IsEmpty());
ScriptValue evaluation_error2 = module.Evaluate(scope.GetScriptState());
ASSERT_FALSE(module.IsEmpty());
ASSERT_TRUE(
ModuleRecord::Instantiate(scope.GetScriptState(), module, js_url_c)
.IsEmpty());
// TODO(rikaf): Replace ModuleRecord with v8::Local<v8::Module>.
ScriptValue evaluation_error2 =
ModuleRecord(scope.GetIsolate(), module, js_url_f)
.Evaluate(scope.GetScriptState());
EXPECT_FALSE(evaluation_error2.IsEmpty());

EXPECT_EQ(evaluation_error, evaluation_error2);
Expand All @@ -269,15 +288,19 @@ TEST(ModuleRecordTest, Evaluate) {
Modulator::SetModulator(scope.GetScriptState(), modulator);

const KURL js_url("https://example.com/foo.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "export const a = 42; window.foo = 'bar';", js_url,
js_url, ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module.IsNull());
ScriptValue exception = module.Instantiate(scope.GetScriptState(), js_url);
ASSERT_FALSE(module.IsEmpty());
ScriptValue exception =
ModuleRecord::Instantiate(scope.GetScriptState(), module, js_url);
ASSERT_TRUE(exception.IsEmpty());

EXPECT_TRUE(module.Evaluate(scope.GetScriptState()).IsEmpty());
// TODO(rikaf): Replace module_record with module.
ModuleRecord module_record = ModuleRecord(scope.GetIsolate(), module, js_url);

EXPECT_TRUE(module_record.Evaluate(scope.GetScriptState()).IsEmpty());
v8::Local<v8::Value> value = scope.GetFrame()
.GetScriptController()
.ExecuteScriptInMainWorldAndReturnValue(
Expand All @@ -286,8 +309,8 @@ TEST(ModuleRecordTest, Evaluate) {
ASSERT_TRUE(value->IsString());
EXPECT_EQ("bar", ToCoreString(v8::Local<v8::String>::Cast(value)));

v8::Local<v8::Object> module_namespace =
v8::Local<v8::Object>::Cast(module.V8Namespace(scope.GetIsolate()));
v8::Local<v8::Object> module_namespace = v8::Local<v8::Object>::Cast(
module_record.V8Namespace(scope.GetIsolate()));
EXPECT_FALSE(module_namespace.IsEmpty());
v8::Local<v8::Value> exported_value =
module_namespace
Expand All @@ -303,14 +326,17 @@ TEST(ModuleRecordTest, EvaluateCaptureError) {
Modulator::SetModulator(scope.GetScriptState(), modulator);

const KURL js_url("https://example.com/foo.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
scope.GetIsolate(), "throw 'bar';", js_url, js_url, ScriptFetchOptions(),
TextPosition::MinimumPosition(), ASSERT_NO_EXCEPTION);
ASSERT_FALSE(module.IsNull());
ScriptValue exception = module.Instantiate(scope.GetScriptState(), js_url);
ASSERT_FALSE(module.IsEmpty());
ScriptValue exception =
ModuleRecord::Instantiate(scope.GetScriptState(), module, js_url);
ASSERT_TRUE(exception.IsEmpty());

ScriptValue error = module.Evaluate(scope.GetScriptState());
// TODO(rikaf): Replace ModuleRecord with v8::Local<v8::Module>.
ScriptValue error = ModuleRecord(scope.GetIsolate(), module, js_url)
.Evaluate(scope.GetScriptState());
ASSERT_FALSE(error.IsEmpty());
ASSERT_TRUE(error.V8Value()->IsString());
EXPECT_EQ("bar", ToCoreString(v8::Local<v8::String>::Cast(error.V8Value())));
Expand Down
12 changes: 8 additions & 4 deletions third_party/blink/renderer/core/layout/custom/layout_worklet_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,19 @@ class LayoutWorkletTest : public PageTestBase {
ScriptState::Scope scope(script_state);

KURL js_url("https://example.com/worklet.js");
ModuleRecord module = ModuleRecord::Compile(
v8::Local<v8::Module> module = ModuleRecord::Compile(
script_state->GetIsolate(), source_code, js_url, js_url,
ScriptFetchOptions(), TextPosition::MinimumPosition(),
ASSERT_NO_EXCEPTION);
EXPECT_FALSE(module.IsNull());
EXPECT_FALSE(module.IsEmpty());

ScriptValue exception = module.Instantiate(script_state, js_url);
ScriptValue exception =
ModuleRecord::Instantiate(script_state, module, js_url);
EXPECT_TRUE(exception.IsEmpty());
return module.Evaluate(script_state);

// TODO(rikaf): Replace ModuleRecord with v8::Local<v8::Module>.
return ModuleRecord(script_state->GetIsolate(), module, js_url)
.Evaluate(script_state);
}

private:
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/loader/modulescript/module_script_loader_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ class ModuleScriptLoaderTestModulator final : public DummyModulator {
requests_.emplace_back(request, TextPosition::MinimumPosition());
}
}
Vector<ModuleRequest> ModuleRequestsFromModuleRecord(ModuleRecord) override {
Vector<ModuleRequest> ModuleRequestsFromModuleRecord(
v8::Local<v8::Module>) override {
return requests_;
}

Expand Down

0 comments on commit 5a8a3c3

Please sign in to comment.