Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567933003:
Remove DOMWindowProperty from PingLoaderImpl (Closed)
DescriptionRemove DOMWindowProperty from PingLoaderImpl
PingLoaderImpl just needs a WeakMember<LocalFrame>.
It doesn't need to inherit from DOMWindowProperty.
BUG=610176
Committed: https://crrev.com/89aaa753cac97aa76612a34dcff6bd5d1a78d698
Cr-Commit-Position: refs/heads/master@{#437894}
Patch Set 1 #
Total comments: 4
Patch Set 2 : temp #
Messages
Total messages: 16 (7 generated)
haraken@chromium.org changed reviewers: + sigbjornf@opera.com
PTAL
lgtm https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/PingLoader.cpp (right): https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/PingLoader.cpp:189: class PingLoaderImpl : public GarbageCollectedFinalized<PingLoaderImpl>, final https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/PingLoader.cpp:201: DECLARE_VIRTUAL_TRACE(); Can be DECLARE_TRACE() now. https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/PingLoader.cpp:315: if (m_frame) { if (m_frame && m_frame->document()) { ... } or if you prefer keeping the nesting, if (m_frame) { if (Document* document = m_frame->document()) { document->... } } https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/PingLoader.h (right): https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/PingLoader.h:39: #include "platform/heap/SelfKeepAlive.h" I don't know how this came about, but could you move some/most of these #includes to the .cpp while you're here? thanks.
> https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/loader/PingLoader.cpp (right): > > https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/loader/PingLoader.cpp:189: class PingLoaderImpl : > public GarbageCollectedFinalized<PingLoaderImpl>, > final > > https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/loader/PingLoader.cpp:201: > DECLARE_VIRTUAL_TRACE(); > Can be DECLARE_TRACE() now. > > https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/loader/PingLoader.cpp:315: if (m_frame) { > if (m_frame && m_frame->document()) { ... } > > or if you prefer keeping the nesting, > > if (m_frame) { > if (Document* document = m_frame->document()) { > document->... > } > } > > https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/loader/PingLoader.h (right): > > https://codereview.chromium.org/2567933003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/loader/PingLoader.h:39: #include > "platform/heap/SelfKeepAlive.h" > I don't know how this came about, but could you move some/most of these > #includes to the .cpp while you're here? thanks. Done.
The CQ bit was checked by haraken@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481556497411320, "parent_rev":
"be79543de36b3874acda34c3f2a7c3c460635db1", "commit_rev":
"0d5547b94b1b1e81ab29b17c1402e7a721248a28"}
Message was sent while issue was closed.
Description was changed from ========== Remove DOMWindowProperty from PingLoaderImpl PingLoaderImpl just needs a WeakMember<LocalFrame>. It doesn't need to inherit from DOMWindowProperty. BUG=610176 ========== to ========== Remove DOMWindowProperty from PingLoaderImpl PingLoaderImpl just needs a WeakMember<LocalFrame>. It doesn't need to inherit from DOMWindowProperty. BUG=610176 Review-Url: https://codereview.chromium.org/2567933003 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove DOMWindowProperty from PingLoaderImpl PingLoaderImpl just needs a WeakMember<LocalFrame>. It doesn't need to inherit from DOMWindowProperty. BUG=610176 Review-Url: https://codereview.chromium.org/2567933003 ========== to ========== Remove DOMWindowProperty from PingLoaderImpl PingLoaderImpl just needs a WeakMember<LocalFrame>. It doesn't need to inherit from DOMWindowProperty. BUG=610176 Committed: https://crrev.com/89aaa753cac97aa76612a34dcff6bd5d1a78d698 Cr-Commit-Position: refs/heads/master@{#437894} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/89aaa753cac97aa76612a34dcff6bd5d1a78d698 Cr-Commit-Position: refs/heads/master@{#437894}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2569983002/ by haraken@chromium.org. The reason for reverting is: This CL was wrong. WeakMember is not cleared until the frame is garbage-collected. This wrongly changes the lifetime of PingLoaderImpl. PingLoaderImpl must stop working when the associated context is detached, not when the frame is garbage-collected.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
