Removed main-thread one dimensional scrolling paths.

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
Committed: https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc
Cr-Commit-Position: refs/heads/master@{#378642}

[bokan, 2016-02-26 19:38:14]

Description was changed from

==========
Removed one dimensional scrolling paths.

BUG=
==========

to

==========
Removed 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.

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 direction used by
ScrollableArea.

Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG=
==========

[bokan, 2016-02-26 19:38:37]

Description was changed from

==========
Removed 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.

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 direction used by
ScrollableArea.

Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG=
==========

to

==========
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.

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 direction used by
ScrollableArea.

Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG=
==========

[bokan, 2016-02-26 19:39:08]

Description was changed from

==========
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.

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 direction used by
ScrollableArea.

Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG=
==========

to

==========
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.

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 direction used by
ScrollableArea. This should make future work and potential CC scroll
unification work more straightforward.

Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG=
==========

[bokan, 2016-02-26 19:49:16]

Description was changed from

==========
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.

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 direction used by
ScrollableArea. This should make future work and potential CC scroll
unification work more straightforward.

Additionally, I've littered a number of TODOs throughout these paths where
I found issues and inconsistencies to clean up in future CLs.

BUG=
==========

to

==========
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=
==========

[bokan, 2016-02-26 19:49:16]

bokan@chromium.org changed reviewers:
+ skobes@chromium.org, tdresser@chromium.org

[bokan, 2016-02-26 19:50:18]

+Tim for EventHandler and friends
+Steve for ScrollableArea/ScrollAnimators

[bokan, 2016-02-26 19:50:26]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-26 19:53:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738243002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738243002/1

[commit-bot: I haz the power, 2016-02-26 20:57:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 20:57:10]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tdresser, 2016-02-29 14:53:43]

Thanks, this looks like a nice cleanup.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:797: // positive if the user
scrolls up. For scrolling, a positive delta implies
positive if the user scrolls up or left

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:238: // result of the
smooth scrolling work.
I may just be lacking context, but it isn't clear to me what this comment is
referring to.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:251:
pixelDelta.setWidth(0);
Can we avoid calling scrollStep in cases where we're going to ignore its output?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:258: return
visualViewport().userScroll(granularity, delta);
If one axis didn't scroll, but the other did, this will cause a change in
behavior, won't it? Shouldn't we allow scrolling the layout viewport on one
axis, but the visual viewport on the other?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:276: ScrollResult
layoutResult = layoutViewport().scrollAnimator().userScroll(granularity,
pixelDelta - visualUsedDelta);
I'm a bit confused about how this works. If I understand correctly:
1. If the layout viewport is user scrollable, scroll it.
2. If the layout viewport didn't scroll, try to scroll the visual viewport.
3. If the visual viewport didn't consume all the scroll, scroll the layout
viewport.

Why would we ever do anything in step #3? Is it for cases where the layout
viewport isn't user scrollable?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:613:
m_frame->document()->updateLayoutIgnorePendingStylesheets();
Why was this not needed before, but needed now?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:619: // If we're at the
stopNode, we should try to scroll it but we shouldn't bubble past it
Missing period.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:630: // on the result
instead rather than lying to the caller.
instead rather -> rather

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:268: // Performs a bubbling
scroll, within a *single* frame, starting from a
I'd prefer if we used the terms "bubble" and "chain" to refer to event bubbling,
and scroll chaining respectively. It's confusing that we use the word bubble to
refer both to scrolling and to event dispatch.

I don't know if it's worth renaming "bubblingScroll", but it would be nice if we
could use the word "chain" in comments.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:282: // from either a
provided starting node or a default based on focus/clicked.
"based on focus/clicked"

Can this be a bit more precise? "based on the focused or most recently clicked
Node"? or something along those lines?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:211: {
I think this would be clearer if you just explicitly handled the 4 cases.

[skobes, 2016-02-29 18:52:47]

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:572: // TODO(bokan): I think the
scroll deltas here are backwards.
Can you clarify this TODO?  Why are they backwards and where should they be
corrected?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:796: // TODO(bokan): The
delta coming in here is the GSU event delta, which is
what is GSU?

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:286: //             based on
that box's writing mode.
Is this behavior covered by any spec?  It seems a little surprising that
chaining might scroll an ancestor in the opposite physical direction.

[bokan, 2016-03-01 05:55:02]

Patchset #4 (id:60001) has been deleted

[bokan, 2016-03-01 05:56:24]

