Mac: Add rails for scrolling.

Mac: Add rails for scrolling.

Add a MouseWheelRailsFilter structure to compute the rail mode for
input events, based on some simple hysteresis.

Make InputHandlerProxy honor the rails requests of MouseWheel events.
This is not an ideal long-term implementation -- the rails should be
passed to cc::InputHandler, so that they can be handled correctly by
other event types. This version is being used so that it may be merged.

BUG=468454
Committed: https://crrev.com/a51f1f51c94835659073afa96a7a57f3544c6da9
Cr-Commit-Position: refs/heads/master@{#321738}

[jdduke (slow), 2015-03-18 23:39:13]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-03-18 23:39:14]

https://codereview.chromium.org/1019153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h (right):

https://codereview.chromium.org/1019153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h:17:
~MouseWheelRailsFilterMac();
I think we're missing a file here?

[ccameron, 2015-03-19 00:09:21]

Yup -- updated

https://codereview.chromium.org/1019153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h (right):

https://codereview.chromium.org/1019153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h:17:
~MouseWheelRailsFilterMac();
On 2015/03/18 23:39:13, jdduke wrote:
> I think we're missing a file here?

Yup -- added.

[jdduke (slow), 2015-03-19 00:21:54]

https://codereview.chromium.org/1019153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:21: const
float kDecayConstant = 0.8f;
Without hooking this to time, the decay becomes frequency-sensitive (i.e.,
you'll get different behavior if events arrive at 60hz vs 120hz for the same
overall motion) which seems a little suspect.

In practice, this may not matter so much, but I wonder if it would be that much
additional work to fix (assuming wheel events are provided sane timestamps).

https://codereview.chromium.org/1019153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:30:
decayed_delta_ += gfx::Vector2dF(fabsf(event.deltaX), fabsf(event.deltaY));
std::abs

[ccameron, 2015-03-19 00:42:26]

Updated

https://codereview.chromium.org/1019153002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:21: const
float kDecayConstant = 0.8f;
On 2015/03/19 00:21:53, jdduke wrote:
> Without hooking this to time, the decay becomes frequency-sensitive (i.e.,
> you'll get different behavior if events arrive at 60hz vs 120hz for the same
> overall motion) which seems a little suspect.
> 
> In practice, this may not matter so much, but I wonder if it would be that
much
> additional work to fix (assuming wheel events are provided sane timestamps).

The timestamps are sane, but the main goal of this is to fix up the last
trailing 2 or 3 events at the end of a scroll, where the delta gets quantized to
0 in the desired axis occasionally. The bulk of the scroll can determine the
mode with the with the x>y test. These last few events also tend to come in at a
lower rate than the other events in the scroll (which I guess makes sense --
they're only appearing as they pass the quantization threshold), so time isn't
so much of a factor.

https://codereview.chromium.org/1019153002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:30:
decayed_delta_ += gfx::Vector2dF(fabsf(event.deltaX), fabsf(event.deltaY));
On 2015/03/19 00:21:53, jdduke wrote:
> std::abs

Done.

[tdresser, 2015-03-19 13:03:40]

tdresser@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2015-03-19 13:03:41]

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:33: if
(decayed_delta_.y() >= decayed_delta_.x())
Do we want to rail this aggressively?
This makes diagonal scrolling impossible for scrollers which respect rails at
all, doesn't it?

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/mouse_wheel_rails_filter_unittest_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_unittest_mac.cc:27:
scoped_ptr<MouseWheelRailsFilterMac> filter(new MouseWheelRailsFilterMac);
Can't this just be stack allocated?

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.h:179:
scoped_ptr<content::MouseWheelRailsFilterMac> mouseWheelFilter_;
Why use a scoped_ptr here? It doesn't need to be nullable, does it?

[ccameron, 2015-03-19 19:33:15]

Thanks -- updated.

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:33: if
(decayed_delta_.y() >= decayed_delta_.x())
On 2015/03/19 13:03:40, tdresser wrote:
> Do we want to rail this aggressively?
> This makes diagonal scrolling impossible for scrollers which respect rails at
> all, doesn't it?
> 

Exactly. This mirrors the behavior that Safari has.

