Introduce ModulePendingScript

Introduce ModulePendingScript

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

[hiroshige, 2017-04-12 19:43:47]

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

[hiroshige, 2017-04-12 19:43:48]

This CL makes ModulePendingScript to be TraceWrapperBase, because
ModulePendingScript has an indirect reference to ModuleScript.

Question: should we make all the classes that can reference ModulePendingScript
inderectly to be TraceWrapperBase and make all those Member<>s to be
TraceWrapperMember<>? i.e.
- PendingScript (the parent class),
- ScriptLoader (that holds PendingScript),
- ScriptElementBase,
etc. etc.

[hiroshige, 2017-04-12 19:44:02]

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

[commit-bot: I haz the power, 2017-04-12 19:45:27]

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

[hiroshige, 2017-04-12 19:57:42]

Description was changed from

==========
Introduce ModulePendingScript

BUG=
==========

to

==========
Introduce ModulePendingScript

BUG=
==========

[commit-bot: I haz the power, 2017-04-12 20:31:09]

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

[commit-bot: I haz the power, 2017-04-12 20:31:10]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[kouhei (in TOK), 2017-04-13 02:25:24]

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulePendingScript.h (right):

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:1: #ifndef
ModulePendingScript_h
missing the copyright 2017 comment

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:21: public
TraceWrapperBase,
This doesn't have to be a TraceWrapperBase as discussed.
ModuleScript::m_instantiationError will be kept alive via
v8::Context->PerContextData->Modulator->ModuleMap->::Entry->ModuleScript->m_instantiationError.

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:46:
TraceWrapperMember<ModulePendingScript> pending_script_;
Ditto

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:65:
DECLARE_TRACE_WRAPPERS();
Ditto

[kouhei (in TOK), 2017-04-13 02:25:44]

mostly lg % TraceWrapper stuff

[hiroshige, 2017-04-13 02:55:37]

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulePendingScript.h (right):

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:21: public
TraceWrapperBase,
On 2017/04/13 02:25:24, kouhei wrote:
> This doesn't have to be a TraceWrapperBase as discussed.
> ModuleScript::m_instantiationError will be kept alive via
>
v8::Context->PerContextData->Modulator->ModuleMap->::Entry->ModuleScript->m_instantiationError.

So, we can avoid TraceWrapperMember<> and TraceWrapperBase everywhere, except
for the classes on the path above, particularly
- We can remove TraceWrapperBase and TraceWrapperMember<> in this file, and
- Also we can make Script non-TraceWrapperBase,
right?

I think we have to add a comment about how m_instantiationError is kept alive
somewhere.

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

On 2017/04/13 02:55:37, hiroshige wrote:
>
https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ModulePendingScript.h (right):
> 
>
https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ModulePendingScript.h:21: public
> TraceWrapperBase,
> On 2017/04/13 02:25:24, kouhei wrote:
> > This doesn't have to be a TraceWrapperBase as discussed.
> > ModuleScript::m_instantiationError will be kept alive via
> >
>
v8::Context->PerContextData->Modulator->ModuleMap->::Entry->ModuleScript->m_instantiationError.
> 
> So, we can avoid TraceWrapperMember<> and TraceWrapperBase everywhere, except
> for the classes on the path above, particularly
> - We can remove TraceWrapperBase and TraceWrapperMember<> in this file, and
> - Also we can make Script non-TraceWrapperBase,
> right?
Correct.

> I think we have to add a comment about how m_instantiationError is kept alive
> somewhere.
Yes. Please.

[hiroshige, 2017-04-13 17:37:56]

Description was changed from

==========
Introduce ModulePendingScript

BUG=
==========

to

==========
Introduce ModulePendingScript

BUG=594639, 686281
==========

[hiroshige, 2017-04-13 17:38:14]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, sigbjornf@opera.com

[hiroshige, 2017-04-13 17:50:13]

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