PTAL. In particular, I fixed the ScrollResult calculations in RFV::userScroll
and SA::userScroll (and improved clarity I hope)

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:572: // TODO(bokan): I think the
scroll deltas here are backwards.
On 2016/02/29 18:52:47, skobes wrote:
> Can you clarify this TODO?  Why are they backwards and where should they be
> corrected?

Done. (I have a follow up patch which fixes the TODO I'll put up shortly).

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:796: // TODO(bokan): The
delta coming in here is the GSU event delta, which is
On 2016/02/29 18:52:47, skobes wrote:
> what is GSU?

Sorry, forgot not everyone works on input :) (Gesture Scroll Update).

I've updated the wording.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:797: // positive if the user
scrolls up. For scrolling, a positive delta implies
On 2016/02/29 14:53:43, tdresser wrote:
> positive if the user scrolls up or left

Done.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:238: // result of the
smooth scrolling work.
On 2016/02/29 14:53:43, tdresser wrote:
> I may just be lacking context, but it isn't clear to me what this comment is
> referring to.

I think it should be clearer now.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:251:
pixelDelta.setWidth(0);
On 2016/02/29 14:53:43, tdresser wrote:
> Can we avoid calling scrollStep in cases where we're going to ignore its
output?

Actually, we'll still want to call scrollStep so we can return the
unusedScrollDelta in pixels, though I realized I don't do that here or in
ScrollableArea::userScroll. I've updated both.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:258: return
visualViewport().userScroll(granularity, delta);
On 2016/02/29 14:53:43, tdresser wrote:
> If one axis didn't scroll, but the other did, this will cause a change in
> behavior, won't it? Shouldn't we allow scrolling the layout viewport on one
> axis, but the visual viewport on the other?

I'm not sure I understand. If pixelDelta.isZero() is true then neither axis is
scrollable and we just let the visual viewport handle it (visual viewport is
always scrollable so I removed the checks). Otherwise we drop down and do the
distribution logic which I think should handle the case you mention.

I've actually rewritten this method to correctly handle the return type. It's a
little complicated (no way around it I think - the viewport distribution
requires a bit of bookkeeping for the return value) but I've added plenty of
comments and hopefully made it more clear.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:276: ScrollResult
layoutResult = layoutViewport().scrollAnimator().userScroll(granularity,
pixelDelta - visualUsedDelta);
On 2016/02/29 14:53:43, tdresser wrote:
> I'm a bit confused about how this works. If I understand correctly:
> 1. If the layout viewport is user scrollable, scroll it.
> 2. If the layout viewport didn't scroll, try to scroll the visual viewport.
> 3. If the visual viewport didn't consume all the scroll, scroll the layout
> viewport.
> 
> Why would we ever do anything in step #3? Is it for cases where the layout
> viewport isn't user scrollable?

It's actually:

1. Try to scroll the visual viewport
2. If there's still unused delta remaining and the layout viewport is scrollable
in that direction, scroll it.

Were you looking at the old version? Admittedly, the various scrollability
checks and cases make it seem a little confusing...Take a look at the updated
version. Hopefully we can remove this as the TODO at the top says and use the
much simpler ScrollableArea::userScroll (it does the distribution once the
offset is actually applied).

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:613:
m_frame->document()->updateLayoutIgnorePendingStylesheets();
On 2016/02/29 14:53:43, tdresser wrote:
> Why was this not needed before, but needed now?

It was there before too, just before we enter the loop.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:619: // If we're at the
stopNode, we should try to scroll it but we shouldn't bubble past it
On 2016/02/29 14:53:43, tdresser wrote:
> Missing period.

Done.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:630: // on the result
instead rather than lying to the caller.
On 2016/02/29 14:53:43, tdresser wrote:
> instead rather -> rather

Done.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:268: // Performs a bubbling
scroll, within a *single* frame, starting from a
On 2016/02/29 14:53:43, tdresser wrote:
> I'd prefer if we used the terms "bubble" and "chain" to refer to event
bubbling,
> and scroll chaining respectively. It's confusing that we use the word bubble
to
> refer both to scrolling and to event dispatch.
> 
> I don't know if it's worth renaming "bubblingScroll", but it would be nice if
we
> could use the word "chain" in comments.

I've updated the comments. I'd like to remove bubblingScroll and replace it with
logicalScroll anyway so I'll do that in a followup.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:282: // from either a
provided starting node or a default based on focus/clicked.
On 2016/02/29 14:53:43, tdresser wrote:
> "based on focus/clicked"
> 
> Can this be a bit more precise? "based on the focused or most recently clicked
> Node"? or something along those lines?

