Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750613002:
Retire WebLocalFrameScope. (Closed)
DescriptionRetire WebLocalFrameScope.
Retire this test-supporting scope object; no longer needed to ensure timely
release and closing of WebLocalFrames in CreateLocalChildWithPreviousSibling
R=dcheng
BUG=
Committed: https://crrev.com/f3e3dcffa4704e72b4c3e34d2f10d4f49b097375
Cr-Commit-Position: refs/heads/master@{#378454}
Patch Set 1 #Patch Set 2 : remove WebLocalFrameScope #Messages
Total messages: 22 (10 generated)
sigbjornf@opera.com changed reviewers: + dcheng@chromium.org, haraken@chromium.org
please take a look. address fails like https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp...
On 2016/02/29 at 18:30:08, sigbjornf wrote: > please take a look. > > address fails like https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp... Hmm, I don't think I quite understand the failure mode: WebViewImpl::close() should detach the frames recursively, right? And the testing WebLocalFrames should have a WebFrameClient that closes them by default on detach? So why do we need WebLocalFrameScope at all?
On 2016/02/29 18:33:22, dcheng wrote: > On 2016/02/29 at 18:30:08, sigbjornf wrote: > > please take a look. > > > > address fails like > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp... > > Hmm, I don't think I quite understand the failure mode: > WebViewImpl::close() should detach the frames recursively, right? And the > testing WebLocalFrames should have a WebFrameClient that closes them by default > on detach? So why do we need WebLocalFrameScope at all? Let me reflect that question back to you as you reviewed https://codereview.chromium.org/1635873003 which altered this test; it seems like you're in a better position to answer that.
On 2016/02/29 at 21:12:26, sigbjornf wrote: > On 2016/02/29 18:33:22, dcheng wrote: > > On 2016/02/29 at 18:30:08, sigbjornf wrote: > > > please take a look. > > > > > > address fails like > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp... > > > > Hmm, I don't think I quite understand the failure mode: > > WebViewImpl::close() should detach the frames recursively, right? And the > > testing WebLocalFrames should have a WebFrameClient that closes them by default > > on detach? So why do we need WebLocalFrameScope at all? > > Let me reflect that question back to you as you reviewed https://codereview.chromium.org/1635873003 which altered this test; it seems like you're in a better position to answer that. That was just a refactoring patch which added a parameter. What I'm wondering is: why do we need WebLocalFrameScope at all? Can we just do without those? In theory, the frames should be owned by the frame tree, so we should rarely need to manually release like this.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750613002/20001
On 2016/02/29 21:24:30, dcheng wrote: > On 2016/02/29 at 21:12:26, sigbjornf wrote: > > On 2016/02/29 18:33:22, dcheng wrote: > > > On 2016/02/29 at 18:30:08, sigbjornf wrote: > > > > please take a look. > > > > > > > > address fails like > > > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20non-Oilp... > > > > > > Hmm, I don't think I quite understand the failure mode: > > > WebViewImpl::close() should detach the frames recursively, right? And the > > > testing WebLocalFrames should have a WebFrameClient that closes them by > default > > > on detach? So why do we need WebLocalFrameScope at all? > > > > Let me reflect that question back to you as you reviewed > https://codereview.chromium.org/1635873003 which altered this test; it seems > like you're in a better position to answer that. > > That was just a refactoring patch which added a parameter. > > What I'm wondering is: why do we need WebLocalFrameScope at all? Can we just do > without those? In theory, the frames should be owned by the frame tree, so we > should rarely need to manually release like this. A head scratcher this one -- I don't understand what broke & when w/ non-Oilpan here, but calling close() on the WebView will now tear down the frame tree as expected and wanted. With and without Oilpan -- WebLocalFrameScope was introduced to work around some unit tests leaks in https://codereview.chromium.org/1273023002. Removed WebLocalFrameScope & verified not to leak (or fail) with either build config.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Fix CreateLocalChildWithPreviousSibling unit test shutdown, non-Oilpan. Arrange for the WebLocalFrames to be closed before the view is, as destruction is synchronous without Oilpan. R= BUG= ========== to ========== Fix CreateLocalChildWithPreviousSibling unit test shutdown, non-Oilpan. Arrange for the WebLocalFrames to be closed before the view is, as destruction is synchronous without Oilpan. R=dcheng BUG= ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750613002/20001
The CQ bit was unchecked by sigbjornf@opera.com
Description was changed from ========== Fix CreateLocalChildWithPreviousSibling unit test shutdown, non-Oilpan. Arrange for the WebLocalFrames to be closed before the view is, as destruction is synchronous without Oilpan. R=dcheng BUG= ========== to ========== Retire WebLocalFrameScope. Retire this test-supporting scope object; no longer needed to ensure timely release and closing of WebLocalFrames in CreateLocalChildWithPreviousSibling R=dcheng BUG= ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750613002/20001
Message was sent while issue was closed.
Description was changed from ========== Retire WebLocalFrameScope. Retire this test-supporting scope object; no longer needed to ensure timely release and closing of WebLocalFrames in CreateLocalChildWithPreviousSibling R=dcheng BUG= ========== to ========== Retire WebLocalFrameScope. Retire this test-supporting scope object; no longer needed to ensure timely release and closing of WebLocalFrames in CreateLocalChildWithPreviousSibling R=dcheng BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Retire WebLocalFrameScope. Retire this test-supporting scope object; no longer needed to ensure timely release and closing of WebLocalFrames in CreateLocalChildWithPreviousSibling R=dcheng BUG= ========== to ========== Retire WebLocalFrameScope. Retire this test-supporting scope object; no longer needed to ensure timely release and closing of WebLocalFrames in CreateLocalChildWithPreviousSibling R=dcheng BUG= Committed: https://crrev.com/f3e3dcffa4704e72b4c3e34d2f10d4f49b097375 Cr-Commit-Position: refs/heads/master@{#378454} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f3e3dcffa4704e72b4c3e34d2f10d4f49b097375 Cr-Commit-Position: refs/heads/master@{#378454} |
