Create bare-bones ScriptModule class

Create bare-bones ScriptModule class

Provides a ScriptModule wrapper around v8::Module, as
well as primordial forms of the three basic operations
on modules:

  - compile(), a static method which takes a string and
    returns a ScriptModule
  - instantiate(), which "links" a module with its dependencies
    (the initial implementation does no actual linking)
  - evaluate(), which runs the module (and its dependencies;
    naturally this version doesn't do the latter)

Landing this in its early state makes it easier for others
working on <script type="module"> support to make progress
on other CLs, many of which will interact with this class

BUG=594639
Review-Url: https://codereview.chromium.org/2566513002
Cr-Commit-Position: refs/heads/master@{#443112}
Committed: https://chromium.googlesource.com/chromium/src/+/57c86c65d60a0e5da917fa3cb6c6b374821b7984

[adamk, 2017-01-10 20:21:08]

Description was changed from

==========
Work in progress: ScriptModule

Provides a basic ScriptModule wrapper around v8::Module, as well
as a static compile() method for creating a v8::Module given
a string.
==========

to

==========
Create bare-bones ScriptModule class

Provides a ScriptModule wrapper around v8::Module, as
well as primordial forms of the three basic operations
on modules:

  - compile(), a static method which takes a string and
    returns a ScriptModule
  - instantiate(), which "links" a module with its dependencies
    (the initial implementation does no actual linking)
  - evaluate(), which runs the module (and its dependencies;
    naturally this version doesn't do the latter)

Landing this in its early state makes it easier for others
working on <script type="module"> support to make progress
on other CLs, many of which will interact with this class

BUG=594639
==========

[adamk, 2017-01-10 20:21:31]

adamk@chromium.org changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org

[adamk, 2017-01-10 20:21:38]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:22:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kouhei (in TOK), 2017-01-10 22:48:38]

lgtm

haraken: This is to unblock split&landing
https://codereview.chromium.org/2555653002/

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:36:
RefPtr<SharedPersistent<v8::Module>> m_module;
Do you mind if I convert this to ScopedPersistent in the future? (but not yet)

[adamk, 2017-01-10 22:53:18]

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:36:
RefPtr<SharedPersistent<v8::Module>> m_module;
On 2017/01/10 22:48:38, kouhei wrote:
> Do you mind if I convert this to ScopedPersistent in the future? (but not yet)

I think I tried to make this a ScopedPersistent and ran into problems because of
the use of ScriptModule as a value type. Happy for it to be anything that works
:)

[kouhei (in TOK), 2017-01-10 22:58:29]

On 2017/01/10 22:53:18, adamk wrote:
>
https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):
> 
>
https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:36:
> RefPtr<SharedPersistent<v8::Module>> m_module;
> On 2017/01/10 22:48:38, kouhei wrote:
> > Do you mind if I convert this to ScopedPersistent in the future? (but not
yet)
> 
> I think I tried to make this a ScopedPersistent and ran into problems because
of
> the use of ScriptModule as a value type. Happy for it to be anything that
works
> :)
Thanks.

Is this in my mega CL? https://codereview.chromium.org/2555653002/
In that case, yes, the current CL depends on the ScriptModule being
SharedPersistent, so I need more work on it.