Done.

https://codereview.chromium.org/1738243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:286: //             based on
that box's writing mode.
On 2016/02/29 18:52:47, skobes wrote:
> Is this behavior covered by any spec?  It seems a little surprising that
> chaining might scroll an ancestor in the opposite physical direction.

I'm not sure about a spec, but this is just describing long-existing behaviour.
Basically, if you press the space bar the scroll is in the "block-forward"
direction. This means that depending on which element ends up consuming the
scroll and what its writing direction is, we might scroll in different
directions. This is only used for directionless scrolls like that.

[tdresser, 2016-03-01 15:28:10]

Thanks, this is a lot clearer.

LGTM.

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:255: FloatSize
visualUsedDelta =
Let's try to be consistent in using "consumed" delta everywhere.

FloatSize visualConsumedDelta.

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:618: // bubble past it.
bubble -> chain

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:619: bool
shouldStopBubbling =
If you feel like it, shouldStopBubbling -> shouldStopChaining.

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:138: // Performs a logical
scroll that bubbles, crossing frames, starting from
bubbles -> chains

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:288: // granularity - The
size of the scroll.
I think the previous |granularity| comment was clearer.

Comments on parameters should probably be identical if the usage of the
parameter is identical.

[bokan, 2016-03-01 18:22:58]

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:255: FloatSize
visualUsedDelta =
On 2016/03/01 15:28:10, tdresser wrote:
> Let's try to be consistent in using "consumed" delta everywhere.
> 
> FloatSize visualConsumedDelta.

Done. (In ScrollAnimators as well)

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:618: // bubble past it.
On 2016/03/01 15:28:10, tdresser wrote:
> bubble -> chain

Done.

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:619: bool
shouldStopBubbling =
On 2016/03/01 15:28:10, tdresser wrote:
> If you feel like it, shouldStopBubbling -> shouldStopChaining.

Done.

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:138: // Performs a logical
scroll that bubbles, crossing frames, starting from
On 2016/03/01 15:28:10, tdresser wrote:
> bubbles -> chains

Done.

https://codereview.chromium.org/1738243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:288: // granularity - The
size of the scroll.
On 2016/03/01 15:28:10, tdresser wrote:
> I think the previous |granularity| comment was clearer.
> 
> Comments on parameters should probably be identical if the usage of the
> parameter is identical.

Done. The startNode param remains slightly different as it is optional in
logical but required in physical.

[skobes, 2016-03-01 18:55:15]

lgtm

https://codereview.chromium.org/1738243002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1738243002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:237: //
TODO(bokan/ymalik): Once smooth scrolling is permanantly enabled we
"permanently"

[bokan, 2016-03-01 18:59:21]

bokan@chromium.org changed reviewers:
+ jbroman@chromium.org

[bokan, 2016-03-01 18:59:21]

+jbroman@ for Source/platform OWNER

[jbroman, 2016-03-01 19:11:37]

platform/ rs lgtm

But this work seems complicated and connected to future work, so a non-empty
BUG= reference would be great.

[bokan, 2016-03-01 19:21:11]

I've created a bug to track all the related work here.

[bokan, 2016-03-01 19:21:24]

Description was changed from

==========
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=
==========

to

==========
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
==========

[bokan, 2016-03-01 19:21:50]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-03-01 19:21:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1738243002/#ps120001
(title: "Spelling nit")

[commit-bot: I haz the power, 2016-03-01 19:22:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738243002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738243002/120001

[commit-bot: I haz the power, 2016-03-01 19:27:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 19:27:17]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbroman, 2016-03-01 19:41:25]

On 2016/03/01 at 19:21:11, bokan wrote:
> I've created a bug to track all the related work here.

Thanks!

[bokan, 2016-03-01 21:25:35]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-03-01 21:25:35]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org,
jbroman@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1738243002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-01 21:26:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738243002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738243002/140001

[commit-bot: I haz the power, 2016-03-02 01:14:44]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-03-02 01:14:45]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-03-02 01:15:42]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc
Cr-Commit-Position: refs/heads/master@{#378642}
==========

[commit-bot: I haz the power, 2016-03-02 01:15:44]

Patchset 7 (id:??) landed as
https://crrev.com/f0713e7d9da9b89301ae0ae1228d51fbb2e17afc
Cr-Commit-Position: refs/heads/master@{#378642}