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

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

Created:
2 years, 10 months ago by dcheng
Modified:
2 years, 10 months 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 ...
2 years, 10 months 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: ...
2 years, 10 months 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? ...
2 years, 10 months 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 ...
2 years, 10 months 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. ...
2 years, 10 months 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 ...
2 years, 10 months 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
2 years, 10 months 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, ...
2 years, 10 months 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
2 years, 10 months ago (2016-07-13 03:22:46 UTC) #20
commit-bot: I haz the power
Committed patchset #5 (id:80001)
2 years, 10 months ago (2016-07-13 05:16:10 UTC) #21
commit-bot: I haz the power
CQ bit was unchecked.
2 years, 10 months ago (2016-07-13 05:16:15 UTC) #22
commit-bot: I haz the power
2 years, 10 months 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