Add touchpad pinch zoom support to Blink

Add touchpad pinch zoom support to Blink

If a touchpad pinch zoom event is not handled, and the synthetic wheel
event is not handled, then do a zoom based on the pich.

Always mark the event as having been handled because otherwise, content
will do a content-scale when the events are no longer handled (once this
lands and is rolled, the content-scale can be removed from content).

BUG=451559
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189460

[ccameron, 2015-01-30 21:08:15]

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

[ccameron, 2015-01-30 21:08:16]

Almost-final part for enabling pinch zoom on Mac (the rest is cleanup -- again
see email for patch slicing).

[aelias_OOO_until_Jul13, 2015-01-30 21:21:40]

https://codereview.chromium.org/889803004/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2155: setPageScaleFactor(oldPageScale *
magnifyDelta);
Please avoid this call to setPageScaleFactor, since setting scale and scroll in
separate calls can sometimes cause subtle clamping bugs.  Instead, you can do
"float newPageScale = clampPageScaleFactorToLimits(oldPageScale *
magnifyDelta())", then call pinchViewport.setScaleAndLocation() instead of
move().

[ccameron, 2015-02-02 23:59:07]

Thanks -- updated.

https://codereview.chromium.org/889803004/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:2155: setPageScaleFactor(oldPageScale *
magnifyDelta);
On 2015/01/30 21:21:40, aelias wrote:
> Please avoid this call to setPageScaleFactor, since setting scale and scroll
in
> separate calls can sometimes cause subtle clamping bugs.  Instead, you can do
> "float newPageScale = clampPageScaleFactorToLimits(oldPageScale *
> magnifyDelta())", then call pinchViewport.setScaleAndLocation() instead of
> move().

Done.

[aelias_OOO_until_Jul13, 2015-02-03 00:16:58]

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2160: setPageScaleFactorAndLocation(newPageScale,
newLocation);
Sorry, I noticed another issue.  Please scroll the normal FrameView scroll
offset first, and only apply the unused portion to the location in
setPageScaleFactorAndLocation.  That will match the current logic in
LayerTreeHostImpl::PinchGestureUpdate.  That will allow pinch zooming out to not
get clamped in a weird way, among other things.

[bokan, 2015-02-03 00:44:57]

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

[bokan, 2015-02-03 00:45:13]

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2156: const FloatPoint anchor(pinchEvent.x,
pinchEvent.y);
Drive-by: Could you please put this logic inside PinchViewport in a method that
takes the magnifyDelta and anchor?

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2160: setPageScaleFactorAndLocation(newPageScale,
newLocation);
Though almost dead, please be aware of the old pinch-path and guard necessary
calls behind the pinchVirtualViewportEnabled() flag. As-is
setPageScaleFactorAndLocation will ASSERT in the old-path. I'll be removing it
by the end of this quarter.

pinchViewportOffset() will return 0 in the old path which I think is fine in
this case.

[ccameron, 2015-02-03 02:29:09]

Thanks --updated.

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2156: const FloatPoint anchor(pinchEvent.x,
pinchEvent.y);
On 2015/02/03 00:45:12, bokan wrote:
> Drive-by: Could you please put this logic inside PinchViewport in a method
that
> takes the magnifyDelta and anchor?

I don't see where the scale factor limits are available in PinchViewport, so I
had the function take the new scale factor, and the caller compute the two (the
caller can early-out and not return true if they are different). LMK if I missed
a way to do this entirely in PinchViewport.

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2160: setPageScaleFactorAndLocation(newPageScale,
newLocation);
On 2015/02/03 00:16:58, aelias wrote:
> Sorry, I noticed another issue.  Please scroll the normal FrameView scroll
> offset first, and only apply the unused portion to the location in
> setPageScaleFactorAndLocation.  That will match the current logic in
> LayerTreeHostImpl::PinchGestureUpdate.  That will allow pinch zooming out to
not
> get clamped in a weird way, among other things.

Done (in the PinchVirtualViewport version).

