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

Issue 2844583002: 4. Do not call Dispose() as ClassicPendingScript's prefinalizer (Closed)

Created:
3 years, 8 months ago by hiroshige
Modified:
3 years, 8 months ago
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

Do not call Dispose() as ClassicPendingScript's prefinalizer Because the things in ClassicPendingScript::DisposeInternal() except for ScriptStreamer::Cancel() doesn't have to be called as a prefinalizer, this CL introduces ClassicPendingScript::Prefinalize() that only calls ScriptStreamer::Cancel() and thus makes Dispose() not to be called there. This CL simplified the prefinalization of ClassicPendingScript, especially order dependencies between ClassicPendingScript's prefinalizer and the prefinalizer of its parent class (ResourceOwner). Leaving ClassicPendingScript in a not-Dispose()d state is not observable if the related classes obeys the rule of Oilpan, and https://codereview.chromium.org/2837413002/ checks that in case there were a bug. BUG=715309 Review-Url: https://codereview.chromium.org/2844583002 Cr-Commit-Position: refs/heads/master@{#467299} Committed: https://chromium.googlesource.com/chromium/src/+/faa055103d806be6bcdb05b721c954d1b1584200

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -12 lines) Patch
M third_party/WebKit/Source/core/dom/ClassicPendingScript.h View 2 chunks +2 lines, -10 lines 2 comments Download
M third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp View 1 chunk +5 lines, -2 lines 2 comments Download

Depends on Patchset:

Messages

Total messages: 18 (12 generated)
kouhei (in TOK)
lgtm https://codereview.chromium.org/2844583002/diff/1/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp (right): https://codereview.chromium.org/2844583002/diff/1/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp#newcode51 third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:51: // TODO(hiroshige): Consider moving this to ScriptStreamer's prefinalizer. ...
3 years, 8 months ago (2017-04-26 00:22:52 UTC) #5
hiroshige
PTAL haraken@ and kouhei@ for Oilpan issues. sof@ (who touched related code when migrating from ...
3 years, 8 months ago (2017-04-26 00:35:31 UTC) #9
haraken
LGTM https://codereview.chromium.org/2844583002/diff/1/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp File third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp (right): https://codereview.chromium.org/2844583002/diff/1/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp#newcode51 third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp:51: // TODO(hiroshige): Consider moving this to ScriptStreamer's prefinalizer. ...
3 years, 8 months ago (2017-04-26 01:06:23 UTC) #10
kouhei (in TOK)
https://codereview.chromium.org/2844583002/diff/1/third_party/WebKit/Source/core/dom/ClassicPendingScript.h File third_party/WebKit/Source/core/dom/ClassicPendingScript.h (right): https://codereview.chromium.org/2844583002/diff/1/third_party/WebKit/Source/core/dom/ClassicPendingScript.h#newcode30 third_party/WebKit/Source/core/dom/ClassicPendingScript.h:30: USING_PRE_FINALIZER(ClassicPendingScript, Prefinalize); On 2017/04/26 01:06:23, haraken wrote: > > ...
3 years, 8 months ago (2017-04-26 01:09:35 UTC) #11
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/2844583002/1
3 years, 8 months ago (2017-04-26 11:40:49 UTC) #15
commit-bot: I haz the power
3 years, 8 months ago (2017-04-26 11:45:06 UTC) #18
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/faa055103d806be6bcdb05b721c9...

Powered by Google App Engine
This is Rietveld 408576698