Synthesize ctrl-wheel events on touchpad pinch

Synthesize ctrl-wheel events on touchpad pinch

On Windows, pinch gestures on a touchpad typically sends wheel events
with the ctrlKey modifier set.  This CL makes Chrome on Mac do the same
thing so that pages that are 'naturally zoomable' (eg. maps) get a
chance to override the pinch behavior before we use it to do browser
zoom.  Getting browser zoom on pinch is a long standing source of
confusion for users of Google Maps.  See http://goo.gl/84CTaJ for
discussion.

To be compatible with existing uses of wheel events, the deltaY value
in the wheel event is computed as -100*log(scale).  So zooming in 2x is
about -70 and zooming out 2x is 70. To compute a scale factor from this
value in JavaScript use 'Math.exp(-deltaY / 100)'.  See demo at
http://jsbin.com/qiyaseza/.

We expect to eventually want this change on ChromeOS as well (once
ChromeOS wires pinch up to pinch-zoom), and so this transformation
belongs in InputRouter.

The trickiest part of this change is handling the WheelEvent ACK
messages properly.  If the wheel was synthesized from a
GesturePinchUpdate, we need to convert the ACK back to the right type.
To do this I attach a 'synthesized_from_pinch' bit to each
WebWheelEvent in InputRouter's coalesced_mouse_wheel_events_ and to the
current_wheel_event_.  Then when I get a wheel ACK I use this bit on
current_wheel_event_ to decide how it should be handled.

This removes some old code which flushed any pending wheel events
whenever we got a non-wheel event.  Doing that causes us to ignore (or
possibly misattribute) the ACKs we'll eventually get back from the
renderer for these events.  This was probably already responsible for
some bugs with the Overscroll controller (which relies on
current_wheel_event representing THE event that we received an ACK for)
and  would be very problematic for the pinch-derived wheels where
(since the GestureEventQueue expects to reliably get an ack per event).
I tracked this code back to http://crbug.com/154740 where it was added
to cope with the fact that we were discarding pending wheel events.  I
think a better fix for that is to just continue to queue and send wheel
events, even when there are other events happening (as we do for
interleaving touch and gesture events with other events).

BUG=289887
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267822

[Rick Byers, 2014-05-01 14:24:01]

Jared, PTAL.

[jdduke (slow), 2014-05-01 15:44:47]