[aelias_OOO_until_Jul13, 2015-02-03 02:37:14]

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2156: const FloatPoint anchor(pinchEvent.x,
pinchEvent.y);
On 2015/02/03 02:29:08, ccameron wrote:
> On 2015/02/03 00:45:12, bokan wrote:
> > Drive-by: Could you please put this logic inside PinchViewport in a method
> that
> > takes the magnifyDelta and anchor?
> 
> I don't see where the scale factor limits are available in PinchViewport, so I
> had the function take the new scale factor, and the caller compute the two
(the
> caller can early-out and not return true if they are different). LMK if I
missed
> a way to do this entirely in PinchViewport.

You can plumb in the information from WebViewImpl by adding an extra method to
ChromeClientImpl (which you can see in this file is reachable from FrameHost). 
I think it would be nicer to have the PinchViewport method take a delta.

[bokan, 2015-02-03 03:05:16]

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2156: const FloatPoint anchor(pinchEvent.x,
pinchEvent.y);
On 2015/02/03 02:29:08, ccameron wrote:
> On 2015/02/03 00:45:12, bokan wrote:
> > Drive-by: Could you please put this logic inside PinchViewport in a method
> that
> > takes the magnifyDelta and anchor?
> 
> I don't see where the scale factor limits are available in PinchViewport, so I
> had the function take the new scale factor, and the caller compute the two
(the
> caller can early-out and not return true if they are different). LMK if I
missed
> a way to do this entirely in PinchViewport.

Right, sorry, forgot about that. You can take Alexandre's suggestion above.

https://codereview.chromium.org/889803004/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.h (right):

https://codereview.chromium.org/889803004/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.h:110: void setScaleAroundAnchor(float scale,
const FloatPoint& anchor);
Could you also add a comment or name anchor so it's clear what coordinates it's
in? I assume window?

https://codereview.chromium.org/889803004/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2152: if (newPageScale != oldPageScale) {
Please fold this "changed" check inside the method.

[ccameron, 2015-02-03 17:03:07]

Patchset #4 (id:60001) has been deleted

[ccameron, 2015-02-03 17:03:29]

Shifted things around (liking this version better). Ptal.

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2156: const FloatPoint anchor(pinchEvent.x,
pinchEvent.y);
On 2015/02/03 03:05:16, bokan wrote:
> On 2015/02/03 02:29:08, ccameron wrote:
> > On 2015/02/03 00:45:12, bokan wrote:
> > > Drive-by: Could you please put this logic inside PinchViewport in a method
> > that
> > > takes the magnifyDelta and anchor?
> > 
> > I don't see where the scale factor limits are available in PinchViewport, so
I
> > had the function take the new scale factor, and the caller compute the two
> (the
> > caller can early-out and not return true if they are different). LMK if I
> missed
> > a way to do this entirely in PinchViewport.
> 
> Right, sorry, forgot about that. You can take Alexandre's suggestion above. 

Done (yeah, I prefer this too -- thanks for pointing out how we get back to
WebViewImpl from PinchViewport).

https://codereview.chromium.org/889803004/diff/40001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.h (right):

https://codereview.chromium.org/889803004/diff/40001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.h:110: void setScaleAroundAnchor(float scale,
const FloatPoint& anchor);
On 2015/02/03 03:05:16, bokan wrote:
> Could you also add a comment or name anchor so it's clear what coordinates
it's
> in? I assume window?

Done.

https://codereview.chromium.org/889803004/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/889803004/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2152: if (newPageScale != oldPageScale) {
On 2015/02/03 03:05:16, bokan wrote:
> Please fold this "changed" check inside the method.

Done.

[ccameron, 2015-02-03 17:06:57]

Patchset #4 (id:80001) has been deleted

[ccameron, 2015-02-03 17:07:21]

(re-uploaded, last upload was messed up)

[aelias_OOO_until_Jul13, 2015-02-03 19:10:11]

lgtm modulo comment below

https://codereview.chromium.org/889803004/diff/100001/Source/core/frame/Pinch...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/889803004/diff/100001/Source/core/frame/Pinch...
Source/core/frame/PinchViewport.cpp:243: FrameView* view = mainFrame()->view();
I'm not confident FrameView is guaranteed to exist when a pinch event arrives. 
Please add an early-return at the top of this method:

if (!mainFrame() || !mainFrame()->view())
  return false;

[bokan, 2015-02-03 20:17:00]

lgtm

[ccameron, 2015-02-03 21:49:17]

Thanks!

[ccameron, 2015-02-03 22:17:06]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-02-03 22:18:20]

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

[commit-bot: I haz the power, 2015-02-03 22:31:23]

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

[commit-bot: I haz the power, 2015-02-03 22:31:24]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[ccameron, 2015-02-03 22:32:47]

ccameron@chromium.org changed reviewers:
+ rbyers@chromium.org

[ccameron, 2015-02-03 22:32:47]

Adding rbyers for Source/core owner.

[Rick Byers, 2015-02-03 22:35:06]

On 2015/02/03 22:32:47, ccameron wrote:
> Adding rbyers for Source/core owner.

Tests?

[ccameron, 2015-02-04 00:07:23]

New patchsets have been uploaded after l-g-t-m from bokan@chromium.org

[ccameron, 2015-02-04 00:07:44]

Tests! (updated)

[Rick Byers, 2015-02-04 01:31:41]

Thanks!  Source/core LGTM
Just a couple nits on the test - feel free to land when they're addressed.

https://codereview.chromium.org/889803004/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/gesture-pinch-zoom.html (right):

https://codereview.chromium.org/889803004/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/gesture-pinch-zoom.html:3: <div
onmousewheel="mouseWheel(event)" style="margin: 0px 0; width: 100px; height:
100px; background-color: blue;"></div>
nit: use 'onwheel' instead of 'onmousewheel'.  'mousewheel' events are
non-standard and deprecated, 'wheel' is the standard API.

https://codereview.chromium.org/889803004/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/gesture-pinch-zoom.html:11:
shouldBeTrue('event.ctrlKey');
nit: Please also validate the co-ordinates and the delta values in the event. 
IIRC the tests I had in chromium validated a bunch of that - just want to avoid
loosing test coverage in the move.

[ccameron, 2015-02-04 02:00:43]

New patchsets have been uploaded after l-g-t-m from rbyers@chromium.org

[ccameron, 2015-02-04 02:01:03]

Thanks -- updated.

https://codereview.chromium.org/889803004/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/gesture-pinch-zoom.html (right):

https://codereview.chromium.org/889803004/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/gesture-pinch-zoom.html:3: <div
onmousewheel="mouseWheel(event)" style="margin: 0px 0; width: 100px; height:
100px; background-color: blue;"></div>
On 2015/02/04 01:31:40, Rick Byers wrote:
> nit: use 'onwheel' instead of 'onmousewheel'.  'mousewheel' events are
> non-standard and deprecated, 'wheel' is the standard API.

Done.

https://codereview.chromium.org/889803004/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/gesture-pinch-zoom.html:11:
shouldBeTrue('event.ctrlKey');
On 2015/02/04 01:31:40, Rick Byers wrote:
> nit: Please also validate the co-ordinates and the delta values in the event. 
> IIRC the tests I had in chromium validated a bunch of that - just want to
avoid
> loosing test coverage in the move.

Done.

[ccameron, 2015-02-04 02:01:09]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-02-04 02:01:32]

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

[commit-bot: I haz the power, 2015-02-04 04:27:48]

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

[commit-bot: I haz the power, 2015-02-04 04:27:49]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41183)

[ccameron, 2015-02-04 05:21:54]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-02-04 05:22:39]

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

[Rick Byers, 2015-02-04 05:26:47]

Thanks! still LGTM

https://codereview.chromium.org/889803004/diff/160001/LayoutTests/fast/events...
File LayoutTests/fast/events/gesture-pinch-fake-mouse-wheel.html (right):

https://codereview.chromium.org/889803004/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/gesture-pinch-fake-mouse-wheel.html:17:
shouldBeCloseTo('event.deltaY', 100 * Math.log(1 / testScale), tolerance);
Oh, I forgot you already added this test!  Sorry I wouldn't have insisted on
duplicating this logic in your new test if I had remembered.  I think it's fine
to have done that though.

[commit-bot: I haz the power, 2015-02-04 06:17:43]

Committed patchset #7 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189460