[ES6 modules] Introduce ScriptModuleResolverImpl

[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-specifier)

This CL also introduces Modulator::getFetchedModuleScript() to synchronously get already fetched ModuleScript from the module map.

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

[kouhei (in TOK), 2017-03-30 07:11:36]

kouhei@chromium.org changed reviewers:
+ adamk@chromium.org, dominicc@chromium.org, hiroshige@chromium.org,
neis@chromium.org, yhirano@chromium.org

[kouhei (in TOK), 2017-03-30 07:11:37]

I'd like to add unit test before commit

[kouhei (in TOK), 2017-03-30 07:11:52]

Description was changed from

==========
[ES6 modules] ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

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

to

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

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

[kouhei (in TOK), 2017-04-05 06:27:38]

Description was changed from

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

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

to

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

This CL also introduce Modulator::getFetchedModuleScript() to synchronously get
already fetched ModuleScript from the module map.

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

[kouhei (in TOK), 2017-04-05 06:27:45]

Description was changed from

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

This CL also introduce Modulator::getFetchedModuleScript() to synchronously get
already fetched ModuleScript from the module map.

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

to

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

This CL also introduces Modulator::getFetchedModuleScript() to synchronously get
already fetched ModuleScript from the module map.

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

[kouhei (in TOK), 2017-04-05 06:28:03]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org
- dominicc@chromium.org

[kouhei (in TOK), 2017-04-05 06:28:12]

kouhei@chromium.org changed reviewers:
+ kinuko@chromium.org

[kouhei (in TOK), 2017-04-05 06:28:25]

Added tests. Please review

[kouhei (in TOK), 2017-04-05 06:29:43]

kouhei@chromium.org changed reviewers:
+ domenic@chromium.org

