Remove special wheel handling path from ScrollableArea

Remove special wheel handling path from ScrollableArea

ScrollableArea maintained its own special path for wheel handlers. This was
duplicating logic with other scrolls that used ScrollableArea::userScroll and
confusing (why isn't there a handleTouch? handleKey?).

I've removed ScrollableArea::handleWheel and its overrides and moved all the
WheelEvelt -> Scroll Delta logic into EventHandler from where it simply calls
ScrollableArea::userScroll like all other scroll paths.

I also made a slight change to the page step size of ScrollableArea: it's now
87.5% of the visible area *including* scrollbars. This was necessary since on
the root frame, the visible area excluding scrollbars is a somewhat arbitrary
calculation when  pinch-zoomed. While it would be possible to come up with some
scheme or make the step size different for root and non-root frames this would
unnecessarily complicate the code for little gain so I just based it off the
size of the frame rect.

BUG=443724
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201145

[bokan, 2015-08-18 18:57:12]

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

[commit-bot: I haz the power, 2015-08-18 18:57:27]

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

[commit-bot: I haz the power, 2015-08-18 20:08:35]

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

[commit-bot: I haz the power, 2015-08-18 20:08:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bokan, 2015-08-19 12:32:03]

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

[bokan, 2015-08-19 12:42:47]

Hey Rick, ptal.

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
File
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html
(right):

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html:29:
debug("document.scrollingElement.scrollLeft = " +
document.scrollingElement.scrollLeft);
The expectation is platform dependent so we don't know the expected values here.

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/platform/ma...
File
LayoutTests/platform/mac/fast/events/platform-wheelevent-paging-xy-in-scrolling-page-expected.txt
(right):

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/platform/ma...
LayoutTests/platform/mac/fast/events/platform-wheelevent-paging-xy-in-scrolling-page-expected.txt:5:
document.scrollingElement.scrollLeft = 760
Mac sets a maxOverlapBetweenPages so that gets used rather than the % based
calculation:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1298973004/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/1298973004/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:313: ScrollResult wheelEvent(const
PlatformWheelEvent&);
This was unused

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimatorNone.cpp (right):

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimatorNone.cpp:434: return
ScrollResultOneDimensional(needToScroll, unusedDelta);
This wasn't returning unused delta. It's needed for overscroll to work
correctly.

[Rick Byers, 2015-08-19 17:33:31]

Nice cleanup, thanks!

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
File
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html
(right):

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html:29:
debug("document.scrollingElement.scrollLeft = " +
document.scrollingElement.scrollLeft);
On 2015/08/19 12:42:47, bokan wrote:
> The expectation is platform dependent so we don't know the expected values
here.

Why did your CL change that though?  Are you intending to make the behavior more
platform dependent?  Ideally we should be setting up modes/inputs in layout
tests to be platform independent (but have multiple test cases for the different
behaviors possible in different platforms).

https://codereview.chromium.org/1298973004/diff/40001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1298973004/diff/40001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1731: granularity = ScrollByPrecisePixel;
why do we need this?  Should be handled up in chromium, no?  Maybe ad a TODO

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimatorNone.cpp (right):

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimatorNone.cpp:434: return
ScrollResultOneDimensional(needToScroll, unusedDelta);
On 2015/08/19 12:42:47, bokan wrote:
> This wasn't returning unused delta. It's needed for overscroll to work
> correctly.

So you're fixing a bug here?  Add a test for it?

[bokan, 2015-08-19 18:45:47]

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
File
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html
(right):

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html:29:
debug("document.scrollingElement.scrollLeft = " +
document.scrollingElement.scrollLeft);
On 2015/08/19 17:33:30, Rick Byers wrote:
> On 2015/08/19 12:42:47, bokan wrote:
> > The expectation is platform dependent so we don't know the expected values
> here.
> 
> Why did your CL change that though?  Are you intending to make the behavior
more
> platform dependent?  Ideally we should be setting up modes/inputs in layout
> tests to be platform independent (but have multiple test cases for the
different
> behaviors possible in different platforms).

No, there's no change in the dependency on platform, I'm just fixing the
currently broken expectation (Mac has a separate expectation with a FAILing
output). The problem is that the value outputted is dependent on platform so I
don't think the test should be outputting PASS/FAIL (since it doesn't know what
the expected value is) - I made it output the value and the platform dependent
expectation tests for correctness.

https://codereview.chromium.org/1298973004/diff/40001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1298973004/diff/40001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1731: granularity = ScrollByPrecisePixel;
On 2015/08/19 17:33:31, Rick Byers wrote:
> why do we need this?  Should be handled up in chromium, no?  Maybe ad a TODO

I don't understand this well but I moved it from
ScrollAnimator::handleWheelEvent. I agree this doesn't belong, I'll add a TODO.
(In general, it'd be nice to unify all touchpad/wheel code, Mac seems to have
numerous exceptions).

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimatorNone.cpp (right):

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimatorNone.cpp:434: return
ScrollResultOneDimensional(needToScroll, unusedDelta);
On 2015/08/19 17:33:31, Rick Byers wrote:
> On 2015/08/19 12:42:47, bokan wrote:
> > This wasn't returning unused delta. It's needed for overscroll to work
> > correctly.
> 
> So you're fixing a bug here?  Add a test for it?

