Synthetic pinch gesture take scale factor as parameter.

Synthetic pinch gesture take scale factor as parameter.

Instead of a zoom direction and the number of pixels to be covered by the pinch
gesture, the gesture now takes a scale factor as parameter. From that, it
computes how to perform the gesture: the ratio of the final span and the initial
span equals the scale factor (modulo touch slop). The spans must not fall below
a certain threshold, provided by the gesture target (GetMinScalingSpanInDips).

Update the JS interface (chrome.gpuBenchmarking.pinchBy) to reflect the changes.
For (temporary) backwards compatibility, add another method to the interface to
tell Telemetry whether or not the browser supports the new interface. This is to
avoid crashes when Telemetry is run with the current stable version. Will be
removed once the stable version supports the new interface.

Update tough_pinch_zoom_cases page set to make sure all pinch gestures start
within the web content (tested on N4 and N10).

Update synthetic gesture controller unit tests to use the new parameter.

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

[Dominik Grewe, 2014-05-19 13:45:50]

ptal, thanks.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (left):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:94: // needed for
negative values.
I can't see a reason for flooring the values any more. Looks like the input
event coordinates have been updated to use floats now. Or am I missing something
else here?

[Sami, 2014-05-19 14:49:04]

Thanks Dominik, just a few nits below.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:200:
total_num_pixels_covered_(0),
Do we still need total_num_pixel_covered here?

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:261:
EXPECT_TRUE(false);
NOTREACHED();

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:127:
CHECK(false) << "Touch events not supported for this browser.";
This could be a NOTREACHED() too. Maybe also leave out the text description from
the binary since it's not really useful for anyone who doesn't know this code
already.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (left):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:94: // needed for
negative values.
On 2014/05/19 13:45:50, Dominik Grewe wrote:
> I can't see a reason for flooring the values any more. Looks like the input
> event coordinates have been updated to use floats now. Or am I missing
something
> else here?

I think we're in the middle of making input coordinates floats or maybe have
done so already. In that world there shouldn't be a need to do flooring, so if
this works already then great.

https://codereview.chromium.org/293683002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/pinch.py (right):

