Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(904)

Issue 2281603002: Make document.rootScroller work properly across iframes. (Closed)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+290 lines, -100 lines) Patch
M third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h View 1 2 5 chunks +20 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp View 1 2 3 4 4 chunks +12 lines, -18 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h View 1 2 3 4 2 chunks +27 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp View 1 2 4 chunks +53 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/web/tests/RootScrollerTest.cpp View 1 2 3 4 5 17 chunks +178 lines, -40 lines 0 comments Download

Messages

Total messages: 22 (13 generated)
bokan
This makes rootScroller work properly (as I think) across iframes. There's still compositing issues that ...
4 years, 3 months ago (2016-08-25 17:25:56 UTC) #6
tdresser
LGTM. I'm OOO tomorrow, if you prefer not to move those methods to the TopDocumentRootScrollerController, ...
4 years, 3 months ago (2016-08-25 19:46:38 UTC) #7
bokan
https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp#newcode150 third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:150: GraphicsLayer* RootScrollerController::rootScrollerLayer() On 2016/08/25 19:46:37, tdresser wrote: > Move ...
4 years, 3 months ago (2016-08-26 19:35:05 UTC) #8
bokan
Hey Tim, didn't land this yes, still-l g t m? https://codereview.chromium.org/2281603002/diff/60001/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp (right): https://codereview.chromium.org/2281603002/diff/60001/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp#newcode88 ...
4 years, 3 months ago (2016-08-29 13:27:10 UTC) #13
tdresser
Still LGTM! https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp#newcode468 third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:468: ASSERT_EQ(nullptr, mainFrame()->document()->rootScroller()); On 2016/08/26 19:35:05, bokan wrote: ...
4 years, 3 months ago (2016-08-29 15:07:51 UTC) #14
bokan
https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp File third_party/WebKit/Source/web/tests/RootScrollerTest.cpp (right): https://codereview.chromium.org/2281603002/diff/20001/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp#newcode468 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 ...
4 years, 3 months ago (2016-08-29 17:04:36 UTC) #15
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/2281603002/100001
4 years, 3 months ago (2016-08-29 17:16:00 UTC) #18
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 3 months ago (2016-08-30 04:15:08 UTC) #20
commit-bot: I haz the power
4 years, 3 months ago (2016-08-30 04:18:44 UTC) #22
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/242c9fb14efa1822a64fcf43cda2c2edef71e406
Cr-Commit-Position: refs/heads/master@{#415081}

Powered by Google App Engine
This is Rietveld 408576698