|
|
Created:
4 years, 3 months ago by bokan Modified:
4 years, 3 months ago Reviewers:
tdresser CC:
chromium-reviews, kenneth.christiansen, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@splitRootScrollerController Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake document.rootScroller work properly across iframes.
This patch makes rootScroller work across iframes in a composable way. The main
thrust is that the RootScrollerController class is aware only of local issues.
Its m_rootScroller and m_effectiveRootScroller members always point to Elements
in the associated Document. The TopDocumentRootScrollerController is responsible
for page-level issues. Including determination and management of the "global"
rootScroller. The "global" root scroller is the Element whose scrolling actually
affects the top controls and produces overscroll effects.
Now, root scrollers chain across iframes. To calculate the global root scroller,
we find the effective root scroller of the root frame. If that's an iframe, we
then descend into the iframe's Document and use its effective root scroller. If
that's an iframe, we descend again, and so on.
BUG=505516
Committed: https://crrev.com/242c9fb14efa1822a64fcf43cda2c2edef71e406
Cr-Commit-Position: refs/heads/master@{#415081}
Patch Set 1 #Patch Set 2 : Top Controls Work From Iframes #
Total comments: 23
Patch Set 3 : Addressed Tim's comments #Patch Set 4 : Rebase #
Total comments: 1
Patch Set 5 : Rebase + remove NOTREACHED in RSC::globalRootScrollerMayHaveChanged #Patch Set 6 : ASSERT->EXPECT in tests #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG= ========== to ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG=505516 ==========
Description was changed from ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, setting an iframe as the rootScroller of the main document means that we'll use the iframe's effective root scroller as the global root scroller rather than the iframe itself. BUG=505516 ========== to ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, root scrollers chain across iframes. To calculate the global root scroller, we find the effective root scroller of the root frame. If that's an iframe, we then descend into the iframe's Document and use its effective root scroller. If that's an iframe, we descend again, and so on. BUG=505516 ==========
bokan@chromium.org changed reviewers: + tdresser@chromium.org
This makes rootScroller work properly (as I think) across iframes. There's still compositing issues that I've solved but split into a separate CL so this patch won't expand the clip when top controls hide but with this you can set an iframe (and then elements in the iframe) as the root scroller and it'll hide the top controls. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:596: // compositing changes. crbug.com/505516 I thought of this so I added the TODO so I don't forget. My next patch will deal with compositing issues so I'll fill this in there.
LGTM. I'm OOO tomorrow, if you prefer not to move those methods to the TopDocumentRootScrollerController, that's fine with me. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:150: GraphicsLayer* RootScrollerController::rootScrollerLayer() Move to TopDocumentRootScrollerController only? https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:95: // Called when the root scroller of descendant frames changes. This will only Called when the root scroller of _any_ descendant ... https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:97: virtual void globalRootScrollerMayHaveChanged(); If it will only ever be called on the top document's RootScrollerController, why isn't it defined on TopDocumentRootScrollerController? I guess it would require a static_cast, or adding a redundant attribute. I think I'd prefer defining this on the TopDocumentRootScrollerController though. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:109: virtual void updateEffectiveRootScroller(); Maybe "forceValid" instead of "update"? https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:54: return nullptr; When would this happen? My intuition is that we should return element here, not nullptr. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h:28: // TODO(bokan): This class is currently OOPIF unawares. It should be broken into unaware https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:468: ASSERT_EQ(nullptr, mainFrame()->document()->rootScroller()); Throughput this file, we're ASSERT'ing when we should be EXPECT'ing. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:555: mainFrame()->document()->documentElement()->getApplyScroll())); This is redundant with the ASSERT below, isn't it? https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:571: mainFrame()->document()->documentElement()->getApplyScroll())); Aren't the two ASSERTs above unneeded?
https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:150: GraphicsLayer* RootScrollerController::rootScrollerLayer() On 2016/08/25 19:46:37, tdresser wrote: > Move to TopDocumentRootScrollerController only? Will do in follow up. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:95: // Called when the root scroller of descendant frames changes. This will only On 2016/08/25 19:46:37, tdresser wrote: > Called when the root scroller of _any_ descendant ... Done. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:97: virtual void globalRootScrollerMayHaveChanged(); On 2016/08/25 19:46:37, tdresser wrote: > If it will only ever be called on the top document's RootScrollerController, why > isn't it defined on TopDocumentRootScrollerController? I guess it would require > a static_cast, or adding a redundant attribute. > > I think I'd prefer defining this on the TopDocumentRootScrollerController > though. This is why I realized it'd probably work better to make TopDocumentRootScrollerController a separate object. I'll move this and the other methods that really belong there in a followup patch. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h:109: virtual void updateEffectiveRootScroller(); On 2016/08/25 19:46:37, tdresser wrote: > Maybe "forceValid" instead of "update"? Changed to recomputeEffectiveRootScroller since I think it captures the spirit best. Also removed 'virtual' since it's not needed. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:54: return nullptr; On 2016/08/25 19:46:37, tdresser wrote: > When would this happen? My intuition is that we should return element here, not > nullptr. I suspect during initialization/finalization. I think you're right on the return value though. Done. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h:28: // TODO(bokan): This class is currently OOPIF unawares. It should be broken into On 2016/08/25 19:46:37, tdresser wrote: > unaware Done. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:468: ASSERT_EQ(nullptr, mainFrame()->document()->rootScroller()); On 2016/08/25 19:46:37, tdresser wrote: > Throughput this file, we're ASSERT'ing when we should be EXPECT'ing. ASSERTs will stop the test when they fail whereas EXPECTs won't. I think in all these cases, if the ASSERT fails, our assumptions following it are violated and so they'll fail trivially which will just add noise to the failure and make it harder to track down. I'll leave it as is, if you disagree I can change it later/we can discuss. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:555: mainFrame()->document()->documentElement()->getApplyScroll())); On 2016/08/25 19:46:37, tdresser wrote: > This is redundant with the ASSERT below, isn't it? If everything's working correctly but it's a bit of a sanity check. i.e. Make sure the apply scroll isn't set on both Elements. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:571: mainFrame()->document()->documentElement()->getApplyScroll())); On 2016/08/25 19:46:37, tdresser wrote: > Aren't the two ASSERTs above unneeded? Same as above, just making sure it's removed from the old Elements..
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hey Tim, didn't land this yes, still-l g t m? https://codereview.chromium.org/2281603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:88: NOTREACHED(); This gets tripped in tests that use a remote main frame so I had to remove it. None of this works for OOPIF at the moment anyway...
Still LGTM! https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:468: ASSERT_EQ(nullptr, mainFrame()->document()->rootScroller()); On 2016/08/26 19:35:05, bokan wrote: > On 2016/08/25 19:46:37, tdresser wrote: > > Throughput this file, we're ASSERT'ing when we should be EXPECT'ing. > > ASSERTs will stop the test when they fail whereas EXPECTs won't. I think in all > these cases, if the ASSERT fails, our assumptions following it are violated and > so they'll fail trivially which will just add noise to the failure and make it > harder to track down. I'll leave it as is, if you disagree I can change it > later/we can discuss. I think there are lots of cases here where seeing multiple expectation failures at once would be better here, but I don't feel strongly about it. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:555: mainFrame()->document()->documentElement()->getApplyScroll())); On 2016/08/26 19:35:05, bokan wrote: > On 2016/08/25 19:46:37, tdresser wrote: > > This is redundant with the ASSERT below, isn't it? > > If everything's working correctly but it's a bit of a sanity check. i.e. Make > sure the apply scroll isn't set on both Elements. Acknowledged. https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:571: mainFrame()->document()->documentElement()->getApplyScroll())); On 2016/08/26 19:35:05, bokan wrote: > On 2016/08/25 19:46:37, tdresser wrote: > > Aren't the two ASSERTs above unneeded? > > Same as above, just making sure it's removed from the old Elements.. Acknowledged.
https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:468: ASSERT_EQ(nullptr, mainFrame()->document()->rootScroller()); On 2016/08/29 15:07:50, tdresser wrote: > On 2016/08/26 19:35:05, bokan wrote: > > On 2016/08/25 19:46:37, tdresser wrote: > > > Throughput this file, we're ASSERT'ing when we should be EXPECT'ing. > > > > ASSERTs will stop the test when they fail whereas EXPECTs won't. I think in > all > > these cases, if the ASSERT fails, our assumptions following it are violated > and > > so they'll fail trivially which will just add noise to the failure and make it > > harder to track down. I'll leave it as is, if you disagree I can change it > > later/we can discuss. > > I think there are lots of cases here where seeing multiple expectation failures > at once would be better here, but I don't feel strongly about it. Personally, I find multiple failures to be distracting but that's a personal preference and EXPECT is more common so I've changed most checks in this file to EXPECT.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2281603002/#ps100001 (title: "ASSERT->EXPECT in tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, root scrollers chain across iframes. To calculate the global root scroller, we find the effective root scroller of the root frame. If that's an iframe, we then descend into the iframe's Document and use its effective root scroller. If that's an iframe, we descend again, and so on. BUG=505516 ========== to ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, root scrollers chain across iframes. To calculate the global root scroller, we find the effective root scroller of the root frame. If that's an iframe, we then descend into the iframe's Document and use its effective root scroller. If that's an iframe, we descend again, and so on. BUG=505516 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, root scrollers chain across iframes. To calculate the global root scroller, we find the effective root scroller of the root frame. If that's an iframe, we then descend into the iframe's Document and use its effective root scroller. If that's an iframe, we descend again, and so on. BUG=505516 ========== to ========== Make document.rootScroller work properly across iframes. This patch makes rootScroller work across iframes in a composable way. The main thrust is that the RootScrollerController class is aware only of local issues. Its m_rootScroller and m_effectiveRootScroller members always point to Elements in the associated Document. The TopDocumentRootScrollerController is responsible for page-level issues. Including determination and management of the "global" rootScroller. The "global" root scroller is the Element whose scrolling actually affects the top controls and produces overscroll effects. Now, root scrollers chain across iframes. To calculate the global root scroller, we find the effective root scroller of the root frame. If that's an iframe, we then descend into the iframe's Document and use its effective root scroller. If that's an iframe, we descend again, and so on. BUG=505516 Committed: https://crrev.com/242c9fb14efa1822a64fcf43cda2c2edef71e406 Cr-Commit-Position: refs/heads/master@{#415081} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/242c9fb14efa1822a64fcf43cda2c2edef71e406 Cr-Commit-Position: refs/heads/master@{#415081} |