Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1316863009:
OOPIF: Call setCoreFrame on remote-to-local swaps. (Closed)
DescriptionOOPIF: Call setCoreFrame on remote-to-local swaps.
This ensures that various initializion in setCoreFrame is triggered when
doing remote-to-local swaps.
BUG=525285
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201726
Patch Set 1 #Patch Set 2 : Add test #Patch Set 3 : #
Total comments: 7
Patch Set 4 : Add comment to put the ASSERT back in. #
Messages
Total messages: 13 (3 generated)
alexmos@chromium.org changed reviewers: + dcheng@chromium.org
Daniel - does this look reasonable? https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:2011: setCoreFrame(frame); I wanted to do this after setting up the tree relationships, since this calls into code that may check them (for example, GeolocationController::GeolocationController checks isMainFrame()). Hence the removal of the ASSERT in setOpener, which will be called with a null m_frame here (I think not calling initSecurityContext there is actually better - probably no point in doing it before we init the frame).
https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:2011: setCoreFrame(frame); On 2015/09/03 at 16:33:55, alexmos wrote: > I wanted to do this after setting up the tree relationships, since this calls into code that may check them (for example, GeolocationController::GeolocationController checks isMainFrame()). Hence the removal of the ASSERT in setOpener, which will be called with a null m_frame here (I think not calling initSecurityContext there is actually better - probably no point in doing it before we init the frame). Which assert is this that you're referring to? I don't seem to see it in this patch.
https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (left): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:750: ASSERT(m_frame); This is the ASSERT. https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:2011: setCoreFrame(frame); On 2015/09/03 16:42:09, dcheng wrote: > On 2015/09/03 at 16:33:55, alexmos wrote: > > I wanted to do this after setting up the tree relationships, since this calls > into code that may check them (for example, > GeolocationController::GeolocationController checks isMainFrame()). Hence the > removal of the ASSERT in setOpener, which will be called with a null m_frame > here (I think not calling initSecurityContext there is actually better - > probably no point in doing it before we init the frame). > > Which assert is this that you're referring to? I don't seem to see it in this > patch. The one I removed above in WebLocalFrameImpl::setOpener
lgtm https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:2011: setCoreFrame(frame); On 2015/09/03 at 16:43:38, alexmos wrote: > On 2015/09/03 16:42:09, dcheng wrote: > > On 2015/09/03 at 16:33:55, alexmos wrote: > > > I wanted to do this after setting up the tree relationships, since this calls > > into code that may check them (for example, > > GeolocationController::GeolocationController checks isMainFrame()). Hence the > > removal of the ASSERT in setOpener, which will be called with a null m_frame > > here (I think not calling initSecurityContext there is actually better - > > probably no point in doing it before we init the frame). > > > > Which assert is this that you're referring to? I don't seem to see it in this > > patch. > > The one I removed above in WebLocalFrameImpl::setOpener Err, oops. I think it's a /little/ weird to remove the assert, but I'm willing to live with it for now: once we don't have provisional local frames anymore, we should restore the assert though.
https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (left): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:750: ASSERT(m_frame); On 2015/09/03 at 16:43:38, alexmos wrote: > This is the ASSERT. (Add a TODO to add this assert back once we don't have provisional local frames?)
https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... File Source/web/WebLocalFrameImpl.cpp (left): https://codereview.chromium.org/1316863009/diff/40001/Source/web/WebLocalFram... Source/web/WebLocalFrameImpl.cpp:750: ASSERT(m_frame); On 2015/09/03 16:51:43, dcheng wrote: > On 2015/09/03 at 16:43:38, alexmos wrote: > > This is the ASSERT. > > (Add a TODO to add this assert back once we don't have provisional local > frames?) Done.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1316863009/#ps60001 (title: "Add comment to put the ASSERT back in.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316863009/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316863009/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201726
Message was sent while issue was closed.
On 2015/09/03 17:51:23, commit-bot: I haz the power wrote: > Committed patchset #4 (id:60001) as > https://src.chromium.org/viewvc/blink?view=rev&revision=201726 I suspect this change or https://codereview.chromium.org/1315523010 broke webkit_unit_tests on Nexus 4: All/ParameterizedWebFrameTest.RemoteToLocalSwapOnMainFrameInitializesCoreFrame/0 All/ParameterizedWebFrameTest.RemoteToLocalSwapOnMainFrameInitializesCoreFrame/1 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForFixedWidth/0 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForFixedWidth/1 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForMinimumScale/0 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForWidthEqualsDeviceWidth/1 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForFixedLayout/1 First failing build: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N... Blamelist: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog_blink.html?u... http://test-results.appspot.com/revision_range?start=347178&end=347178
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1315543009/ by falken@chromium.org. The reason for reverting is: I suspect this change broke webkit_unit_tests on Nexus 4: All/ParameterizedWebFrameTest.RemoteToLocalSwapOnMainFrameInitializesCoreFrame/0 All/ParameterizedWebFrameTest.RemoteToLocalSwapOnMainFrameInitializesCoreFrame/1 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForFixedWidth/0 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForFixedWidth/1 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForMinimumScale/0 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForWidthEqualsDeviceWidth/1 All/WebFrameResizeTest.ResizeYieldsCorrectScrollAndScaleForFixedLayout/1 First failing build: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N... Blamelist: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog_blink.html?u... http://test-results.appspot.com/revision_range?start=347178&end=347178. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