[commit-bot: I haz the power, 2017-01-10 23:12:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 23:12:39]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-01-11 02:21:35]

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:27: if
(!v8Call(v8::ScriptCompiler::CompileModule(isolate, &scriptSource), module,

I want to create a helper function in V8ScriptRunner so that we can centralize
all compile & run logic there. And add tracing / inspector instrumentation,
MicroTaskScope etc as necessary.

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:46:
->Instantiate(context, &dummyCallback);

Can we make V8 accept a nullptr as a callback?

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:50: // to code
there.

Yes, I'd prefer moving the Evaluate call to V8ScriptRunner. You can keep the
TryCatch block here though.

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:36:
RefPtr<SharedPersistent<v8::Module>> m_module;
On 2017/01/10 22:53:18, adamk wrote:
> On 2017/01/10 22:48:38, kouhei wrote:
> > Do you mind if I convert this to ScopedPersistent in the future? (but not
yet)
> 
> I think I tried to make this a ScopedPersistent and ran into problems because
of
> the use of ScriptModule as a value type. Happy for it to be anything that
works
> :)

Ah, this makes sense.

[adamk, 2017-01-11 18:27:30]

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:27: if
(!v8Call(v8::ScriptCompiler::CompileModule(isolate, &scriptSource), module,
On 2017/01/11 02:21:35, haraken wrote:
> 
> I want to create a helper function in V8ScriptRunner so that we can centralize
> all compile & run logic there. And add tracing / inspector instrumentation,
> MicroTaskScope etc as necessary.

Ok added a helper to V8ScriptRunner with some basic tracing.

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:46:
->Instantiate(context, &dummyCallback);
On 2017/01/11 02:21:35, haraken wrote:
> 
> Can we make V8 accept a nullptr as a callback?

There's not a good use for such a thing in the module API: this really isn't an
optional piece of modules.

https://codereview.chromium.org/2566513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:50: // to code
there.
On 2017/01/11 02:21:35, haraken wrote:
> 
> Yes, I'd prefer moving the Evaluate call to V8ScriptRunner. You can keep the
> TryCatch block here though.

Done

[haraken, 2017-01-11 23:40:59]

LGTM

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:1: // Copyright 2016
The Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.

2017

[adamk, 2017-01-11 23:43:12]

The CQ bit was checked by adamk@chromium.org

[adamk, 2017-01-11 23:43:13]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2566513002/#ps100001
(title: "Update copyrights")

[adamk, 2017-01-11 23:43:15]

Thanks for the review

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right):

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/01/11 23:40:59, haraken wrote:
> 
> 2017

Done.

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right):

https://codereview.chromium.org/2566513002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/01/11 23:40:59, haraken wrote:
> 
> 2017

Done.

[commit-bot: I haz the power, 2017-01-11 23:44:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 01:36:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484178192669900,
"parent_rev": "d6ab9a5f6ec448b92078fdb56dc041b42772c385", "commit_rev":
"57c86c65d60a0e5da917fa3cb6c6b374821b7984"}

[commit-bot: I haz the power, 2017-01-12 01:36:48]

Description was changed from

==========
Create bare-bones ScriptModule class

Provides a ScriptModule wrapper around v8::Module, as
well as primordial forms of the three basic operations
on modules:

  - compile(), a static method which takes a string and
    returns a ScriptModule
  - instantiate(), which "links" a module with its dependencies
    (the initial implementation does no actual linking)
  - evaluate(), which runs the module (and its dependencies;
    naturally this version doesn't do the latter)

Landing this in its early state makes it easier for others
working on <script type="module"> support to make progress
on other CLs, many of which will interact with this class

BUG=594639
==========

to

==========
Create bare-bones ScriptModule class

Provides a ScriptModule wrapper around v8::Module, as
well as primordial forms of the three basic operations
on modules:

  - compile(), a static method which takes a string and
    returns a ScriptModule
  - instantiate(), which "links" a module with its dependencies
    (the initial implementation does no actual linking)
  - evaluate(), which runs the module (and its dependencies;
    naturally this version doesn't do the latter)

Landing this in its early state makes it easier for others
working on <script type="module"> support to make progress
on other CLs, many of which will interact with this class

BUG=594639

Review-Url: https://codereview.chromium.org/2566513002
Cr-Commit-Position: refs/heads/master@{#443112}
Committed:
https://chromium.googlesource.com/chromium/src/+/57c86c65d60a0e5da917fa3cb6c6...
==========

[commit-bot: I haz the power, 2017-01-12 01:36:49]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/57c86c65d60a0e5da917fa3cb6c6...