Implement Wheel Gesture Scrolling on OSX.

Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1827dd232f84cc1143914fe89e375eb33c7d12d6
Cr-Commit-Position: refs/heads/master@{#380639}

[dtapuska, 2016-03-01 22:02:24]

Description was changed from

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
==========

to

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
==========

[dtapuska, 2016-03-01 22:02:24]

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

[tdresser, 2016-03-02 18:26:53]

Seems like we could refactor this to share more between the wheel event and
gesture event implementations, but I guess we'll be pulling out the wheel
implementations soon enough that it isn't a big deal.

I haven't done a thorough review of the tests, and will need another pass over
the wheel event queue.

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

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:89:
(base::TimeTicks::Now() - base::TimeTicks()).InSecondsF();
We should take the timestamp from the wheel event. (We use the touch event
timestamps for gesture events).

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:104:
blink::WebMouseWheelEvent::PhaseNone;
What's the momentum phase for non-mac?
Maybe add a comment?

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:193:
(base::TimeTicks::Now() - base::TimeTicks()).InSecondsF();
I suppose this timestamp needs to be made up, unfortunately.

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221:
gesture.event.data.scrollUpdate.inertial;
It isn't clear to me when we'd need a scroll begin but have an inertial update.

When will this happen?

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
content/renderer/input/input_handler_manager.cc:150: const
cc::InputHandlerScrollResult& scroll_result) {
Should we check that we're on the correct thread here?

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:107: const
cc::InputHandlerScrollResult& scroll_result);
nit: newline

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
content/renderer/input/render_widget_input_handler.cc:370: if (input_event.type
== WebInputEvent::MouseWheel) {
We should make sure that removing all the wheel specific code is part of the
plan here.

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/mus/re...
File content/renderer/mus/render_widget_mus_connection.h (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/mus/re...
content/renderer/mus/render_widget_mus_connection.h:47: const gfx::Vector2dF&
wheel_unused_delta,
nit:indent

https://codereview.chromium.org/1749343004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1749343004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:516: bool inertial;
Can we add a comment indicating what this means?
Can it ever be true if the source if touch?

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:695:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Can we share this logic with HandleGestureScrollBegin/End?

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller.cc:117: wheel_event.phase ==
blink::WebMouseWheelEvent::PhaseBegan) {
This change will regress this behavior, because we'll switch from using real
wheel event phases to using our fake phases, right?

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller_unittest.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:134: }
nit: newline

[dtapuska, 2016-03-04 21:23:09]

Description was changed from

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
==========

to

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dtapuska, 2016-03-07 18:03:59]

PTAL

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

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:89:
(base::TimeTicks::Now() - base::TimeTicks()).InSecondsF();
On 2016/03/02 18:26:53, tdresser wrote:
> We should take the timestamp from the wheel event. (We use the touch event
> timestamps for gesture events).

Done.

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:104:
blink::WebMouseWheelEvent::PhaseNone;
On 2016/03/02 18:26:53, tdresser wrote:
> What's the momentum phase for non-mac?
> Maybe add a comment?

Done.

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:193:
(base::TimeTicks::Now() - base::TimeTicks()).InSecondsF();
On 2016/03/02 18:26:53, tdresser wrote:
> I suppose this timestamp needs to be made up, unfortunately.

Acknowledged.

https://codereview.chromium.org/1749343004/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:221:
gesture.event.data.scrollUpdate.inertial;
On 2016/03/02 18:26:53, tdresser wrote:
> It isn't clear to me when we'd need a scroll begin but have an inertial
update.
> 
> When will this happen?

So the way phasing is done on OSX is that the phase terminates and it doesn't
transition into the inertial state via a GSU... ie; it will be:
GSB (inertial = false)
GSU (inertial = false)
GSE 
GSB (inertial = true)
GSU (inertial = true)
GSE

If we wanted to avoid the GSE/GSB in the middle we could hold onto generating
the GSE not based on when the phases change but based on a timer. However I have
seen events not generate a momentum phase and just terminate. So in those cases
the GSE would be delayed by the timer. This seemed to be the best compromise;
and it was similar to the events requested in here:
https://github.com/w3c/uievents/issues/58

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
content/renderer/input/input_handler_manager.cc:150: const
cc::InputHandlerScrollResult& scroll_result) {
On 2016/03/02 18:26:53, tdresser wrote:
> Should we check that we're on the correct thread here?

I've added a check in the OnCompositorThread since that is an affirmative check;
adding it here OnMainThread might not be wise since task_runner_ could in-fact
be the same thread as the main thread.

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:107: const
cc::InputHandlerScrollResult& scroll_result);
On 2016/03/02 18:26:53, tdresser wrote:
> nit: newline