(I have a suspicion that Safari has this behavior because WebKit does a
terribleterribleterrible job propagating diagonal scrolls -- we've inherited
this in Blink, and forcing things to an axis does make behavior much better for
Blink scrolls. For cc, it's less of an improvement).

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/input/mouse_wheel_rails_filter_unittest_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_unittest_mac.cc:27:
scoped_ptr<MouseWheelRailsFilterMac> filter(new MouseWheelRailsFilterMac);
On 2015/03/19 13:03:41, tdresser wrote:
> Can't this just be stack allocated?

Yup, it can. Habit... done.

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.h:179:
scoped_ptr<content::MouseWheelRailsFilterMac> mouseWheelFilter_;
On 2015/03/19 13:03:41, tdresser wrote:
> Why use a scoped_ptr here? It doesn't need to be nullable, does it?

True -- done.

[tdresser, 2015-03-19 20:57:51]

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc
(right):

https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:33: if
(decayed_delta_.y() >= decayed_delta_.x())
On 2015/03/19 19:33:14, ccameron wrote:
> On 2015/03/19 13:03:40, tdresser wrote:
> > Do we want to rail this aggressively?
> > This makes diagonal scrolling impossible for scrollers which respect rails
at
> > all, doesn't it?
> > 
> 
> Exactly. This mirrors the behavior that Safari has.
> 
> (I have a suspicion that Safari has this behavior because WebKit does a
> terribleterribleterrible job propagating diagonal scrolls -- we've inherited
> this in Blink, and forcing things to an axis does make behavior much better
for
> Blink scrolls. For cc, it's less of an improvement).

Yikes. I just tried it out on Safari - can't say I'm a fan, but we can always
revisit this later, possibly after unifying main and impl scrolling.

[ccameron, 2015-03-19 21:05:58]

On 2015/03/19 20:57:51, tdresser wrote:
>
https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
> File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc
> (right):
> 
>
https://codereview.chromium.org/1019153002/diff/40001/content/browser/rendere...
> content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.cc:33: if
> (decayed_delta_.y() >= decayed_delta_.x())
> On 2015/03/19 19:33:14, ccameron wrote:
> > On 2015/03/19 13:03:40, tdresser wrote:
> > > Do we want to rail this aggressively?
> > > This makes diagonal scrolling impossible for scrollers which respect rails
> at
> > > all, doesn't it?
> > > 
> > 
> > Exactly. This mirrors the behavior that Safari has.
> > 
> > (I have a suspicion that Safari has this behavior because WebKit does a
> > terribleterribleterrible job propagating diagonal scrolls -- we've inherited
> > this in Blink, and forcing things to an axis does make behavior much better
> for
> > Blink scrolls. For cc, it's less of an improvement).
> 
> Yikes. I just tried it out on Safari - can't say I'm a fan, but we can always
> revisit this later, possibly after unifying main and impl scrolling.

Definitely agree about not being a big fan of the Safari-style -- the real
nastiness is when you remove rails and have main-thread/Blink scrolling (I'll
capture a video for the bug). Adding rails at least prevents that.

[jdduke (slow), 2015-03-19 22:27:45]

content/browser/renderer_host/input and content/renderer/input lgtm.

A sanity InputHandlerProxyTest would be welcome :).

https://codereview.chromium.org/1019153002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h (right):

https://codereview.chromium.org/1019153002/diff/60001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h:14: class
CONTENT_EXPORT MouseWheelRailsFilterMac {
It would be nice to have a brief comment here explaining the basic functionality
and its inspiration (Safari).

https://codereview.chromium.org/1019153002/diff/60001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_rails_filter_mac.h:22:
gfx::Vector2dF decayed_delta_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1019153002/diff/60001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1019153002/diff/60001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:337: gfx::Vector2dF scroll_delta(
Not sure if it would be easier to read like:

gfx::Vector2dF scroll_delta(-wheel_event.deltaX, -wheel_event.deltaY);
if (wheel_event.railsMode == WebInputEvent::RailsModeVertical)
  scroll_delta.set_x(0);
if (wheel_event.railsMode == WebInputEvent::RailsModeHorizontal)
  scroll_delta.set_y(0);

[jdduke (slow), 2015-03-19 22:30:58]

On 2015/03/19 20:57:51, tdresser wrote:
> On 2015/03/19 19:33:14, ccameron wrote:
> > Exactly. This mirrors the behavior that Safari has.
> > 
> > (I have a suspicion that Safari has this behavior because WebKit does a
> > terribleterribleterrible job propagating diagonal scrolls -- we've inherited
> > this in Blink, and forcing things to an axis does make behavior much better
> for
> > Blink scrolls. For cc, it's less of an improvement).
> 
> Yikes. I just tried it out on Safari - can't say I'm a fan, but we can always
> revisit this later, possibly after unifying main and impl scrolling.

Wow, yeah I just tried it too, it's pretty extreme =/

[tdresser, 2015-03-20 15:43:46]

On 2015/03/19 22:30:58, jdduke wrote:
> On 2015/03/19 20:57:51, tdresser wrote:
> > On 2015/03/19 19:33:14, ccameron wrote:
> > > Exactly. This mirrors the behavior that Safari has.
> > > 
> > > (I have a suspicion that Safari has this behavior because WebKit does a
> > > terribleterribleterrible job propagating diagonal scrolls -- we've
inherited
> > > this in Blink, and forcing things to an axis does make behavior much
better
> > for
> > > Blink scrolls. For cc, it's less of an improvement).
> > 
> > Yikes. I just tried it out on Safari - can't say I'm a fan, but we can
always
> > revisit this later, possibly after unifying main and impl scrolling.
> 
> Wow, yeah I just tried it too, it's pretty extreme =/

content/browser/renderer_host/input LGTM.

[ccameron, 2015-03-23 05:23:00]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-03-23 05:23:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019153002/60001

[commit-bot: I haz the power, 2015-03-23 06:21:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-23 06:22:10]

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