[commit-bot: I haz the power, 2017-04-13 17:51:07]

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

[hiroshige, 2017-04-13 17:53:56]

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ModulePendingScript.h (right):

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:1: #ifndef
ModulePendingScript_h
On 2017/04/13 02:25:24, kouhei wrote:
> missing the copyright 2017 comment

Done.

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:21: public
TraceWrapperBase,
On 2017/04/13 02:55:37, hiroshige wrote:
> On 2017/04/13 02:25:24, kouhei wrote:
> > This doesn't have to be a TraceWrapperBase as discussed.
> > ModuleScript::m_instantiationError will be kept alive via
> >
>
v8::Context->PerContextData->Modulator->ModuleMap->::Entry->ModuleScript->m_instantiationError.
> 
> So, we can avoid TraceWrapperMember<> and TraceWrapperBase everywhere, except
> for the classes on the path above, particularly
> - We can remove TraceWrapperBase and TraceWrapperMember<> in this file, and
> - Also we can make Script non-TraceWrapperBase,
> right?
> 
> I think we have to add a comment about how m_instantiationError is kept alive
> somewhere.

Done.
Also created a separate CL for adding comments and making Script
non-TraceWrapperBase:
https://codereview.chromium.org/2820533003/

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:46:
TraceWrapperMember<ModulePendingScript> pending_script_;
On 2017/04/13 02:25:24, kouhei wrote:
> Ditto

Done.

https://codereview.chromium.org/2815453006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:65:
DECLARE_TRACE_WRAPPERS();
On 2017/04/13 02:25:24, kouhei wrote:
> Ditto

Done.

https://codereview.chromium.org/2815453006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Modulator.h (right):

https://codereview.chromium.org/2815453006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Modulator.h:37: // A ModuleTreeClient is
notified when a module script and its whole descendent
This section is imported from kouhei's CL to pass compilation
(ModulePendingScript.h depends on this declaration).

[hiroshige, 2017-04-13 17:54:07]

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

[commit-bot: I haz the power, 2017-04-13 17:54:16]

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

[hiroshige, 2017-04-13 17:59:09]

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

[commit-bot: I haz the power, 2017-04-13 17:59:51]

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-13 22:04:09]

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

[commit-bot: I haz the power, 2017-04-13 22:04:10]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-14 00:20:44]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-14 00:20:46]

lgtm

https://codereview.chromium.org/2815453006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ModulePendingScript.h (right):

https://codereview.chromium.org/2815453006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ModulePendingScript.h:86: void
RemoveFromMemoryCache() override { NOTREACHED(); }
Comment explaining "why" would help

[commit-bot: I haz the power, 2017-04-14 00:21:09]

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

[hiroshige, 2017-04-14 00:27:15]

Description was changed from

==========
Introduce ModulePendingScript

BUG=594639, 686281
==========

to

==========
Introduce ModulePendingScript

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

[commit-bot: I haz the power, 2017-04-14 00:36:38]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492129244634420,
"parent_rev": "90a1c8c75a3cc1cc60f4bd9cc88473ae161ff038", "commit_rev":
"2e5a5b8efccf655f183cc7ce0c59a88f77bb50f5"}

[commit-bot: I haz the power, 2017-04-14 00:37:36]

Description was changed from

==========
Introduce ModulePendingScript

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

to

==========
Introduce ModulePendingScript

BUG=594639

Review-Url: https://codereview.chromium.org/2815453006
Cr-Commit-Position: refs/heads/master@{#464627}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e5a5b8efccf655f183cc7ce0c59...
==========

[commit-bot: I haz the power, 2017-04-14 00:37:37]

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

[hiroshige, 2017-04-14 18:55:06]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2816043005/ by hiroshige@chromium.org.

The reason for reverting is: Reason for revert:
https://codereview.chromium.org/2653923008/ is
suspected to cause CHECK() failure (crbug.com/711703)
inside PendingScript's prefinalizer.

BUG=711703.