Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2134113002: Use ChildFrameDisconnector when detaching child frames of a LocalFrame. (Closed)

Created:
3 years, 1 month ago by dcheng
Modified:
3 years, 1 month ago
Reviewers:
esprehn, Nate Chapin
CC:
esprehn, blink-reviews, chromium-reviews, dcheng, haraken, mlamouri+watch-blink_chromium.org, ojan, piman, sof, Yuki
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use ChildFrameDisconnector when detaching child frames of a LocalFrame. Currently, UpdateSuspendScope is used to defer widget updates when the DOM hierarchy is mutated. This is used to prevent script from running in the middle of DOM mutations, since plugins can run script when destroyed. This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part 1 changes LocalFrame detach to always use ChildFrameDisconnector to detach child frames. Part 2 will rework ChildFrameDisconnector to also detach plugin elements. This should eliminate the need to defer widget updates, since script will never have to run during the actual mutation of internal state. BUG=524113, 528867, 561683, 621362 Committed: https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0 Cr-Commit-Position: refs/heads/master@{#405040}

Patch Set 1 #

Patch Set 2 : Meh #

Patch Set 3 : DCHECK #

Total comments: 5

Patch Set 4 : Use local #

Patch Set 5 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -12 lines) Patch
M third_party/WebKit/Source/core/frame/Frame.h View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.cpp View 1 chunk +0 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.cpp View 1 2 3 4 2 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrame.h View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrame.cpp View 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (12 generated)
dcheng
Note that this is somewhat speculative: I'm still working on persisted widget handling in the ...
3 years, 1 month ago (2016-07-11 12:15:42 UTC) #5
esprehn
Hmm, what about inside updateStyle when we unload plugins? https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Source/core/frame/RemoteFrame.cpp File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right): https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Source/core/frame/RemoteFrame.cpp#newcode204 third_party/WebKit/Source/core/frame/RemoteFrame.cpp:204: ...
3 years, 1 month ago (2016-07-11 19:39:30 UTC) #8
dcheng
On 2016/07/11 19:39:30, esprehn wrote: > Hmm, what about inside updateStyle when we unload plugins? ...
3 years, 1 month ago (2016-07-12 01:14:41 UTC) #9
esprehn
https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode455 third_party/WebKit/Source/core/frame/LocalFrame.cpp:455: if (document()) if (Document* document = this->document()) avoids the ...
3 years, 1 month ago (2016-07-13 02:35:46 UTC) #10
esprehn
lgtm, though I think long term perhaps we should centralize this logic in one place. ...
3 years, 1 month ago (2016-07-13 02:38:43 UTC) #11
dcheng
https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode455 third_party/WebKit/Source/core/frame/LocalFrame.cpp:455: if (document()) On 2016/07/13 02:35:45, esprehn wrote: > if ...
3 years, 1 month ago (2016-07-13 02:51:34 UTC) #12
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/2134113002/60001
3 years, 1 month ago (2016-07-13 02:54:03 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/34832) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 1 month ago (2016-07-13 02:58:42 UTC) #17
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/2134113002/80001
3 years, 1 month ago (2016-07-13 03:22:46 UTC) #20
commit-bot: I haz the power
Committed patchset #5 (id:80001)
3 years, 1 month ago (2016-07-13 05:16:10 UTC) #21
commit-bot: I haz the power
CQ bit was unchecked.
3 years, 1 month ago (2016-07-13 05:16:15 UTC) #22
commit-bot: I haz the power
3 years, 1 month ago (2016-07-13 05:17:16 UTC) #24
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0
Cr-Commit-Position: refs/heads/master@{#405040}

Powered by Google App Engine
This is Rietveld 408576698