Done.

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/input/...
content/renderer/input/render_widget_input_handler.cc:370: if (input_event.type
== WebInputEvent::MouseWheel) {
On 2016/03/02 18:26:53, tdresser wrote:
> We should make sure that removing all the wheel specific code is part of the
> plan here.

Acknowledged.

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/mus/re...
File content/renderer/mus/render_widget_mus_connection.h (right):

https://codereview.chromium.org/1749343004/diff/20001/content/renderer/mus/re...
content/renderer/mus/render_widget_mus_connection.h:47: const gfx::Vector2dF&
wheel_unused_delta,
On 2016/03/02 18:26:53, tdresser wrote:
> nit:indent

Done.

https://codereview.chromium.org/1749343004/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1749343004/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:516: bool inertial;
On 2016/03/02 18:26:53, tdresser wrote:
> Can we add a comment indicating what this means?
> Can it ever be true if the source if touch?

Done.

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:695:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/03/02 18:26:53, tdresser wrote:
> Can we share this logic with HandleGestureScrollBegin/End?

Done.

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller.cc:117: wheel_event.phase ==
blink::WebMouseWheelEvent::PhaseBegan) {
On 2016/03/02 18:26:53, tdresser wrote:
> This change will regress this behavior, because we'll switch from using real
> wheel event phases to using our fake phases, right?

There will always be a scroll begin, update, end.

So this logic has moved into the Scroll begin. Correct if we get a may begin in
the middle of current phase information it won't actually reset it to the
correct state; but I never saw incorrectly sequenced messages from OSX.

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller_unittest.cc (right):

https://codereview.chromium.org/1749343004/diff/20001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:134: }
On 2016/03/02 18:26:53, tdresser wrote:
> nit: newline

Done.

[tdresser, 2016-03-08 14:28:53]

https://codereview.chromium.org/1749343004/diff/80001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/1749343004/diff/80001/cc/input/input_handler....
cc/input/input_handler.h:103: GESTURE,  // TODO(dtapuska): Remove this and just
use TOUCHSCREEN.
I'm a fan of giving TODOs associated bugs and referencing them in the comments -
it makes it easier to tell that you've done all the associated cleanup.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:135: send_end_now
= event_sent_for_gesture_ack_->event.phase ==
Add a comment describing the conditions in which we should send an end.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:148: if
(needs_update || !(needs_scroll_begin_ && send_end_now)) {
I'm a bit confused by this condition.

It's probably a bit easier to read as:
if (needs_update || !needs_scroll_begin || !send_end_now)

I'm not quite clear on why this it's necessary though. Maybe add a comment?

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:150:
SendScrollBegin(scroll_update, !needs_scroll_begin_);
This is a bit confusing. Why do we mark it synthetic if we don't need a scroll
begin?

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:155: if
(send_end_now) {
The naming of this variable is confusing. We send a scroll end even if
send_end_now is false?

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc
(right):

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:301:
HAS_SYNTHETIC_END);
The distance between sending the mousewheel and defining the expected result
makes this hard to follow. I also found interpreting the way that |flags| is
being used to be a bit tricky.

A few options for making this easier to read:

1. Create a local variable per set of flags, that you define directly after the
associated SendMouseWheelEventWithPhase. Rename |flags| to something like
|expected|.

2. Rename ValidatePhaseSequence to something like SendWheelAck, which returns a
struct containing the information you are currently expecting on via flags. You
could then instantiate one of these structs directly beneath each call to
SendMouseWheelWithPhase.

I think 2 would be my preference. You'd end up with something like:

SendMouseWheelWithPhase(kWheelScrollX, kWheelScrollY, kWheelScrollGlobalX,
                        kWheelScrollGlobalY, 1, 1, 0, high_precision,
                        WebMouseWheelEvent::PhaseBegan,
                        WebMouseWheelEvent::PhaseNone);
ExpectedEvents wheel_1_expected;
wheel_1_expected.has_scroll_update = true;
wheel_1_expected.has_mousewheel = true;
wheel_1_expected.has_synthetic_begin = true;
wheel_1_expected.has_synthetic_end = true;

// .. Rest of SendMouseWheelWithPhase calls, a few EXPECT's.

EXPECT_EQ(wheel_1_expected, SendWheelAck(high_precision));

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:369:
TEST_F(MouseWheelEventQueueTest, GestureSendingWithPhaseInformation) {
You could use TEST_P here, but I don't think it has any strong benefits.

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:524: // until crbug.com/526463 is
fully implemented.
Mentioning that these synthesized begin events are used to force a hit test
might make this a bit clearer. 

Incorporating something like the following perhaps:

True if this event was synthesized in order to force a hit test, to avoid scroll
latching behavior until crbug.com/526463 is fully implemented.

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:550: // scroll sequence; OSX has
unique phases for normal and
Is this correct? If this is true, the event comes after an inertial gesture
scroll sequence, right?

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:557: // until crbug.com/526463 is
fully implemented.
I'd mention that these are only needed to match with the synthesized GSB events
that we use to force hit testing to avoid scroll latching.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:440: ;
Remove extra ;

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:686: // Do nothing if we generated called
ScrollAnimate; it will handle the end
"if we generated called"
grammar

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:1336: // the over-scroll animation.Note
that the call to the elasticity controller
animation.Note 
missing space

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.h (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.h:141: bool ShouldAnimate(bool
has_coarse_scroll_pixels) const;
Can we pass has_precise_scroll_deltas? It would be more consistent with what we
pass around elsewhere.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller.cc:177: switch
(gesture_event.type) {
Considering ordering this switch
Begin, Update, End

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller.h (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller.h:65: // this class may
disregard some scrolls that come in at unexpected times.
How hard would it be to have a test of this behavior, ensuring that things don't
explode? I guess we'd have to characterize the unreliable Cocoa behavior, which
probably isn't worth it?

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller_unittest.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:428: // Verify
that an stretch caused by a momentum scroll will switch to the
an stretch -> a stretch

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:551: // Verify
that an stretch opposing a scroll is correctly resolved.
a stretch

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:646: // and also
that the stretch amount isn't even ever changed.
This comment is a bit confusing.

What's the difference between "doesn't change" and "isn't even ever changed"?

[dtapuska, 2016-03-08 20:31:50]

https://codereview.chromium.org/1749343004/diff/80001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/1749343004/diff/80001/cc/input/input_handler....
cc/input/input_handler.h:103: GESTURE,  // TODO(dtapuska): Remove this and just
use TOUCHSCREEN.
On 2016/03/08 14:28:52, tdresser wrote:
> I'm a fan of giving TODOs associated bugs and referencing them in the comments
-
> it makes it easier to tell that you've done all the associated cleanup.

Done.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:135: send_end_now
= event_sent_for_gesture_ack_->event.phase ==
On 2016/03/08 14:28:52, tdresser wrote:
> Add a comment describing the conditions in which we should send an end.

I'm hoping the renaming of the variable makes it clear that a comment isn't
needed. Let me know if you still feel it does.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:148: if
(needs_update || !(needs_scroll_begin_ && send_end_now)) {
On 2016/03/08 14:28:52, tdresser wrote:
> I'm a bit confused by this condition.
> 
> It's probably a bit easier to read as:
> if (needs_update || !needs_scroll_begin || !send_end_now)
> 
> I'm not quite clear on why this it's necessary though. Maybe add a comment?
> 
> 
> 

Done.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:150:
SendScrollBegin(scroll_update, !needs_scroll_begin_);
On 2016/03/08 14:28:52, tdresser wrote:
> This is a bit confusing. Why do we mark it synthetic if we don't need a scroll
> begin?

I've broken it apart to make it more clear.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:155: if
(send_end_now) {
On 2016/03/08 14:28:52, tdresser wrote:
> The naming of this variable is confusing. We send a scroll end even if
> send_end_now is false?

I've changed send_end_now to current_phase_ended...

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc
(right):

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:301:
HAS_SYNTHETIC_END);
On 2016/03/08 14:28:53, tdresser wrote:
> The distance between sending the mousewheel and defining the expected result
> makes this hard to follow. I also found interpreting the way that |flags| is
> being used to be a bit tricky.
> 
> A few options for making this easier to read:
> 
> 1. Create a local variable per set of flags, that you define directly after
the
> associated SendMouseWheelEventWithPhase. Rename |flags| to something like
> |expected|.
> 
> 2. Rename ValidatePhaseSequence to something like SendWheelAck, which returns
a
> struct containing the information you are currently expecting on via flags.
You
> could then instantiate one of these structs directly beneath each call to
> SendMouseWheelWithPhase.
> 
> I think 2 would be my preference. You'd end up with something like:
> 
> SendMouseWheelWithPhase(kWheelScrollX, kWheelScrollY, kWheelScrollGlobalX,
>                         kWheelScrollGlobalY, 1, 1, 0, high_precision,
>                         WebMouseWheelEvent::PhaseBegan,
>                         WebMouseWheelEvent::PhaseNone);
> ExpectedEvents wheel_1_expected;
> wheel_1_expected.has_scroll_update = true;
> wheel_1_expected.has_mousewheel = true;
> wheel_1_expected.has_synthetic_begin = true;
> wheel_1_expected.has_synthetic_end = true;
> 
> // .. Rest of SendMouseWheelWithPhase calls, a few EXPECT's.
> 
> EXPECT_EQ(wheel_1_expected, SendWheelAck(high_precision));

I've replaced it with macros that I find much easier to read.

https://codereview.chromium.org/1749343004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:369:
TEST_F(MouseWheelEventQueueTest, GestureSendingWithPhaseInformation) {
On 2016/03/08 14:28:53, tdresser wrote:
> You could use TEST_P here, but I don't think it has any strong benefits.

For two parameters; true vs false it is much more code.. but if this expands
then I agree.

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:524: // until crbug.com/526463 is
fully implemented.
On 2016/03/08 14:28:53, tdresser wrote:
> Mentioning that these synthesized begin events are used to force a hit test
> might make this a bit clearer. 
> 
> Incorporating something like the following perhaps:
> 
> True if this event was synthesized in order to force a hit test, to avoid
scroll
> latching behavior until crbug.com/526463 is fully implemented.

Done.

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:550: // scroll sequence; OSX has
unique phases for normal and
On 2016/03/08 14:28:53, tdresser wrote:
> Is this correct? If this is true, the event comes after an inertial gesture
> scroll sequence, right?

correct; I had copied the text from the begin which isn't really correct.

https://codereview.chromium.org/1749343004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebInputEvent.h:557: // until crbug.com/526463 is
fully implemented.
On 2016/03/08 14:28:53, tdresser wrote:
> I'd mention that these are only needed to match with the synthesized GSB
events
> that we use to force hit testing to avoid scroll latching.

Done.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:440: ;
On 2016/03/08 14:28:53, tdresser wrote:
> Remove extra ;

Done.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:686: // Do nothing if we generated called
ScrollAnimate; it will handle the end
On 2016/03/08 14:28:53, tdresser wrote:
> "if we generated called"
> grammar

Done.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:1336: // the over-scroll animation.Note
that the call to the elasticity controller
On 2016/03/08 14:28:53, tdresser wrote:
> animation.Note 
> missing space

Done.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.h (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.h:141: bool ShouldAnimate(bool
has_coarse_scroll_pixels) const;
On 2016/03/08 14:28:53, tdresser wrote:
> Can we pass has_precise_scroll_deltas? It would be more consistent with what
we
> pass around elsewhere.

done; although for the gesture case; it is a double negative; but that might be
easier to understand I guess

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller.cc:177: switch
(gesture_event.type) {
On 2016/03/08 14:28:53, tdresser wrote:
> Considering ordering this switch
> Begin, Update, End

Done.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller.h (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller.h:65: // this class may
disregard some scrolls that come in at unexpected times.
On 2016/03/08 14:28:53, tdresser wrote:
> How hard would it be to have a test of this behavior, ensuring that things
don't
> explode? I guess we'd have to characterize the unreliable Cocoa behavior,
which
> probably isn't worth it?

I think the behavior is that if you are using two input devices the phase
information will be interleaved and could be interpreted at incorrect times. The
mouse wheel event queue will try to make something sane out of it. But it could
certainly confused this class; this comment was copied from the
ObserveWheelEventAndResult because I feel it still applies.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
File ui/events/blink/input_scroll_elasticity_controller_unittest.cc (right):

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:428: // Verify
that an stretch caused by a momentum scroll will switch to the
On 2016/03/08 14:28:53, tdresser wrote:
> an stretch -> a stretch

done; fixed copied comments that were the source of the error as well.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:551: // Verify
that an stretch opposing a scroll is correctly resolved.
On 2016/03/08 14:28:53, tdresser wrote:
> a stretch

Done; and fixed original call issue site.

https://codereview.chromium.org/1749343004/diff/80001/ui/events/blink/input_s...
ui/events/blink/input_scroll_elasticity_controller_unittest.cc:646: // and also
that the stretch amount isn't even ever changed.
On 2016/03/08 14:28:53, tdresser wrote:
> This comment is a bit confusing.
> 
> What's the difference between "doesn't change" and "isn't even ever changed"?

Done; removed confusing part.

[tdresser, 2016-03-09 14:30:06]

LGTM with nits.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right):

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:150: // because
the events generated will be a GSB (non-sythetic) and GSE
sythetic -> synthetic

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:158: // If no GSB
has been sent; it will be a non-syntehtic GSB.
syntehtic -> synthetic

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:175: // Generate
a synthetic GSE for every update to force hit testing so
Line wrapping is weird here. Also, the synthetic GSB is to force hit testing,
right? The synthetic GSE is just to pair with the synthetic GSB.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right):

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.h:20: // Current set
the default wheel transaction to 0ms until
"Current set" -> "Set"

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.h:91: bool
needs_scroll_begin_;
Can we add a comment on these?

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc
(right):

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:37:
struct CombinedEvent {
What about just storing a WebInputEvent?
You'd need a few extra casts, but it might be cleaner.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:270:
WebMouseWheelEvent::PhaseNone);
I still find this difficult to read, as the expected result is so far from where
we dispatch the wheel event.

That being said, the approach you've taken with the EXPECT_* macros is a nice
cleanup.

Let's discuss this in person.

https://codereview.chromium.org/1749343004/diff/100001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1749343004/diff/100001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:687: // the ScrollEnd when it is done.
Line wrapping is weird here.

[dtapuska, 2016-03-09 16:12:29]

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right):

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:150: // because
the events generated will be a GSB (non-sythetic) and GSE
On 2016/03/09 14:30:05, tdresser wrote:
> sythetic -> synthetic

Done.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:158: // If no GSB
has been sent; it will be a non-syntehtic GSB.
On 2016/03/09 14:30:05, tdresser wrote:
> syntehtic -> synthetic

Done.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:175: // Generate
a synthetic GSE for every update to force hit testing so
On 2016/03/09 14:30:05, tdresser wrote:
> Line wrapping is weird here. Also, the synthetic GSB is to force hit testing,
> right? The synthetic GSE is just to pair with the synthetic GSB.

Sorry; using xcode and git cl format caused the weird line wrap issue. 

The synthetic GSE won't always correspond to a synthetic GSB.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue.h (right):

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.h:20: // Current set
the default wheel transaction to 0ms until
On 2016/03/09 14:30:05, tdresser wrote:
> "Current set" -> "Set"

Done.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.h:91: bool
needs_scroll_begin_;
On 2016/03/09 14:30:06, tdresser wrote:
> Can we add a comment on these?

Done.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc
(right):

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:37:
struct CombinedEvent {
On 2016/03/09 14:30:06, tdresser wrote:
> What about just storing a WebInputEvent?
> You'd need a few extra casts, but it might be cleaner.

Done.

https://codereview.chromium.org/1749343004/diff/100001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:270:
WebMouseWheelEvent::PhaseNone);
On 2016/03/09 14:30:06, tdresser wrote:
> I still find this difficult to read, as the expected result is so far from
where
> we dispatch the wheel event.
> 
> That being said, the approach you've taken with the EXPECT_* macros is a nice
> cleanup.
> 
> Let's discuss this in person.

Done.

https://codereview.chromium.org/1749343004/diff/100001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1749343004/diff/100001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:687: // the ScrollEnd when it is done.
On 2016/03/09 14:30:06, tdresser wrote:
> Line wrapping is weird here.

Done.

[dtapuska, 2016-03-09 16:14:59]

Description was changed from

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dtapuska, 2016-03-09 16:14:59]

dtapuska@chromium.org changed reviewers:
+ aelias@chromium.org, creis@chromium.org

[tdresser, 2016-03-09 16:24:33]

Still LGTM, thanks!

[Charlie Reis, 2016-03-09 22:49:17]

Rubber stamp LGTM for content/, mainly deferring to tdresser's review.

CC'ing kenrb/wjmaclean for FYI, since they're thinking about these gestures for
OOPIFs.

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right):

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:149: // needs to
be sent; this event sequence doesn't need to be generated
nit: comma rather than semicolon (since first phrase isn't a full sentence)

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:151: //
(non-synthetic). This situation arrises when OSX generates double
nit: arises

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:158: // If no GSB
has been sent; it will be a non-synthetic GSB.
Same: use a comma here and below.

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/input...
File content/renderer/input/render_widget_input_handler_delegate.h (right):

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/input...
content/renderer/input/render_widget_input_handler_delegate.h:52: virtual void
ObserveGestureEventAndResult(
nit: Add a similar comment as above.

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/rende...
content/renderer/render_widget.cc:965: void
RenderWidget::ObserveGestureEventAndResult(
nit: Move above OnDidHandleKeyEvent.

[aelias_OOO_until_Jul13, 2016-03-10 19:22:54]

lgtm

[dtapuska, 2016-03-10 19:51:16]

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right):

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:149: // needs to
be sent; this event sequence doesn't need to be generated
On 2016/03/09 22:49:17, Charlie Reis wrote:
> nit: comma rather than semicolon (since first phrase isn't a full sentence)

Done.

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:151: //
(non-synthetic). This situation arrises when OSX generates double
On 2016/03/09 22:49:17, Charlie Reis wrote:
> nit: arises

Done.

https://codereview.chromium.org/1749343004/diff/120001/content/browser/render...
content/browser/renderer_host/input/mouse_wheel_event_queue.cc:158: // If no GSB
has been sent; it will be a non-synthetic GSB.
On 2016/03/09 22:49:17, Charlie Reis wrote:
> Same: use a comma here and below.

Done.

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/input...
File content/renderer/input/render_widget_input_handler_delegate.h (right):

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/input...
content/renderer/input/render_widget_input_handler_delegate.h:52: virtual void
ObserveGestureEventAndResult(
On 2016/03/09 22:49:17, Charlie Reis wrote:
> nit: Add a similar comment as above.

Done.

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1749343004/diff/120001/content/renderer/rende...
content/renderer/render_widget.cc:965: void
RenderWidget::ObserveGestureEventAndResult(
On 2016/03/09 22:49:17, Charlie Reis wrote:
> nit: Move above OnDidHandleKeyEvent.

Done.

[dtapuska, 2016-03-10 19:52:02]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-03-10 19:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
creis@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1749343004/#ps140001
(title: "Fix nits in patch set 7")

[commit-bot: I haz the power, 2016-03-10 19:53:07]

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

[commit-bot: I haz the power, 2016-03-10 21:09:02]

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

[commit-bot: I haz the power, 2016-03-10 21:09:04]

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_...)

[dtapuska, 2016-03-10 22:10:03]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-03-10 22:10:03]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
creis@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1749343004/#ps160001
(title: "Ensure only high precision scroll begins are used")

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

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

[commit-bot: I haz the power, 2016-03-10 23:10:08]

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

[commit-bot: I haz the power, 2016-03-10 23:10:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dtapuska, 2016-03-11 14:35:05]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-03-11 14:35:24]

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

[commit-bot: I haz the power, 2016-03-11 15:25:07]

Description was changed from

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-11 15:25:08]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-11 15:26:14]

Description was changed from

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement Wheel Gesture Scrolling on OSX.

Create wheel based gestures from mouse wheel event phases when
available. Pass overscroll into the input elasticity controller
to handle the elastic overflow scroll on OSX.

Transition the FrameWatcher to be a global IPC Message filter so
that it gets first crack at the messages (it is a passive
listener); because the RenderBrowserFilter was consuming the
FrameSwap messages causing the browser test to always fail on Mac.

BUG=587979
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1827dd232f84cc1143914fe89e375eb33c7d12d6
Cr-Commit-Position: refs/heads/master@{#380639}
==========

[commit-bot: I haz the power, 2016-03-11 15:26:15]

Patchset 9 (id:??) landed as
https://crrev.com/1827dd232f84cc1143914fe89e375eb33c7d12d6
Cr-Commit-Position: refs/heads/master@{#380639}