Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(113)

Issue 2850673002: Move a RegisterModuleScript() call to ModuleScript::Create() (Closed)

Created:
3 years, 7 months ago by hiroshige
Modified:
3 years, 7 months ago
Reviewers:
kouhei (in TOK)
CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Move a RegisterModuleScript() call to ModuleScript::CreateInternal() To align with the spec, and also this is needed for inline module scripts because NotifyNewSingleModuleFinished() is not called for inline scripts. This CL also makes DummyModulator to ignore RegisterModuleScript() calls where it is not tested explcitly, because this CL makes an additional RegisterModuleScript() call for CreateForTest() which is not coupled with NotifyNewSingleModuleFinished(). This CL doesn't change the non-test behavior, because in the non-test code ModuleScript::Create() is always coupled with NotifyNewSingleModuleFinished() and thus this CL just makes RegisterModuleScript() a little earlier. BUG=594639, 715369 Review-Url: https://codereview.chromium.org/2850673002 Cr-Commit-Position: refs/heads/master@{#467907} Committed: https://chromium.googlesource.com/chromium/src/+/fca892fe54b316243806c3b1cfc6cad5e23ae098

Patch Set 1 #

Patch Set 2 : Fix includes #

Patch Set 3 : Rebase #

Patch Set 4 : Fix tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -11 lines) Patch
M third_party/WebKit/Source/core/dom/ModuleMap.cpp View 1 2 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ModuleScript.cpp View 1 2 3 2 chunks +9 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/testing/DummyModulator.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/testing/DummyModulator.cpp View 1 2 3 2 chunks +24 lines, -3 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 29 (22 generated)
hiroshige
PTAL.
3 years, 7 months ago (2017-04-27 21:56:04 UTC) #13
kouhei (in TOK)
lgtm
3 years, 7 months ago (2017-04-28 00:08:50 UTC) #14
hiroshige
On 2017/04/28 00:08:50, kouhei wrote: > lgtm Oh, unit tests are failing. It seems I ...
3 years, 7 months ago (2017-04-28 00:19:11 UTC) #15
hiroshige
On 2017/04/28 00:19:11, hiroshige wrote: > On 2017/04/28 00:08:50, kouhei wrote: > > lgtm > ...
3 years, 7 months ago (2017-04-28 01:08:51 UTC) #19
kouhei (in TOK)
still lgtm
3 years, 7 months ago (2017-04-28 01:13:09 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2850673002/60001
3 years, 7 months ago (2017-04-28 05:38:07 UTC) #26
commit-bot: I haz the power
3 years, 7 months ago (2017-04-28 05:42:14 UTC) #29
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fca892fe54b316243806c3b1cfc6...

Powered by Google App Engine
This is Rietveld 408576698