Enable pinch-zoom telemetry on Mac

Enable pinch-zoom telemetry on Mac

This change adds a SyntheticGestureTargetMac which is able to interpret the
touches sent from telemetry's pinch simulator and convert these into trackpad
zoom gestures.

BUG=
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

Committed: https://crrev.com/ecb20ba696fb953b2100b335fcea8a7922dfb993
Cr-Commit-Position: refs/heads/master@{#350316}

[ericrk, 2015-09-16 23:07:13]

ericrk@chromium.org changed reviewers:
+ dominikg@chromium.org, vmiura@chromium.org

[ericrk, 2015-09-16 23:07:13]

I'm adding in some code to allow our pinch gestures to work on mac (piped
through as a trackpad "pinch" using the zoom event in Mac OS).

I have a working version, but not sure about my approach. It's a bit annoying,
as the mac needs to compute the zoom amount from the touch points passed in, and
also adds some extra methods to represent a mouse with pinch.

One alternative is to make a new gesture type (zoom, vs pinch), or to make two
implementations of pinch. In either case, the implementation which calls a new
"zoom" entry point on the synthetic gesture target instead of passing touches...
not sure if that's better or not.. would be a bit more code.

Let me know what you think.

[vmiura, 2015-09-17 23:56:57]

vmiura@chromium.org changed reviewers:
+ nednguyen@google.com

[vmiura, 2015-09-17 23:56:58]

lgtm for telemetry change.

nednguyen@google.com would you mind to rubber stamp this?

[nednguyen, 2015-09-18 01:48:38]

lgtm

[nednguyen, 2015-09-18 01:49:21]

On 2015/09/18 01:48:38, nednguyen wrote:
> lgtm

This could break mac reference bot. Please coordinate with perf sherrif when you
land this.

[ericrk, 2015-09-18 17:20:31]

ericrk@chromium.org changed reviewers:
+ aelias@chromium.org
- dominikg@chromium.org

[ericrk, 2015-09-18 17:20:31]

+aelias for content/browser/renderer_host/input/ - please let me know if my
approach here seems reasonable (+see my previous comment for an alternative I
was considering)

[aelias_OOO_until_Jul13, 2015-09-18 18:13:16]

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

[aelias_OOO_until_Jul13, 2015-09-18 18:13:55]

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

[aelias_OOO_until_Jul13, 2015-09-18 18:13:55]

Jared is a better reviewer for this and may have stronger opinions on how this
should be written, replacing him as OWNERS for that.

[jdduke (slow), 2015-09-21 16:41:56]

Hmm, yeah I'm not enthusiastic about tricking the gesture pipeline into
forwarding touch pinch gestures that we hijack to create trackpad zoom events.

The key idea behind the synthetic gesture framework is  creating events that are
(potentially) visible to web content, but that also have default actions. In the
case of a touch pinch gesture, we want to actually be able to offer the
underlying touch events to the web content.

However, for touchpad pinch, we really only care about creating a stream of
wheel/touchpad events that would correspond to a pinch, as those are the only
events that would be visible to web content. For that case, I'm sympathetic to
creating a new gesture implementation that has touchpad output, as you
suggested. We could also use it for other platforms and I suspect the
implementation should be a lot simpler relative to the current touchscreen pinch
gesture.

On the browser side, we could call the existing pinch gesture
SyntheticTouchscreenPinchGesture and the new one SyntheticTouchpadPinchGesture.
We can reuse the same set of pinch params, we just conditionally create one or
the other in the browser depending on the source (mouse vs touch) type.
Thoughts?

[ericrk, 2015-09-21 16:50:20]

On 2015/09/21 16:41:56, jdduke wrote:
> Hmm, yeah I'm not enthusiastic about tricking the gesture pipeline into
> forwarding touch pinch gestures that we hijack to create trackpad zoom events.
> 
> The key idea behind the synthetic gesture framework is  creating events that
are
> (potentially) visible to web content, but that also have default actions. In
the
> case of a touch pinch gesture, we want to actually be able to offer the
> underlying touch events to the web content.
> 
> However, for touchpad pinch, we really only care about creating a stream of
> wheel/touchpad events that would correspond to a pinch, as those are the only
> events that would be visible to web content. For that case, I'm sympathetic to
> creating a new gesture implementation that has touchpad output, as you
> suggested. We could also use it for other platforms and I suspect the
> implementation should be a lot simpler relative to the current touchscreen
pinch
> gesture.
> 
> On the browser side, we could call the existing pinch gesture
> SyntheticTouchscreenPinchGesture and the new one
SyntheticTouchpadPinchGesture.
> We can reuse the same set of pinch params, we just conditionally create one or
> the other in the browser depending on the source (mouse vs touch) type.
> Thoughts?