https://codereview.chromium.org/293683002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/pinch.py:26: raise
page_action.PageActionNotSupported("""
Nit: use single instead of triple quotes since otherwise the exception message
will have funny indentation and newlines in it.

[jdduke (slow), 2014-05-19 15:21:10]

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target.h (right):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target.h:45: virtual int
GetTouchSlopInDips() const = 0;
We should be returning floats for anything in DIPs.  There are no guarantees
that the Android constants will be integral.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (left):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:94: // needed for
negative values.
On 2014/05/19 14:49:04, Sami wrote:
> On 2014/05/19 13:45:50, Dominik Grewe wrote:
> > I can't see a reason for flooring the values any more. Looks like the input
> > event coordinates have been updated to use floats now. Or am I missing
> something
> > else here?
> 
> I think we're in the middle of making input coordinates floats or maybe have
> done so already. In that world there shouldn't be a need to do flooring, so if
> this works already then great.

Have we verified that this works with the (old/current) Aura GR? I'm not quite
as familiar with how they compute pinch scale. Fractional coords should work
just fine in the unified GR.

https://codereview.chromium.org/293683002/diff/20001/content/common/input/syn...
File content/common/input/synthetic_pinch_gesture_params.h (right):

https://codereview.chromium.org/293683002/diff/20001/content/common/input/syn...
content/common/input/synthetic_pinch_gesture_params.h:26: int
relative_pointer_speed_in_pixels_s;
This speed isn't quite as intuitive now.  Not really sure what the best
alternative is, perhaps a total time or scale delta per second? Not a big deal
though.

[Dominik Grewe, 2014-05-19 17:31:42]

Thanks.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:200:
total_num_pixels_covered_(0),
On 2014/05/19 14:49:04, Sami wrote:
> Do we still need total_num_pixel_covered here?

I'm only using for the NoScaling unit test to make sure we're not moving in that
case. But I guess zoom_direction_ = UNKNOWN should tell us that already. I'll
remove it.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:261:
EXPECT_TRUE(false);
On 2014/05/19 14:49:04, Sami wrote:
> NOTREACHED();

Done.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target.h (right):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target.h:45: virtual int
GetTouchSlopInDips() const = 0;
On 2014/05/19 15:21:11, jdduke wrote:
> We should be returning floats for anything in DIPs.  There are no guarantees
> that the Android constants will be integral.

Done.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:127:
CHECK(false) << "Touch events not supported for this browser.";
On 2014/05/19 14:49:04, Sami wrote:
> This could be a NOTREACHED() too. Maybe also leave out the text description
from
> the binary since it's not really useful for anyone who doesn't know this code
> already.

Done.

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (left):

https://codereview.chromium.org/293683002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:94: // needed for
negative values.
On 2014/05/19 15:21:11, jdduke wrote:
> On 2014/05/19 14:49:04, Sami wrote:
> > On 2014/05/19 13:45:50, Dominik Grewe wrote:
> > > I can't see a reason for flooring the values any more. Looks like the
input
> > > event coordinates have been updated to use floats now. Or am I missing
> > something
> > > else here?
> > 
> > I think we're in the middle of making input coordinates floats or maybe have
> > done so already. In that world there shouldn't be a need to do flooring, so
if
> > this works already then great.
> 
> Have we verified that this works with the (old/current) Aura GR? I'm not quite
> as familiar with how they compute pinch scale. Fractional coords should work
> just fine in the unified GR.

We're only running pinch gestures on Android at the moment, and it's working
fine there. We rarely ever achieve the exact scale factor we want any way, so I
guess if it's a bit inaccurate we don't care too much.

https://codereview.chromium.org/293683002/diff/20001/content/common/input/syn...
File content/common/input/synthetic_pinch_gesture_params.h (right):

https://codereview.chromium.org/293683002/diff/20001/content/common/input/syn...
content/common/input/synthetic_pinch_gesture_params.h:26: int
relative_pointer_speed_in_pixels_s;
On 2014/05/19 15:21:11, jdduke wrote:
> This speed isn't quite as intuitive now.  Not really sure what the best
> alternative is, perhaps a total time or scale delta per second? Not a big deal
> though.

Yeah, not sure here either. I'm against having a total time, I'd rather have
some kind of velocity so it's straightforward to have the same speed regardless
of the scale factor.

I'm not quite sure how a scale delta per second would work. How would you deal
with scale factors > 1 vs scale factors < 1? Would you have to specify the
velocity differently in each case?

https://codereview.chromium.org/293683002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/pinch.py (right):

https://codereview.chromium.org/293683002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/pinch.py:26: raise
page_action.PageActionNotSupported("""
On 2014/05/19 14:49:04, Sami wrote:
> Nit: use single instead of triple quotes since otherwise the exception message
> will have funny indentation and newlines in it.

Done.

[Sami, 2014-05-20 17:11:49]

Thanks Dominik. lgtm!

[jdduke (slow), 2014-05-20 17:25:03]

Oops, sorry didn't see the response. lgtm.

[Dominik Grewe, 2014-05-20 17:34:32]

Thanks. Adding piman for review of content/

[Dominik Grewe, 2014-05-20 17:35:39]

+kenrb for security review of content/common/input_messages.h

[kenrb, 2014-05-20 19:27:31]

lgtm

[piman, 2014-05-20 21:10:11]

https://codereview.chromium.org/293683002/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/293683002/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:142: return
ui::GestureConfiguration::min_distance_for_pinch_scroll_in_pixels();
No DIPs for Aura?

[jdduke (slow), 2014-05-20 21:13:01]

https://codereview.chromium.org/293683002/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/293683002/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:142: return
ui::GestureConfiguration::min_distance_for_pinch_scroll_in_pixels();
On 2014/05/20 21:10:11, piman wrote:
> No DIPs for Aura?

Aura "pixels" are DIPs =/ It's a sad, sad world we live in...

[piman, 2014-05-20 21:15:46]

LGTM then, but o.O

[Dominik Grewe, 2014-05-21 08:41:07]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-05-21 08:42:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominikg@chromium.org/293683002/30001

[Dominik Grewe, 2014-05-21 12:28:29]

The CQ bit was unchecked by dominikg@chromium.org

[Dominik Grewe, 2014-05-21 13:15:23]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:57:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominikg@chromium.org/293683002/50001

[commit-bot: I haz the power, 2014-05-22 09:48:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10890)

[commit-bot: I haz the power, 2014-05-22 15:49:33]

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

[commit-bot: I haz the power, 2014-05-22 15:49:34]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11121)

[Dominik Grewe, 2014-05-22 17:03:41]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:05:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominikg@chromium.org/293683002/70001

[commit-bot: I haz the power, 2014-05-22 23:13:14]

Change committed as 272359