This is caught by
WebFrameOverscrollTest.ReportingLatestOverscrollForElasticOverscroll, it was
uncovered by this refactoring because we no longer use a special path for the
RootFrame.

[Rick Byers, 2015-08-19 19:42:09]

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
File
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html
(right):

https://codereview.chromium.org/1298973004/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html:29:
debug("document.scrollingElement.scrollLeft = " +
document.scrollingElement.scrollLeft);
On 2015/08/19 18:45:46, bokan wrote:
> On 2015/08/19 17:33:30, Rick Byers wrote:
> > On 2015/08/19 12:42:47, bokan wrote:
> > > The expectation is platform dependent so we don't know the expected values
> > here.
> > 
> > Why did your CL change that though?  Are you intending to make the behavior
> more
> > platform dependent?  Ideally we should be setting up modes/inputs in layout
> > tests to be platform independent (but have multiple test cases for the
> different
> > behaviors possible in different platforms).
> 
> No, there's no change in the dependency on platform, I'm just fixing the
> currently broken expectation (Mac has a separate expectation with a FAILing
> output). The problem is that the value outputted is dependent on platform so I
> don't think the test should be outputting PASS/FAIL (since it doesn't know
what
> the expected value is) - I made it output the value and the platform dependent
> expectation tests for correctness.

Sorry, I wrote this before seeing the test changes.  In general we rely on
shouldBe to indicate what we really expect, and 'debug' to indicate things that
can probably be blindly updated.  Ideally layout tests would have a way to set
or get the maxOverlapBetweenPages so we're not platform dependant, but adding
that is probably overkill.  What do you think about just hard-coding the
expected maxOverlapBetweenPages value in the test file, using a UA string check
to see if we're on Mac?  Then we could have a shouldBe that would PASS on all
platforms.

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimatorNone.cpp (right):

https://codereview.chromium.org/1298973004/diff/40001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimatorNone.cpp:434: return
ScrollResultOneDimensional(needToScroll, unusedDelta);
On 2015/08/19 18:45:47, bokan wrote:
> On 2015/08/19 17:33:31, Rick Byers wrote:
> > On 2015/08/19 12:42:47, bokan wrote:
> > > This wasn't returning unused delta. It's needed for overscroll to work
> > > correctly.
> > 
> > So you're fixing a bug here?  Add a test for it?
> 
> This is caught by
> WebFrameOverscrollTest.ReportingLatestOverscrollForElasticOverscroll, it was
> uncovered by this refactoring because we no longer use a special path for the
> RootFrame.

Ah, ok - thanks.

https://codereview.chromium.org/1298973004/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1298973004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1729: // TODO(bokan): This should be moved
out into Chromium or removed if possible.
In particular, I can't imagine why CrOS and Mac should be different here.  Maybe
this code is fine but just shouldn't have an #if?

[bokan, 2015-08-25 16:34:49]

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

[commit-bot: I haz the power, 2015-08-25 16:35:04]

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

[commit-bot: I haz the power, 2015-08-25 17:32:43]

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

[commit-bot: I haz the power, 2015-08-25 17:32:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2015-08-25 17:53:22]

Hey Rick, I've updated the test expectations to be more idiomatic. If you could,
please take a look before you leave.

https://codereview.chromium.org/1298973004/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1298973004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1729: // TODO(bokan): This should be moved
out into Chromium or removed if possible.
On 2015/08/19 19:42:09, Rick Byers wrote:
> In particular, I can't imagine why CrOS and Mac should be different here. 
Maybe
> this code is fine but just shouldn't have an #if?

Ok, I've removed the #ifdef and it seems not to be breaking any tests.

[Rick Byers, 2015-08-25 18:09:58]

Thanks, LGTM!

https://codereview.chromium.org/1298973004/diff/80001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1298973004/diff/80001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1729: // TODO(bokan): This should be moved
out into Chromium or removed if possible.
nit: now that the #if is gone, do you still even want this TODO?  I guess you
could argue that PlatformMouseWheelEvent hasPrecisePixelDeltas should get merged
into the granularity field there.  But doesn't seem too bad to me (certainly no
longer really a bug here - this just converts from the PlatformMouseWheelEvent
representation of granularity to the ScrollGranularity representation).

[bokan, 2015-08-25 18:13:45]

https://codereview.chromium.org/1298973004/diff/80001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1298973004/diff/80001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:1729: // TODO(bokan): This should be moved
out into Chromium or removed if possible.
On 2015/08/25 18:09:58, Rick Byers wrote:
> nit: now that the #if is gone, do you still even want this TODO?  I guess you
> could argue that PlatformMouseWheelEvent hasPrecisePixelDeltas should get
merged
> into the granularity field there.  But doesn't seem too bad to me (certainly
no
> longer really a bug here - this just converts from the PlatformMouseWheelEvent
> representation of granularity to the ScrollGranularity representation).

Yah, I suppose it'd be cleaner to just have a "PrecisePixel" enum on the wheel
event, but it's probably not worth the effort. I'll remove this TODO.

[bokan, 2015-08-25 18:49:58]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-08-25 18:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1298973004/#ps100001
(title: "Removed TODO")

[commit-bot: I haz the power, 2015-08-25 18:50:08]

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

[commit-bot: I haz the power, 2015-08-25 20:03:07]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201145