[kouhei (in TOK), 2017-04-05 06:30:11]

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp (right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:46: //
(Modulator) from context where HostResolveImportedModule was called.
+domenic FYI

[domenic, 2017-04-05 07:07:45]

looks good in terms of spec implementation

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp (right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:33: #if
DCHECK_IS_ON()
You may want to add a second line pointing out this is an implementation of
https://html.spec.whatwg.org/commit-snapshots/f8e75a974ed9185e5b462bc5b2dfb32...
specifically.

[haraken, 2017-04-05 11:48:29]

LGTM in terms of implementation.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h (right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h:21: class
CORE_EXPORT ScriptModuleResolverImpl final : public ScriptModuleResolver {

Add a class-level comment.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h:39:
ScriptModuleResolverImpl(Modulator* modulator) : m_modulator(modulator) {}

Add explicit.

[kinuko, 2017-04-05 14:05:01]

some nit-pick comments:

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp (right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:16: DVLOG(1) <<
"ScriptModuleResolverImpl::registerModuleScript(url=\""
I don't think we need to wrap DVLOG with DCHECK_IS_ON

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:40: CHECK_NE(it,
m_recordToModuleScriptMap.end())
Do we need CHECK (not DCHECK) here?

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
(right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:79: KURL
referrerUrl(KURL(), "https://example.com/referrer.js");
KURL referrerUrl(ParsedURLString, "https://example.com/referrer.js");

?  (here and below)

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:135:
EXPECT_EQ(m_modulator->getFetchedModuleScriptCalled(), 0);
nit: EXPECT_EQ expects expected value as 1st param, i.e. (expected, actual)

[yhirano, 2017-04-06 05:30:21]

lgtm

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
(right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:75:
ModuleScript* createReferrerModuleScript(V8TestingScope& scope) {
Please define this function in an unnamed namespace.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:88:
ModuleScript* createTargetModuleScript(V8TestingScope& scope) {
ditto

[kouhei (in TOK), 2017-04-06 05:34:19]

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp (right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:33: #if
DCHECK_IS_ON()
On 2017/04/05 07:07:45, domenic wrote:
> You may want to add a second line pointing out this is an implementation of
>
https://html.spec.whatwg.org/commit-snapshots/f8e75a974ed9185e5b462bc5b2dfb32...
> specifically.

Done.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:40: CHECK_NE(it,
m_recordToModuleScriptMap.end())
On 2017/04/05 14:05:00, kinuko wrote:
> Do we need CHECK (not DCHECK) here?
hiroshige and I try to use CHECK wherever possible in script code to demystify
crash reports.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h (right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h:21: class
CORE_EXPORT ScriptModuleResolverImpl final : public ScriptModuleResolver {
On 2017/04/05 11:48:29, haraken wrote:
> 
> Add a class-level comment.

Done.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h:39:
ScriptModuleResolverImpl(Modulator* modulator) : m_modulator(modulator) {}
On 2017/04/05 11:48:29, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
(right):

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:75:
ModuleScript* createReferrerModuleScript(V8TestingScope& scope) {
On 2017/04/06 05:30:21, yhirano wrote:
> Please define this function in an unnamed namespace.

Done.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:79: KURL
referrerUrl(KURL(), "https://example.com/referrer.js");
On 2017/04/05 14:05:00, kinuko wrote:
> KURL referrerUrl(ParsedURLString, "https://example.com/referrer.js");
> 
> ?  (here and below)

Done.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:88:
ModuleScript* createTargetModuleScript(V8TestingScope& scope) {
On 2017/04/06 05:30:21, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp:135:
EXPECT_EQ(m_modulator->getFetchedModuleScriptCalled(), 0);
On 2017/04/05 14:05:00, kinuko wrote:
> nit: EXPECT_EQ expects expected value as 1st param, i.e. (expected, actual)

Done.

[kouhei (in TOK), 2017-04-06 05:34:23]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-06 05:34:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2790473002/#ps60001
(title: "yhirano/kinuko/haraken")

[commit-bot: I haz the power, 2017-04-06 05:34:32]

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

[kinuko, 2017-04-06 05:42:28]

>
https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:40:
CHECK_NE(it,
> m_recordToModuleScriptMap.end())
> On 2017/04/05 14:05:00, kinuko wrote:
> > Do we need CHECK (not DCHECK) here?
> hiroshige and I try to use CHECK wherever possible in script code to demystify
> crash reports.

Ok, but please consider converting these into DCHECK at some point per our style
guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

(lgtm/2)

[kouhei (in TOK), 2017-04-06 05:46:29]

On 2017/04/06 05:42:28, kinuko wrote:
> >
>
https://codereview.chromium.org/2790473002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp:40:
> CHECK_NE(it,
> > m_recordToModuleScriptMap.end())
> > On 2017/04/05 14:05:00, kinuko wrote:
> > > Do we need CHECK (not DCHECK) here?
> > hiroshige and I try to use CHECK wherever possible in script code to
demystify
> > crash reports.
> 
> Ok, but please consider converting these into DCHECK at some point per our
style
> guide:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> (lgtm/2)

We will bulk convert before turning on the flag by default. Thanks for the
guideline (I wasn't aware)

[commit-bot: I haz the power, 2017-04-06 06:07:05]

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

[commit-bot: I haz the power, 2017-04-06 06:07:06]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kouhei (in TOK), 2017-04-06 06:08:06]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2017-04-06 06:14:02]

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-04-06 07:25:50]

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

[commit-bot: I haz the power, 2017-04-06 07:25:51]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yhirano, 2017-04-07 07:14:12]

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

[commit-bot: I haz the power, 2017-04-07 07:14:30]

Dry run: 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-04-07 08:20:20]

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

[commit-bot: I haz the power, 2017-04-07 08:20:21]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kouhei (in TOK), 2017-04-10 06:14:35]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-10 06:14:35]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2790473002/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2017-04-10 06:14:46]

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-04-10 07:28:54]

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

[commit-bot: I haz the power, 2017-04-10 07:28:55]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kouhei (in TOK), 2017-04-10 08:13:43]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-10 08:13:43]

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

[commit-bot: I haz the power, 2017-04-10 08:14:01]

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-04-10 09:44:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491812023149910,
"parent_rev": "5754a56b61f227091f272bc84bfb212b9ba78416", "commit_rev":
"a0158c4544b8dbf598dfb829426ab6d755843369"}

[commit-bot: I haz the power, 2017-04-10 09:45:07]

Description was changed from

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

This CL also introduces Modulator::getFetchedModuleScript() to synchronously get
already fetched ModuleScript from the module map.

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

to

==========
[ES6 modules] Introduce ScriptModuleResolverImpl

This CL introduces ScriptModuleResolverImpl, which implements
https://html.spec.whatwg.org/#hostresolveimportedmodule(referencingmodule,-sp...

This CL also introduces Modulator::getFetchedModuleScript() to synchronously get
already fetched ModuleScript from the module map.

BUG=594639

Review-Url: https://codereview.chromium.org/2790473002
Cr-Commit-Position: refs/heads/master@{#463205}
Committed:
https://chromium.googlesource.com/chromium/src/+/a0158c4544b8dbf598dfb829426a...
==========

[commit-bot: I haz the power, 2017-04-10 09:45:09]

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