This all sounds reasonable to me - thanks for the detailed feedback! Let me code
this up and see how it looks.

[ericrk, 2015-09-22 18:07:53]

Ok, re-worked this a bit to avoid reverse-engineering the zoom level from
generated touch points - we now have a SyntheticTouchpadPinchGesture which
generates pinch gestures directly.

There's still some more potential work (adding new py/js paths for touchpad vs
touchscreen), but this seemed like a good step one to get out (and is fully
functional).

[jdduke (slow), 2015-09-22 18:52:37]

Thanks, this looks pretty reasonable. Does the zoom in speed appear constant
visually or does it appear to accelerate/decelerate?

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

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture.cc:45: if
(SyntheticGestureParams::IsGestureSourceTypeSupported(
Hmm, in theory |gesture_params| can specify either mouse or default, which may
or may not be touch.

We could have a thin SyntheticPinchGesture wrapper, which instantiates the right
implementation lazily (in the first SyntheticPinchGesture::ForwardInputEvents
call), depending on |gesture_params| as well as
|target->GetDefaultSyntheticGestureSourceType()|.

Alternatively, we could get rid of the abstract
GetDefaultSyntheticGestureSourceType() and instead make it a static method on
SyntheticGestureParams (which would allow us to instantiate the right
implementation here).

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

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchpad_pinch_gesture.cc:10:
#include "ui/events/latency_info.h"
Do we need the latency_info include?

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchpad_pinch_gesture.cc:136:
params_.relative_pointer_speed_in_pixels_s));
Hmm, yeah, this parameter should really have just been a |duration| from the
beginning. Sigh. For now I think it's fine but I think we'll eventually want to
refactor it. I guess there's already a TODO to use a separate set of params,
maybe add another TODO here or at the call site that the scale duration should
be provided?

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h
(right):

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
For moved files do we update the copyright year? I can never remember.

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h:5:
#ifndef
CONTENT_BROWSER_RENDERER_HOST_INPUT_synthetic_touchscreen_pinch_gesture_H_
Nit: Uppercase.

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h:67:
#endif  //
CONTENT_BROWSER_RENDERER_HOST_INPUT_synthetic_touchscreen_pinch_gesture_H_
Uppercase

[ericrk, 2015-09-22 21:29:23]

Patchset #6 (id:100001) has been deleted

[ericrk, 2015-09-22 21:31:53]

Patchset #6 (id:120001) has been deleted

[ericrk, 2015-09-22 21:37:11]

Patchset #6 (id:140001) has been deleted

[ericrk, 2015-09-22 21:44:00]

Patchset #6 (id:160001) has been deleted

[ericrk, 2015-09-22 21:46:27]

Thanks for the feedback! PTAL

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

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture.cc:45: if
(SyntheticGestureParams::IsGestureSourceTypeSupported(
On 2015/09/22 18:52:37, jdduke wrote:
> Hmm, in theory |gesture_params| can specify either mouse or default, which may
> or may not be touch.
> 
> We could have a thin SyntheticPinchGesture wrapper, which instantiates the
right
> implementation lazily (in the first SyntheticPinchGesture::ForwardInputEvents
> call), depending on |gesture_params| as well as
> |target->GetDefaultSyntheticGestureSourceType()|.
> 
> Alternatively, we could get rid of the abstract
> GetDefaultSyntheticGestureSourceType() and instead make it a static method on
> SyntheticGestureParams (which would allow us to instantiate the right
> implementation here).

Makes sense. Went with the thin wrapper approach, as it seemed like pulling
GetDefaultSyntheticGestureSourceType into SyntheticGestureParams as a static
would require additional #ifdef type logic, which seemed a bit uglier? both
would work fine I think.

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

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchpad_pinch_gesture.cc:10:
#include "ui/events/latency_info.h"
On 2015/09/22 18:52:37, jdduke wrote:
> Do we need the latency_info include?

nope.

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchpad_pinch_gesture.cc:136:
params_.relative_pointer_speed_in_pixels_s));
On 2015/09/22 18:52:37, jdduke wrote:
> Hmm, yeah, this parameter should really have just been a |duration| from the
> beginning. Sigh. For now I think it's fine but I think we'll eventually want
to
> refactor it. I guess there's already a TODO to use a separate set of params,
> maybe add another TODO here or at the call site that the scale duration should
> be provided?

