[ES6 modules] ModuleTreeLinker::FetchDescendants should not assume urls isn't empty.

[ES6 modules] ModuleTreeLinker::FetchDescendants should not assume urls isn't empty.

Before this CL, we assumed "urls" in ModuleTreeLinker::FetchDescendants() to be non-empty
if record.[[RequestedModules]] is not empty.

However, this isn't always the case, as the resolved url may be in the ancestor list.
This CL removes the DCHECK based on this assumption, and complete the algorithm in that case.

TEST=ModuleTreeLinkerTest.FetchDependencyOfCyclicGraph
BUG=594639
Review-Url: https://codereview.chromium.org/2852963002
Cr-Commit-Position: refs/heads/master@{#468554}
Committed: https://chromium.googlesource.com/chromium/src/+/d087434ab42e298587480260ee4cafbfd002e614

[kouhei (in TOK), 2017-05-01 06:42:17]

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

[commit-bot: I haz the power, 2017-05-01 06:42:27]

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-05-01 06:42:47]

kouhei@chromium.org changed reviewers:
+ hiroshige@chromium.org, kinuko@chromium.org, module-dev@chromium.org

[kouhei (in TOK), 2017-05-01 06:43:14]

Description was changed from

==========
[ES6 modules] ModuleTreeLinker::FetchDescendants should not assume urls isn't
empty.

Before this CL, we assumed "urls" in ModuleTreeLinker::FetchDescendants() to be
non-empty
if record.[[RequestedModules]] is not empty.

However, this isn't always the case, as the resolved url may be in the ancestor
list.
This CL removes the DCHECK based on this assumption, and complete the algorithm
in that case.

TEST=ModuleTreeLinkerTest.FetchDependencyOfCyclicGraph
BUG=594639
==========

to

==========
[ES6 modules] ModuleTreeLinker::FetchDescendants should not assume urls isn't
empty.

Before this CL, we assumed "urls" in ModuleTreeLinker::FetchDescendants() to be
non-empty
if record.[[RequestedModules]] is not empty.

However, this isn't always the case, as the resolved url may be in the ancestor
list.
This CL removes the DCHECK based on this assumption, and complete the algorithm
in that case.

TEST=ModuleTreeLinkerTest.FetchDependencyOfCyclicGraph
BUG=594639
==========

[kouhei (in TOK), 2017-05-01 06:43:14]

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

[commit-bot: I haz the power, 2017-05-01 08:27:06]

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

[commit-bot: I haz the power, 2017-05-01 08:27:07]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-01 18:00:49]

lgtm

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:288: //
Step 5, when "urls" is empty, or...
Could you clarify this (Line 288, and Line 299) is not a text from the spec?

[kinuko, 2017-05-01 22:27:02]

lgtm

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:290: //
this algorithm with module script.
If line 289 is true line 288 should be true too, so connecting these by 'or...' 
seems a bit strange to me.

'If "urls" is empty, that happens when record.[[RequestedModules]] is empty in
Step 2, or when resolved URL was already in the ancestor list, asynchronously
complete this algorithm with module script'

? (If these are not exact spec text)

[hiroshige, 2017-05-01 23:16:12]

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:290: //
this algorithm with module script.
On 2017/05/01 22:27:02, kinuko wrote:
> If line 289 is true line 288 should be true too, so connecting these by
'or...' 
> seems a bit strange to me.
> 
> 'If "urls" is empty, that happens when record.[[RequestedModules]] is empty in
> Step 2, or when resolved URL was already in the ancestor list, asynchronously
> complete this algorithm with module script'
> 
> ? (If these are not exact spec text)

[optional] My preference is e.g.:
  Step 2. If record.[[RequestedModules]] is empty, asynchronously
  complete this algorithm with module script. [spec text]
  Also, if record.[[RequestedModules]] is not empty but |urls| is
  empty here, we can immediately proceed to Instantiate() because
  we don't have to do or wait for anything in Step 5. [non-spec text]
(and perhaps placing this if block before Line 278).

[kouhei (in TOK), 2017-05-02 01:20:38]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-05-02 01:20:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2852963002/#ps20001
(title: "commentfix")

[commit-bot: I haz the power, 2017-05-02 01:21:17]

CQ is trying da patch.

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

[kouhei (in TOK), 2017-05-02 01:21:30]

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
(right):

https://codereview.chromium.org/2852963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp:290: //
this algorithm with module script.
On 2017/05/01 23:16:12, hiroshige wrote:
> On 2017/05/01 22:27:02, kinuko wrote:
> > If line 289 is true line 288 should be true too, so connecting these by
> 'or...' 
> > seems a bit strange to me.
> > 
> > 'If "urls" is empty, that happens when record.[[RequestedModules]] is empty
in
> > Step 2, or when resolved URL was already in the ancestor list,
asynchronously
> > complete this algorithm with module script'
> > 
> > ? (If these are not exact spec text)
> 
> [optional] My preference is e.g.:
>   Step 2. If record.[[RequestedModules]] is empty, asynchronously
>   complete this algorithm with module script. [spec text]
>   Also, if record.[[RequestedModules]] is not empty but |urls| is
>   empty here, we can immediately proceed to Instantiate() because
>   we don't have to do or wait for anything in Step 5. [non-spec text]
> (and perhaps placing this if block before Line 278).

Done.

[commit-bot: I haz the power, 2017-05-02 03:06:39]

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

[commit-bot: I haz the power, 2017-05-02 03:06:40]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kouhei (in TOK), 2017-05-02 03:31:50]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2017-05-02 03:32:08]

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-05-02 04:15:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493695910564350,
"parent_rev": "2c36828dfc0bef3e7792bdd6e13fdb199b5d97fd", "commit_rev":
"d087434ab42e298587480260ee4cafbfd002e614"}

[commit-bot: I haz the power, 2017-05-02 04:16:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493695910564350,
"parent_rev": "2c36828dfc0bef3e7792bdd6e13fdb199b5d97fd", "commit_rev":
"d087434ab42e298587480260ee4cafbfd002e614"}

[commit-bot: I haz the power, 2017-05-02 04:17:28]

Description was changed from

==========
[ES6 modules] ModuleTreeLinker::FetchDescendants should not assume urls isn't
empty.

Before this CL, we assumed "urls" in ModuleTreeLinker::FetchDescendants() to be
non-empty
if record.[[RequestedModules]] is not empty.

However, this isn't always the case, as the resolved url may be in the ancestor
list.
This CL removes the DCHECK based on this assumption, and complete the algorithm
in that case.

TEST=ModuleTreeLinkerTest.FetchDependencyOfCyclicGraph
BUG=594639
==========

to

==========
[ES6 modules] ModuleTreeLinker::FetchDescendants should not assume urls isn't
empty.

Before this CL, we assumed "urls" in ModuleTreeLinker::FetchDescendants() to be
non-empty
if record.[[RequestedModules]] is not empty.

However, this isn't always the case, as the resolved url may be in the ancestor
list.
This CL removes the DCHECK based on this assumption, and complete the algorithm
in that case.

TEST=ModuleTreeLinkerTest.FetchDependencyOfCyclicGraph
BUG=594639

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

[commit-bot: I haz the power, 2017-05-02 04:17:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d087434ab42e298587480260ee4c...