I think my only worry is completely removing the wheel flush.  Surely the
original clearing code was there for a reason (before the flushing code,
https://chromiumcodereview.appspot.com/11411016/diff/5003/content/browser/ren...
Do we have any idea what that reason was?

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:213: // it in with
other wheel events as that would make the ACK handling
I'm starting to think maintaining separate Touchpad and Touchscreen gesture
queues is the way to go.  The event streams look sufficiently different, and it
would clean up some of the weird one-off touchpad solutions we have (like for
tap suppression with mouse events).  It would also let us be stricter about
validating gesture event streams.  That probably wouldn't simplify this patch
very much, but it's something to think about.

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:446: if
(input_event.type == WebInputEvent::GesturePinchUpdate) {
Can you explain to me why it's important for the view to distinguish between a
GesturePinchUpdate from a touchpad, and a MouseWheel event with the ControlKey
modifier?  Would they have different behaviors in the browser if they go
unhandled?

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:403: if
(input_event.type == WebInputEvent::GesturePinchUpdate) {
Can we move this to |SendGestureEventImmediately|.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:533: wheelEvent.deltaY
= 100.0f * (1.0f - pinch_event.data.pinchUpdate.scale);
Maybe DCHECK that the scale is positive? Is the magnification percentage the
general convention here? (it's probably the only thing that makes sense, I'm
just wondering if there's precedent?)

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:683: ACK_SOURCE_NONE);
ACK_SOURCE_NONE -> current_ack_source_

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:690:
mouse_wheel_pending_ = false;
Could you put a note that the order this bool is set is important, e.g., ack'ing
the pinch could trigger forwarding of another pinch, so keeping it true prevents
forwarding another wheel event within the ack dispatch?

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:786:
InputRouterImpl::QueuedWheelEvent::QueuedWheelEvent() {
synthesized_from_pinch(false)

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.h (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.h:122: // and it's latency
info.
its

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:540:
EXPECT_EQ(-16, wheel_event->deltaY);  // coalesced
Maybe write this as |-10 + -6| for clarity?

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1485: //
Verify a second wheel event was sent represending the 2nd and 3rd pinch
representing

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1518: // Event
2: enqueued in InputRouter
Might as well slap periods on the end of each comment for consistency.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1574: //
Verify we sent #4
#4.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/mock_input_ack_handler.h (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/mock_input_ack_handler.h:73:
blink::WebInputEvent::Type ack_event_type_;
Initialize in the constructor to |WebInputEvent::Undefined|.

https://codereview.chromium.org/250923004/diff/110001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.h (right):

https://codereview.chromium.org/250923004/diff/110001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.h:55:
blink::WebGestureEvent::SourceDevice sourceDevice);
Hmm, I'd say |source_device| but it looks like we're already not very consistent
with the casing in some of these tests, oh well.

[jdduke (slow), 2014-05-01 16:26:01]

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:213: // it in with
other wheel events as that would make the ACK handling
On 2014/05/01 15:44:47, jdduke wrote:
> I'm starting to think maintaining separate Touchpad and Touchscreen gesture
> queues is the way to go.  The event streams look sufficiently different, and
it
> would clean up some of the weird one-off touchpad solutions we have (like for
> tap suppression with mouse events).  It would also let us be stricter about
> validating gesture event streams.  That probably wouldn't simplify this patch
> very much, but it's something to think about.

This is very much a future thing :)

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:446: if
(input_event.type == WebInputEvent::GesturePinchUpdate) {
On 2014/05/01 15:44:47, jdduke wrote:
> Can you explain to me why it's important for the view to distinguish between a
> GesturePinchUpdate from a touchpad, and a MouseWheel event with the ControlKey
> modifier?  Would they have different behaviors in the browser if they go
> unhandled?

As per offline discussion, nevermind this comment.

[Rick Byers, 2014-05-01 16:54:34]

On 2014/05/01 15:44:47, jdduke wrote:
> I think my only worry is completely removing the wheel flush.  Surely the
> original clearing code was there for a reason (before the flushing code,
>
https://chromiumcodereview.appspot.com/11411016/diff/5003/content/browser/ren...)
> Do we have any idea what that reason was?

Yeah I tried to track it down but it's been there for awhile.  I suspect the
motivation is similar to the logic for dropping mouse moves: if we get behind we
can simply ignore the events.  I feel this is misguided.  It may be OK for
general scrolling, but wheel is used for other things (especially in USB mice -
it's essentially a button like a keypress).  We allow taps, key presses, and
button clicks to be queued forever if necessary, why not coalesced wheel events?
 We could have a broader discussion on input-dev - see if we can find anyone who
can argue why we'd really want this.  Or we can just try it and see how it goes.
 Which do you prefer?

[jdduke (slow), 2014-05-01 17:02:13]

On 2014/05/01 16:54:34, Rick Byers wrote:
> On 2014/05/01 15:44:47, jdduke wrote:
> > I think my only worry is completely removing the wheel flush.  Surely the
> > original clearing code was there for a reason (before the flushing code,
> >
>
https://chromiumcodereview.appspot.com/11411016/diff/5003/content/browser/ren...)
> > Do we have any idea what that reason was?
> 
> Yeah I tried to track it down but it's been there for awhile.  I suspect the
> motivation is similar to the logic for dropping mouse moves: if we get behind
we
> can simply ignore the events.  I feel this is misguided.  It may be OK for
> general scrolling, but wheel is used for other things (especially in USB mice
-
> it's essentially a button like a keypress).  We allow taps, key presses, and
> button clicks to be queued forever if necessary, why not coalesced wheel
events?
>  We could have a broader discussion on input-dev - see if we can find anyone
who
> can argue why we'd really want this.  Or we can just try it and see how it
goes.
>  Which do you prefer?

Oh I'm definitely in agreement with you, and the originator is probably long
gone (or no longer remembers :). Maybe just put a comment somewhere relevant in
reference to the original bug (not sure where the best place is, I'll leave that
up to you).

[Rick Byers, 2014-05-01 17:32:03]

On 2014/05/01 17:02:13, jdduke wrote:
> On 2014/05/01 16:54:34, Rick Byers wrote:
> > On 2014/05/01 15:44:47, jdduke wrote:
> > > I think my only worry is completely removing the wheel flush.  Surely the
> > > original clearing code was there for a reason (before the flushing code,
> > >
> >
>
https://chromiumcodereview.appspot.com/11411016/diff/5003/content/browser/ren...)
> > > Do we have any idea what that reason was?
> > 
> > Yeah I tried to track it down but it's been there for awhile.  I suspect the
> > motivation is similar to the logic for dropping mouse moves: if we get
behind
> we
> > can simply ignore the events.  I feel this is misguided.  It may be OK for
> > general scrolling, but wheel is used for other things (especially in USB
mice
> -
> > it's essentially a button like a keypress).  We allow taps, key presses, and
> > button clicks to be queued forever if necessary, why not coalesced wheel
> events?
> >  We could have a broader discussion on input-dev - see if we can find anyone
> who
> > can argue why we'd really want this.  Or we can just try it and see how it
> goes.
> >  Which do you prefer?
> 
> Oh I'm definitely in agreement with you, and the originator is probably long
> gone (or no longer remembers :). Maybe just put a comment somewhere relevant
in
> reference to the original bug (not sure where the best place is, I'll leave
that
> up to you).

Tracked it back over 4 years to when mousewheel coalescing was first added:
https://codereview.chromium.org/536059.  Kind of interesting to see the history
of coalescing queues in chromium (patch originally kept only a single event!),
but there's no specific discussion of this question.  I think it's safe to
assume we understand this better now after years of experience with similar
approaches (of course I'm always prepared to be surprised).

[Rick Byers, 2014-05-01 17:34:56]

Thanks.

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:213: // it in with
other wheel events as that would make the ACK handling
On 2014/05/01 16:26:01, jdduke wrote:
> On 2014/05/01 15:44:47, jdduke wrote:
> > I'm starting to think maintaining separate Touchpad and Touchscreen gesture
> > queues is the way to go.  The event streams look sufficiently different, and
> it
> > would clean up some of the weird one-off touchpad solutions we have (like
for
> > tap suppression with mouse events).  It would also let us be stricter about
> > validating gesture event streams.  That probably wouldn't simplify this
patch
> > very much, but it's something to think about.
> 
> This is very much a future thing :)

Agreed.  The gesture streams are as distinct as mouse and touch streams are. 
But I agree it's mostly orthogonal to this change.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:403: if
(input_event.type == WebInputEvent::GesturePinchUpdate) {
On 2014/05/01 15:44:47, jdduke wrote:
> Can we move this to |SendGestureEventImmediately|.

With a bit of refactoring, yes.  That's better - thanks!

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:533: wheelEvent.deltaY
= 100.0f * (1.0f - pinch_event.data.pinchUpdate.scale);
On 2014/05/01 15:44:47, jdduke wrote:
> Maybe DCHECK that the scale is positive? 

Done.

> Is the magnification percentage the
> general convention here? (it's probably the only thing that makes sense, I'm
> just wondering if there's precedent?)

I don't think there's precedent (windows doesn't define the conversion
precisely) but the choices are pretty constrained.  I actually posted another
patch before you reviewed this which uses a different formula (after playing
with many scenarios and devices), but I explained the constraints in detail. 
WDYT?

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:683: ACK_SOURCE_NONE);
On 2014/05/01 15:44:47, jdduke wrote:
> ACK_SOURCE_NONE -> current_ack_source_

Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:690:
mouse_wheel_pending_ = false;
On 2014/05/01 15:44:47, jdduke wrote:
> Could you put a note that the order this bool is set is important, e.g.,
ack'ing
> the pinch could trigger forwarding of another pinch, so keeping it true
prevents
> forwarding another wheel event within the ack dispatch?

Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:786:
InputRouterImpl::QueuedWheelEvent::QueuedWheelEvent() {
On 2014/05/01 15:44:47, jdduke wrote:
> synthesized_from_pinch(false)

Thanks!  Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.h (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.h:122: // and it's latency
info.
On 2014/05/01 15:44:47, jdduke wrote:
> its

Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:540:
EXPECT_EQ(-16, wheel_event->deltaY);  // coalesced
On 2014/05/01 15:44:47, jdduke wrote:
> Maybe write this as |-10 + -6| for clarity?

Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1485: //
Verify a second wheel event was sent represending the 2nd and 3rd pinch
On 2014/05/01 15:44:47, jdduke wrote:
> representing

Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1518: // Event
2: enqueued in InputRouter
On 2014/05/01 15:44:47, jdduke wrote:
> Might as well slap periods on the end of each comment for consistency.

Done.  I think our style guide even requires (or at least encourages) this
somewhere.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1574: //
Verify we sent #4
On 2014/05/01 15:44:47, jdduke wrote:
> #4.

Done.

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/input/mock_input_ack_handler.h (right):

https://codereview.chromium.org/250923004/diff/110001/content/browser/rendere...
content/browser/renderer_host/input/mock_input_ack_handler.h:73:
blink::WebInputEvent::Type ack_event_type_;
On 2014/05/01 15:44:47, jdduke wrote:
> Initialize in the constructor to |WebInputEvent::Undefined|.

Done.

https://codereview.chromium.org/250923004/diff/110001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.h (right):

https://codereview.chromium.org/250923004/diff/110001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.h:55:
blink::WebGestureEvent::SourceDevice sourceDevice);
On 2014/05/01 15:44:47, jdduke wrote:
> Hmm, I'd say |source_device| but it looks like we're already not very
consistent
> with the casing in some of these tests, oh well.

Sigh.  I find it hard enough moving between blink and chromium coding style,
it's even worse when you have a file in chromium making extensive use of blink
types (with, eg. WebGestureEvent::sourceDevice) <grin>.  Anyway I've updated the
param names to match chromium convention.

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:520: //
https://crbug.com/154740.
Added comment referencing that rubber-banding bug here as you suggested.

[jdduke (slow), 2014-05-01 17:53:26]

This lgtm with a couple nits and assuming it works as originally desired with
Google Maps?

https://codereview.chromium.org/250923004/diff/130001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/250923004/diff/130001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:135: float
scaleToDelta(float scale) {
PinchScaleToWheelDelta

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:540:
DCHECK(pinch_event.event.data.pinchUpdate.scale >= 0);
Super nit: DCHECK_GT

[Rick Byers, 2014-05-01 18:11:43]

> This lgtm with a couple nits and assuming it works as originally desired with
> Google Maps?

Thanks.  Yes, Google Maps is actually an easy case - looks like they look only
at ticks, not the deltas.  It feels good (better than wheel scrolling actually
due to the lack of fling).

+Sadrul for OWNERS of
content/browser/renderer_host/render_widget_host_unittest.cc

https://codereview.chromium.org/250923004/diff/130001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/250923004/diff/130001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:135: float
scaleToDelta(float scale) {
On 2014/05/01 17:53:27, jdduke wrote:
> PinchScaleToWheelDelta

Done.

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:540:
DCHECK(pinch_event.event.data.pinchUpdate.scale >= 0);
On 2014/05/01 17:53:27, jdduke wrote:
> Super nit: DCHECK_GT

Done, except it's DCHECK_GE

[jdduke (slow), 2014-05-01 18:51:19]

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:540:
DCHECK(pinch_event.event.data.pinchUpdate.scale >= 0);
On 2014/05/01 18:11:44, Rick Byers wrote:
> On 2014/05/01 17:53:27, jdduke wrote:
> > Super nit: DCHECK_GT
> 
> Done, except it's DCHECK_GE
> 

We should never get a scale of 0... And log(0) is undefined.

[Rick Byers, 2014-05-01 19:54:36]

On 2014/05/01 18:51:19, jdduke wrote:
>
https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/250923004/diff/150001/content/browser/rendere...
> content/browser/renderer_host/input/input_router_impl.cc:540:
> DCHECK(pinch_event.event.data.pinchUpdate.scale >= 0);
> On 2014/05/01 18:11:44, Rick Byers wrote:
> > On 2014/05/01 17:53:27, jdduke wrote:
> > > Super nit: DCHECK_GT
> > 
> > Done, except it's DCHECK_GE
> > 
> 
> We should never get a scale of 0... And log(0) is undefined.

As discussed offline, with touchpad hitting 0 (or Infinity) was technically
previously possible.  I've changed this to make it impossible and included a
test, and updated this DCHECK to be strictly >0.

[jdduke (slow), 2014-05-01 19:58:13]

epsilon check lgtm, thanks.

[sadrul, 2014-05-02 01:46:12]

lgtm

[jdduke (slow), 2014-05-02 01:57:47]

So I was reading through the referenced bug where you describe generating a
synthetic wheel event in the render process.  Is there a reason we're not doing
that? That is, why can't we just send the GesturePinchUpdate events with
source==Touchpad  to the renderer? InputHandlerProxy would just forward them to
the main thread, and blink would synthesize a wheel update with the ctrl
modifier, along the lines of synthesizing a mouse click from GestureTap.  

I vaguely recall our discussion here but now I've forgotten and I can't seem to
find any arguments here for doing it in the browser process.

[jdduke (slow), 2014-05-02 02:09:47]

On 2014/05/02 01:57:47, jdduke wrote:
> So I was reading through the referenced bug where you describe generating a
> synthetic wheel event in the render process.  Is there a reason we're not
doing
> that? That is, why can't we just send the GesturePinchUpdate events with
> source==Touchpad  to the renderer? InputHandlerProxy would just forward them
to
> the main thread, and blink would synthesize a wheel update with the ctrl
> modifier, along the lines of synthesizing a mouse click from GestureTap.  
> 
> I vaguely recall our discussion here but now I've forgotten and I can't seem
to
> find any arguments here for doing it in the browser process.

I'd be more sympathetic to this (wheel synthesis in the InputRouter) approach if
we did hit testing in the compositor for wheel events with the ctrl modifier,
but I don't believe we do.

[jdduke (slow), 2014-05-02 03:04:08]

On 2014/05/02 02:09:47, jdduke wrote:
> On 2014/05/02 01:57:47, jdduke wrote:
> > So I was reading through the referenced bug where you describe generating a
> > synthetic wheel event in the render process.  Is there a reason we're not
> doing
> > that? That is, why can't we just send the GesturePinchUpdate events with
> > source==Touchpad  to the renderer? InputHandlerProxy would just forward them
> to
> > the main thread, and blink would synthesize a wheel update with the ctrl
> > modifier, along the lines of synthesizing a mouse click from GestureTap.  
> > 
> > I vaguely recall our discussion here but now I've forgotten and I can't seem
> to
> > find any arguments here for doing it in the browser process.
> 
> I'd be more sympathetic to this (wheel synthesis in the InputRouter) approach
if
> we did hit testing in the compositor for wheel events with the ctrl modifier,
> but I don't believe we do.

OK, sorry for the chatter, I recall now the argument that we eventually want to
be able to handle touchpad GesturePinchUpdate's in the compositor, but they need
to be offered to the main thread first (assuming there's a wheel handler).

[Rick Byers, 2014-05-02 03:15:22]

On 2014/05/02 02:09:47, jdduke wrote:
> On 2014/05/02 01:57:47, jdduke wrote:
> > So I was reading through the referenced bug where you describe generating a
> > synthetic wheel event in the render process.  Is there a reason we're not
> doing
> > that? That is, why can't we just send the GesturePinchUpdate events with
> > source==Touchpad  to the renderer? InputHandlerProxy would just forward them
> to
> > the main thread, and blink would synthesize a wheel update with the ctrl
> > modifier, along the lines of synthesizing a mouse click from GestureTap.  
> > 
> > I vaguely recall our discussion here but now I've forgotten and I can't seem
> to
> > find any arguments here for doing it in the browser process.
> 
> I'd be more sympathetic to this (wheel synthesis in the InputRouter) approach
if
> we did hit testing in the compositor for wheel events with the ctrl modifier,
> but I don't believe we do.

The reason we can't easily do it that way as I first planned is that the next
step here is to enable trackpad control over pinch-zoom (which happens on impl).
 We have no path for sending an ACK from main to impl so we need the browser
process to mediate these two uses (first sending a wheel to main, then if
unhandled sending a pinch to impl).  We could invent some third type or another
flag instead or something.  But I don't think its fundamentally any simpler.  As
it is now I think it'll be easy to wire in pinch-zoom support.  We can of course
revisit as we go - shouldn't be hard to move later if we think its better.

I also think we can tighten up the hit test in CC.  There was just a TODO from a
jamesr that made me nervous (we're sending wheels to main even when we think
there is no handler due to "synchronization bugs").  But cc should know reliably
when a layer has no wheel handlers, so could ACK immediately in those cases
regardless of modifiers.  We had such bugs with touch hit testing but have
ironed them out.  Wheel testing should be strictly easier.

[Rick Byers, 2014-05-02 03:30:20]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-05-02 03:30:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rbyers@chromium.org/250923004/220001

[commit-bot: I haz the power, 2014-05-02 03:53:52]

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

[commit-bot: I haz the power, 2014-05-02 03:53:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Rick Byers, 2014-05-02 11:30:18]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-05-02 11:30:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rbyers@chromium.org/250923004/220001

[commit-bot: I haz the power, 2014-05-02 17:00:03]

Change committed as 267822