Done - filed a bug as well.

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h
(right):

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/09/22 18:52:37, jdduke wrote:
> For moved files do we update the copyright year? I can never remember.

The only thing I see in our c++ guidelines is "The year should be the current
year when a new file is created, and should not be changed when the file is
updated thereafter." - I could see this going either way w.r.t. moves, but I'd
probably lean towards not updating?

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h:5:
#ifndef
CONTENT_BROWSER_RENDERER_HOST_INPUT_synthetic_touchscreen_pinch_gesture_H_
On 2015/09/22 18:52:37, jdduke wrote:
> Nit: Uppercase.

Done.

https://codereview.chromium.org/1349813002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.h:67:
#endif  //
CONTENT_BROWSER_RENDERER_HOST_INPUT_synthetic_touchscreen_pinch_gesture_H_
On 2015/09/22 18:52:37, jdduke wrote:
> Uppercase

Done.

[jdduke (slow), 2015-09-22 21:58:49]

lgtm, thanks.

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1311:
params.gesture_source_type = SyntheticGestureParams::TOUCH_INPUT;
What are we validating in these 4 tests? I guess we validate the touch vs
gesture output in the targets?

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
File content/browser/renderer_host/input/synthetic_gesture_target_mac.h (right):

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_gesture_target_mac.h:17: explicit
SyntheticGestureTargetMac(RenderWidgetHostImpl* host,
Nit: Not sure we need "explicit" for multi-arg constructors?

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:27:
DCHECK(source_type != SyntheticGestureParams::DEFAULT_INPUT);
Nit: DCHECK_NE

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:31:
DCHECK(source_type == SyntheticGestureParams::MOUSE_INPUT);
Nit: DCHECK_EQ

[ericrk, 2015-09-22 22:08:49]

Thanks!

Ned - when you said "This could break mac reference bot. Please coordinate with
perf sherrif when you
land this.", can the introduction of a new telemetry metric break a bot?

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1311:
params.gesture_source_type = SyntheticGestureParams::TOUCH_INPUT;
On 2015/09/22 21:58:49, jdduke wrote:
> What are we validating in these 4 tests? I guess we validate the touch vs
> gesture output in the targets?

The targets will fail if they receive the wrong events (touch vs pinch gesture)
- does this seem OK? Or should i find a way to be more explicit?

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
File content/browser/renderer_host/input/synthetic_gesture_target_mac.h (right):

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_gesture_target_mac.h:17: explicit
SyntheticGestureTargetMac(RenderWidgetHostImpl* host,
On 2015/09/22 21:58:49, jdduke wrote:
> Nit: Not sure we need "explicit" for multi-arg constructors?

we don't - wish there was a lint check for this one, guess I should write a CL
:D

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:27:
DCHECK(source_type != SyntheticGestureParams::DEFAULT_INPUT);
On 2015/09/22 21:58:49, jdduke wrote:
> Nit: DCHECK_NE

Done.

https://codereview.chromium.org/1349813002/diff/180001/content/browser/render...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:31:
DCHECK(source_type == SyntheticGestureParams::MOUSE_INPUT);
On 2015/09/22 21:58:49, jdduke wrote:
> Nit: DCHECK_EQ

Done.

[ericrk, 2015-09-22 22:09:02]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-09-22 22:09:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, vmiura@chromium.org, jdduke@chromium.org
Link to the patchset: https://codereview.chromium.org/1349813002/#ps200001
(title: "feedback")

[ericrk, 2015-09-22 22:09:04]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-09-22 22:09:04]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-09-22 22:11:51]

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

[commit-bot: I haz the power, 2015-09-22 22:17:20]

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

[commit-bot: I haz the power, 2015-09-22 22:17:21]

Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL)

[ericrk, 2015-09-22 23:12:24]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-09-22 23:12:48]

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

[commit-bot: I haz the power, 2015-09-22 23:18:38]

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

[commit-bot: I haz the power, 2015-09-22 23:18:39]

Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL)

[ericrk, 2015-09-23 01:41:34]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-09-23 01:43:36]

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

[commit-bot: I haz the power, 2015-09-23 06:12:19]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-09-23 06:13:20]

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