New issue
Advanced search Search tips

Issue 620721 link

Starred by 3 users
Status: Fixed
Owner:
bokan@chromium.org
Closed: Dec 2017
Cc:
tdres...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Remove main thread scroll code duplication

Project Member Reported by bokan@chromium.org, Jun 16 2016 
Currently, main thread (gesture) scrolling code diverges based on whether ScrollCustomization is enabled or not. While ScrollCustomization may or may not ship in the distant future, we can remove the duplication by basing all our scrolling internally on ScrollCustomization. This has already been started with the RootScroller work which scrolls the main frame using ScrollCustomization primitives.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 24 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98

commit c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98
Author: bokan <bokan@chromium.org>
Date: Fri Jun 24 22:45:12 2016

Make all gesture scrolls use customization path internally

This patch removes the physicalScroll code path in ScrollManager.
This means that all gesture scrolls are now taking the ScrollCustomization
path through scrolling; building a scroll chain, calling distribute and
apply scroll.

Some notable changes here:

iframes now also scroll like the main frame, using a ViewportScrollCallback
but without affecting top controls or overscroll.

The ScrollChain calculated in ScrollCustomization used to end on the
scrollingElement. It now terminates at the RootScroller. This is so that
1) the RootScroller API works as expected when the page sets a root
scroller and 2) if the scrollingElement is <body>, we still want to chain
the scroll up to the documentElement (assuming it's the root scroller)
since that'll actually scroll the frame and top controls.

Clear m_deltaConsumedForScrollSequence on GestureScrollBegin. Otherwise,
this doesn't get cleared during a fling gesture so a new scroll during
the fling will get dropped in the ScrollCustomization path.

BUG= 620721 

Review-Url: https://codereview.chromium.org/2069713002
Cr-Commit-Position: refs/heads/master@{#401997}

[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/dom/Node.h
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/input/ScrollManager.cpp
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/input/ScrollManager.h
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.cpp
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.h
[modify] https://crrev.com/c81f2158da2b43ae5a8b0c5d0e0fcf3b5d2c8b98/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp

Comment 2 by nzolghadr@chromium.org, Dec 14 2017 

bokan@ is the work for this issue finished?

Comment 3 by bokan@chromium.org, Dec 14 2017

Status: Fixed (was: Started)
Not quite, there's still paths in ScrollManager::BubblingScroll and the like that don't use scroll customization. But, I think that work can be better described as "make all scrolls use gestures" (and this isn't as critical from a code health perspective as the wheel and touch paths which have been fixed) so lets close this in favor of that work (which is unstarted but tracked in our OKRs).

Comment 4 by bokan@chromium.org, Dec 14 2017 

Actually, sahel@ is currently working on that, tracked from issue 125223
►

Sign in to add a comment