New issue
Advanced search Search tips

Issue 591124 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 476553



Sign in to add a comment

Clean up main-thread scrolling paths

Project Member Reported by bokan@chromium.org, Mar 1 2016

Issue description

There's various refactorings we should do here:

-Removing the one-dimensional scroll methods that take a direction and delta rather than a FloatSize
-Seperating ScrollAnimators/ScrollableAreas/EventHandler more cleanly. Currently, details of EventHandler like ScrollGranularity, Direction, etc. are passed all the way through to the Animators even though they're not really needed there.
-Making frame scrolling less special compared to scrolling other LayoutBoxes
-Merging various similar scrolling paths to reduce complexity and bugs

This will all be quite helpful once we start undertaking the scroll unification project. Making the main thread paths look more like the CC equivalents will make that easier.
 

Comment 1 by bokan@chromium.org, Mar 1 2016

Already landed:

http://crrev.com/1741573002 Remove step parameter from ScrollAnimator::userScroll
http://crrev.com/1708393002 Removed m_previousWheelScrolledNode from EventHandler
http://crrev.com/1724353002 Fix main thread overscroll glow when scrolling iframes.
I have noticed in the version 46 and above especially when using the grid some of the rows having scroll does not render only after scrolling 2-3 pixels it is displayed.
Is this bug related to the same.

Comment 3 by bokan@chromium.org, Mar 1 2016

Re #2, no, this "bug" is not a problem with chrome but a code health issue.

If you believe you're seeing a bug please file a new bug with steps to reproduce (feel free to CC me and I'll make sure it ends up in the right place).
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 2 2016

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

commit f0713e7d9da9b89301ae0ae1228d51fbb2e17afc
Author: bokan <bokan@chromium.org>
Date: Wed Mar 02 01:14:36 2016

Removed main-thread one dimensional scrolling paths.

This patch is a large cleanup of the main-thread scrolling paths in Blink.
The main change is the removal of most single dimension scrolling paths.
These paths, going from EventHandler, through ScrollableAreas, and into
ScrollAnimators used a single float and a ScrollDirection to determine
how to scroll. This style of scrolling is needed only for a few cases:
scrollbar and keyboard scrolling. The fact that Animators and
ScrollableAreas work naturally in two dimensions complicated their interface
and implementations needlessly. Implementing Wheel and Gesture scrolling
required awkward 2-phase scrolls and coordination between them.

I've completely replaced the one dimensional interfaces in ScrollableArea
and ScrollAnimators with a more standard 2d FloatSize. It's now
EventHandler's job, where needed, to translate a direction/float instruction
to scroll into the necessary physical scroll used by ScrollableArea. This
should make future work and potential CC scroll unification work more
straightforward.

Some other minor cleanups along the way:

-ScrollAnimator and its children use a FloatPoint rather than two floats for
current position.
-Split EventHandler::scroll into logical and physical. Using it for both
made the interface and usage complicated.
-Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG= 591124 

Review URL: https://codereview.chromium.org/1738243002

Cr-Commit-Position: refs/heads/master@{#378642}

[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/frame/RootFrameViewport.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/frame/RootFrameViewportTest.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/input/EventHandler.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/layout/LayoutEmbeddedObject.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/layout/LayoutEmbeddedObject.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/core/layout/api/LayoutBoxItem.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollAnimator.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollTypes.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/ScrollableArea.h
[modify] https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc/third_party/WebKit/Source/platform/scroll/Scrollbar.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 2 2016

Labels: Merge-Merged-master1
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/bling/chromium.git/+/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc

commit f0713e7d9da9b89301ae0ae1228d51fbb2e17afc
Author: bokan <bokan@chromium.org>
Date: Wed Mar 02 01:14:36 2016

Hi Bokan,

I created the issue.
Issue no is Issue 591270
<https://bugs.chromium.org/p/chromium/issues/detail?id=591270>

Thank you.

Mangesh

Regards,
Mangesh Pimpalkar

On Tue, Mar 1, 2016 at 2:13 PM, bokan@chromium.org via Monorail <
monorail@chromium.org> wrote:
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 2 2016

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

commit 1535512d6b044044e58111011a48ffd07194b49c
Author: bokan <bokan@chromium.org>
Date: Wed Mar 02 21:18:16 2016

Pass the physical scroll delta through the scroll customization path.

The scroll customization path of scrolling passes through the gesture event
deltas which are inverted from actual scroll deltas. E.g. A finger moving
up and left has positive gesture event delta but causes the page to scroll down
and right which increases the page's scrollX and scrollY.

This patch converts the event delta to a scroll delta before feeding it into the
scroll customization path. I've also "uninverted" some of the helper methods
that were relying on this like LocalFrame::applyScrollDelta and
TopControls::scrollBy.

BUG= 591124 

Review URL: https://codereview.chromium.org/1755773002

Cr-Commit-Position: refs/heads/master@{#378834}

[modify] https://crrev.com/1535512d6b044044e58111011a48ffd07194b49c/third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
[modify] https://crrev.com/1535512d6b044044e58111011a48ffd07194b49c/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/1535512d6b044044e58111011a48ffd07194b49c/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/1535512d6b044044e58111011a48ffd07194b49c/third_party/WebKit/Source/core/frame/TopControls.cpp
[modify] https://crrev.com/1535512d6b044044e58111011a48ffd07194b49c/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/1535512d6b044044e58111011a48ffd07194b49c/third_party/WebKit/Source/web/tests/TopControlsTest.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 10 2016

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

commit fd7ede807eb0e64820c805e8d5d0caf7ba9c5516
Author: bokan <bokan@chromium.org>
Date: Thu Mar 10 14:54:35 2016

Merged FrameView and LayoutBox scrolling in EventHandler.

Scrolling a FrameView (window and iframes) has some small differences from
scrolling a normal LayoutBox. Because of this, we've generally forked all our
scrolling code to take two different paths based on whether a LayoutBox is a
FrameView/LayoutView. This has caused pain in that much logic has been
duplicated between the two.

This patch removes the duplication in the calling code and makes the decision
based on the virtual LayoutBox::scroll function.

BUG= 591124 

Review URL: https://codereview.chromium.org/1752043002

Cr-Commit-Position: refs/heads/master@{#380403}

[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/LayoutTests/fast/events/platform-wheelevent-paging-x-in-scrolling-div.html
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-div.html
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/LayoutTests/fast/events/platform-wheelevent-paging-y-in-scrolling-div.html
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-page-zoomed.html
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-page.html
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/core/input/EventHandler.h
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/core/layout/LayoutView.h
[modify] https://crrev.com/fd7ede807eb0e64820c805e8d5d0caf7ba9c5516/third_party/WebKit/Source/web/tests/TopControlsTest.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 27 2016

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

commit 33a081977fa4f03d96265f6ada2d3347d9f0db46
Author: bokan <bokan@chromium.org>
Date: Wed Apr 27 19:39:22 2016

Move overscroll logic out of EventHandler.

Overscrolling now happens from the viewport apply scroll callback so it doesn't
have much connection to EventHandler. I've moved all the overscrolling logic
into a new OverscrollController class, owned by FrameHost, that handles related
state and forwards the calls to the content layer.

This patch should have no behavior changes.

BUG= 591124 

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

[modify] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/frame/FrameHost.cpp
[modify] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/frame/FrameHost.h
[modify] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/input/EventHandler.h
[add] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/page/scrolling/OverscrollController.cpp
[add] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/page/scrolling/OverscrollController.h
[modify] https://crrev.com/33a081977fa4f03d96265f6ada2d3347d9f0db46/third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.cpp

Status: Fixed (was: Started)

Sign in to add a comment