Make Mac pinch thresholding apply only to zoom (Chromium side)

Make Mac pinch thresholding apply only to zoom (Chromium side)

Add a zoomDisabled flag (disabled so that it will default to false
everywhere except where we set it) to WebGestureEvent.

Note that the equivalent blink::Event does not have a corresponding
gesture sub-type that sends pinch scale information, so the
zoomDisabled information is not passed on either.

Blink side:
https://codereview.chromium.org/1127013002/

BUG=478981
Committed: https://crrev.com/0a8c2754afbae766a5db45ead34d0154e8b03997
Cr-Commit-Position: refs/heads/master@{#329266}

[ccameron, 2015-05-06 03:53:05]

ccameron@chromium.org changed reviewers:
+ aelias@chromium.org, jdduke@chromium.org

[ccameron, 2015-05-06 03:53:22]

This is the Chromium side of
https://codereview.chromium.org/1127013002

[aelias_OOO_until_Jul13, 2015-05-06 23:20:24]

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:2293: // If more
than 1 second has passed since the last pinch gesture ended, reset
I don't like timeouts as a rule.  Why not reset the threshold immediately on
pinchEnd?

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:2430:
pinchUnusedAmount_ += [event magnification];
Please make this multiplicative.  Otherwise this will behave differently
depending on the speed of the gesture and how exactly it was divided into
events.  Making it multiplicative will consistently threshold at the same
overall magnitude.

[jdduke (slow), 2015-05-07 01:53:50]

Perhaps add an additional assertion or two to one of the existing
InputHandlerProxy tests? Just to make sure somebody doesn't come in and muck
with this.

https://codereview.chromium.org/1129693002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1129693002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_proxy.cc:284: return DID_HANDLE;
What about DROP_EVENT? Should have a similar effect but maybe slightly more
informative?

[ccameron, 2015-05-07 17:09:06]

Thanks -- added some tests, too.

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:2293: // If more
than 1 second has passed since the last pinch gesture ended, reset
On 2015/05/06 23:20:24, aelias wrote:
> I don't like timeouts as a rule.  Why not reset the threshold immediately on
> pinchEnd?

If someone is doing several pinches in a row, to zoom to the maximum (or a
higher) level, they will re-start the gesture once their fingers are spread too
far apart. It feels really awkward to have each begin/end block re-require
thresholding.

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:2430:
pinchUnusedAmount_ += [event magnification];
On 2015/05/06 23:20:24, aelias wrote:
> Please make this multiplicative.  Otherwise this will behave differently
> depending on the speed of the gesture and how exactly it was divided into
> events.  Making it multiplicative will consistently threshold at the same
> overall magnitude.

Done. I had made it additive to match the earlier thresholding math.
Empirically, the 40% additive matches a 50% multiplicative.

https://codereview.chromium.org/1129693002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/1129693002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_proxy.cc:284: return DID_HANDLE;
On 2015/05/07 01:53:50, jdduke wrote:
> What about DROP_EVENT? Should have a similar effect but maybe slightly more
> informative?

Sure -- updated. I was originally thinking DROP, but wanted to minimize changes.

[jdduke (slow), 2015-05-07 17:14:53]

content/renderer/input lgtm.

[aelias_OOO_until_Jul13, 2015-05-07 18:16:16]

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:2293: // If more
than 1 second has passed since the last pinch gesture ended, reset
On 2015/05/07 at 17:09:05, ccameron wrote:
> On 2015/05/06 23:20:24, aelias wrote:
> > I don't like timeouts as a rule.  Why not reset the threshold immediately on
> > pinchEnd?
> 
> If someone is doing several pinches in a row, to zoom to the maximum (or a
higher) level, they will re-start the gesture once their fingers are spread too
far apart. It feels really awkward to have each begin/end block re-require
thresholding.

I see.  How about checking if page_scale_factor == minimum_scale from the
CompositorFrameMetadata instead of using the timeout?  It seems like you want to
threshold when not currently zoomed in, but if you're already zoomed in then you
want it to be easier to pinch.

[ccameron, 2015-05-07 18:55:04]

On 2015/05/07 18:16:16, aelias wrote:
>
https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/1129693002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_view_mac.mm:2293: // If more
> than 1 second has passed since the last pinch gesture ended, reset
> On 2015/05/07 at 17:09:05, ccameron wrote:
> > On 2015/05/06 23:20:24, aelias wrote:
> > > I don't like timeouts as a rule.  Why not reset the threshold immediately
on
> > > pinchEnd?
> > 
> > If someone is doing several pinches in a row, to zoom to the maximum (or a
> higher) level, they will re-start the gesture once their fingers are spread
too
> far apart. It feels really awkward to have each begin/end block re-require
> thresholding.
> 
> I see.  How about checking if page_scale_factor == minimum_scale from the
> CompositorFrameMetadata instead of using the timeout?  It seems like you want
to
> threshold when not currently zoomed in, but if you're already zoomed in then
you
> want it to be easier to pinch.

I was thinking that, but it occurred to me that if zooms in to a paragraph (e.g,
via double-tap), they will probably scroll, and they will not want to be more
susceptible to pinch for those scrolls.

[aelias_OOO_until_Jul13, 2015-05-07 19:19:35]

On 2015/05/07 at 18:55:04, ccameron wrote:
> I was thinking that, but it occurred to me that if zooms in to a paragraph
(e.g, via double-tap), they will probably scroll, and they will not want to be
more susceptible to pinch for those scrolls.

Yes, but they probably want to zoom back out afterwards, and we shouldn't make
that hard via thresholding right?

I just think timeouts make messy code and unpredictable UX (because if you wait
slightly longer, it mysteriously behaves differently).  The min-zoom is easier
to understand I feel.  Users will realize that the zoom level tends to "stick"
to the minimum scale, but is looser otherwise.

[ccameron, 2015-05-11 17:08:55]

On 2015/05/07 19:19:35, aelias wrote:
> On 2015/05/07 at 18:55:04, ccameron wrote:
> > I was thinking that, but it occurred to me that if zooms in to a paragraph
> (e.g, via double-tap), they will probably scroll, and they will not want to be
> more susceptible to pinch for those scrolls.
> 
> Yes, but they probably want to zoom back out afterwards, and we shouldn't make
> that hard via thresholding right?
> 
> I just think timeouts make messy code and unpredictable UX (because if you
wait
> slightly longer, it mysteriously behaves differently).  The min-zoom is easier
> to understand I feel.  Users will realize that the zoom level tends to "stick"
> to the minimum scale, but is looser otherwise.

Well, I didn't like this idea, then I implemented it, and it turns out I do like
it.

Uploaded -- ptal.

[aelias_OOO_until_Jul13, 2015-05-11 18:32:32]

lgtm modulo minor comments.

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

https://codereview.chromium.org/1129693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1472:
page_is_at_scale_one_ = (frame->metadata.page_scale_factor == 1);
Please check for "== min_page_scale_factor" instead, that's a bit more generic
and future-proof, and rename the variable to page_at_minimum_scale_.

https://codereview.chromium.org/1129693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2291: // If more
than 1 second has passed since the last pinch gesture ended, reset
This comment is out of date.

[ccameron, 2015-05-11 19:21:43]

Thanks -- done -- will merge once it appears to work on TOT.

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

https://codereview.chromium.org/1129693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1472:
page_is_at_scale_one_ = (frame->metadata.page_scale_factor == 1);
On 2015/05/11 18:32:32, aelias wrote:
> Please check for "== min_page_scale_factor" instead, that's a bit more generic
> and future-proof, and rename the variable to page_at_minimum_scale_.

If we end up doing the rubber-band bounce for pinch-zoom, then we will probably
want one. But, setting it to minimum page scale for now.

https://codereview.chromium.org/1129693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2291: // If more
than 1 second has passed since the last pinch gesture ended, reset
On 2015/05/11 18:32:32, aelias wrote:
> This comment is out of date.

Done.

[ccameron, 2015-05-11 20:01:14]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-05-11 20:01:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1129693002/#ps80001
(title: "Fix compile")

[commit-bot: I haz the power, 2015-05-11 20:02:07]

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

[commit-bot: I haz the power, 2015-05-11 20:09:35]

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

[commit-bot: I haz the power, 2015-05-11 20:09:37]

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

[aelias_OOO_until_Jul13, 2015-05-11 21:22:34]

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

https://codereview.chromium.org/1129693002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1472:
page_is_at_scale_one_ = (frame->metadata.page_scale_factor == 1);
On 2015/05/11 19:21:43, ccameron wrote:
> On 2015/05/11 18:32:32, aelias wrote:
> > Please check for "== min_page_scale_factor" instead, that's a bit more
generic
> > and future-proof, and rename the variable to page_at_minimum_scale_.
> 
> If we end up doing the rubber-band bounce for pinch-zoom, then we will
probably
> want one. But, setting it to minimum page scale for now.

If we do, I would probably favor storing the "overpinched" scale in a separate
field, like you did for scrolling and for similar reasons, so this would still
work.

[ccameron, 2015-05-11 22:34:00]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-05-11 22:35:17]

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

[commit-bot: I haz the power, 2015-05-11 23:11:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-11 23:12:33]

Patchset 5 (id:??) landed as
https://crrev.com/0a8c2754afbae766a5db45ead34d0154e8b03997
Cr-Commit-Position: refs/heads/master@{#329266}