|
|
DescriptionIntroduce 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
Patch Set 1 #
Total comments: 9
Patch Set 2 : Remove TraceWrappers #Patch Set 3 : Add header #Patch Set 4 : refine #
Total comments: 1
Patch Set 5 : update comments #Patch Set 6 : Rebase #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (20 generated)
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org, yhirano@chromium.org
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.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Introduce ModulePendingScript BUG= ========== to ========== Introduce ModulePendingScript BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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
mostly lg % TraceWrapper stuff
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.
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.
Description was changed from ========== Introduce ModulePendingScript BUG= ========== to ========== Introduce ModulePendingScript BUG=594639, 686281 ==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, sigbjornf@opera.com
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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).
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kouhei@chromium.org
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Introduce ModulePendingScript BUG=594639, 686281 ========== to ========== Introduce ModulePendingScript BUG=594639 ==========
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492129244634420, "parent_rev": "90a1c8c75a3cc1cc60f4bd9cc88473ae161ff038", "commit_rev": "2e5a5b8efccf655f183cc7ce0c59a88f77bb50f5"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2e5a5b8efccf655f183cc7ce0c59...
Message was sent while issue was closed.
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. |