Bug 125604 – [wk2] Handle pinch-to-zoom gesture

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

For live magnification gestures.

[Tim Horton]

Created attachment 219013 [details]
take 1

[Tim Horton]

Created attachment 219017 [details]
maybe take 2 will apply

[Sam Weinig]

Comment on attachment 219013 [details]
take 1

View in context: https://bugs.webkit.org/attachment.cgi?id=219013&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:80
> +@property (readwrite) BOOL shouldHandleMagnificationGesture;

This should probably be _shouldHandleMagnificationGesture.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:236
> +    std::unique_ptr<ViewGestureController> _gestureController;

It might be better for this to be stored on the WebPageProxy instead.  I'm not 100% sure though, since it might not be generally useful outside of this WKView implementation.

> Source/WebKit2/UIProcess/ViewGestureController.cpp:129
> +    double newMagnification = MAX(m_magnification, 1);

Please use std::max() here (unless there is a good reason not to).

> Source/WebKit2/UIProcess/ViewGestureController.h:39
> +    static std::unique_ptr<ViewGestureController> create(WebPageProxy*);

You can just remove this and use std::make_unique<ViewGestureController>(page) instead.

> Source/WebKit2/UIProcess/ViewGestureController.h:70
> +    WebPageProxy* m_webPageProxy;

This should be a WebPageProxy&.

[Simon Fraser (smfr)]

Comment on attachment 219017 [details]
maybe take 2 will apply

View in context: https://bugs.webkit.org/attachment.cgi?id=219017&action=review

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:727
> +    PlatformCALayer* renderViewLayer = static_cast<GraphicsLayerCA*>(renderView->layer()->backing()->graphicsLayer())->platformCALayer();
> +    renderViewLayer->setTransform(transform);
> +    renderViewLayer->setAnchorPoint(FloatPoint3D());
> +    renderViewLayer->setPosition(FloatPoint3D());

Ew

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:743
> +    if (m_transientZoomScale != m_webPage->pageScaleFactor())
> +       static_cast<GraphicsLayerCA*>(renderView->layer()->backing()->graphicsLayer())->platformCALayer()->setTransform(TransformationMatrix());

What about your clobbering of anchor point and position?

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 219013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219013&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:80
> > +@property (readwrite) BOOL shouldHandleMagnificationGesture;
> 
> This should probably be _shouldHandleMagnificationGesture.

We don’t use underscores for properties. I suggest using something like

@property (readwrite, getter=_shouldHandleMagnificationGesture, setter=_setShouldHandleMagnificationGesture:) BOOL shouldHandleMagnificationGesture;

> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:236
> > +    std::unique_ptr<ViewGestureController> _gestureController;
> 
> It might be better for this to be stored on the WebPageProxy instead.  I'm not 100% sure though, since it might not be generally useful outside of this WKView implementation.

I think this should go on the WKView. It’s Mac specific code that is also NSView specific.

[Build Bot]

Comment on attachment 219017 [details]
maybe take 2 will apply

Attachment 219017 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48098101

[Build Bot]

Comment on attachment 219017 [details]
maybe take 2 will apply

Attachment 219017 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48078099

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 219013 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219013&action=review
> > 
> > > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:80
> > > +@property (readwrite) BOOL shouldHandleMagnificationGesture;
> > 
> > This should probably be _shouldHandleMagnificationGesture.
> 
> We don’t use underscores for properties.

I think we should use underscores for the names of private properties, so that we are free to introduce public properties with the same names (except for the underscores) with different semantics while maintaining source compatibility.

It’s true that when doing this, the default setter has a non-conventional name, so we should explicitly specify setter=_setShouldHandleMagnificationGesture.

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 219017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219017&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:727
> > +    PlatformCALayer* renderViewLayer = static_cast<GraphicsLayerCA*>(renderView->layer()->backing()->graphicsLayer())->platformCALayer();
> > +    renderViewLayer->setTransform(transform);
> > +    renderViewLayer->setAnchorPoint(FloatPoint3D());
> > +    renderViewLayer->setPosition(FloatPoint3D());
> 
> Ew

smfr suggests trying mucking with the GraphicsLayer and flushing just that layer, so I'm going to try that.

> > Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:743
> > +    if (m_transientZoomScale != m_webPage->pageScaleFactor())
> > +       static_cast<GraphicsLayerCA*>(renderView->layer()->backing()->graphicsLayer())->platformCALayer()->setTransform(TransformationMatrix());
> 
> What about your clobbering of anchor point and position?

They get restored magically when I scalePage() IIRC.

[Tim Horton]

(In reply to comment #5)
> (In reply to comment #3)
> > 
> > > Source/WebKit2/UIProcess/API/mac/WKView.mm:236
> > > +    std::unique_ptr<ViewGestureController> _gestureController;
> > 
> > It might be better for this to be stored on the WebPageProxy instead.  I'm not 100% sure though, since it might not be generally useful outside of this WKView implementation.
> 
> I think this should go on the WKView. It’s Mac specific code that is also NSView specific.

There's actually nothing Mac or NSView specific about ViewGestureController.

[Tim Horton]

Created attachment 219023 [details]
address review comments (except GraphicsLayer bit)

[Tim Horton]

Comment on attachment 219023 [details]
address review comments (except GraphicsLayer bit)

View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review

> Source/WebKit2/UIProcess/mac/TiledCoreAnimationDrawingAreaProxy.mm:161
> +void TiledCoreAnimationDrawingAreaProxy::endTransientZoom(double scale, FloatPoint origin)

smfr also mentioned that this should probably be "commit" since it does apply the pageScale

[Tim Horton]

(In reply to comment #9)
> (In reply to comment #4)
> > (From update of attachment 219017 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219017&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:727
> > > +    PlatformCALayer* renderViewLayer = static_cast<GraphicsLayerCA*>(renderView->layer()->backing()->graphicsLayer())->platformCALayer();
> > > +    renderViewLayer->setTransform(transform);
> > > +    renderViewLayer->setAnchorPoint(FloatPoint3D());
> > > +    renderViewLayer->setPosition(FloatPoint3D());
> > 
> > Ew
> 
> smfr suggests trying mucking with the GraphicsLayer and flushing just that layer, so I'm going to try that.

This goes very poorly because it causes the tile cache layer to be informed that its bounds are changing, leading to it marking itself for revalidateTiles (and also hitting a bunch of asserts).

[Simon Fraser (smfr)]

Comment on attachment 219023 [details]
address review comments (except GraphicsLayer bit)

View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review

r=me with some renaming.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:475
> +		0FA24D7A162DF91900A3F4C0 /* GraphicsLayerUpdater.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FA24D78162DF91900A3F4C0 /* GraphicsLayerUpdater.h */; settings = {ATTRIBUTES = (Private, ); }; };

Why?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1476
> +		49AF2D6914435D050016A784 /* DisplayRefreshMonitor.h in Headers */ = {isa = PBXBuildFile; fileRef = 49AF2D6814435D050016A784 /* DisplayRefreshMonitor.h */; settings = {ATTRIBUTES = (Private, ); }; };

Why?

> Source/WebKit2/ChangeLog:12
> +        Add ViewGestureController off of WKView, which currently only handles

"off of"

> Source/WebKit2/ChangeLog:14
> +        and origin and such, and forwards relevant events on to the DrawingArea

"and such"

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:98
> +- (void)_setShouldHandleMagnificationGesture:(BOOL)flag;

flag -> shouldHandle ?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:236
> +    std::unique_ptr<ViewGestureController> _gestureController;

Any reason not to just have one by value?

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:75
> +    virtual void endTransientZoom(double scale, WebCore::FloatPoint origin) { }

commitZoom?

> Source/WebKit2/UIProcess/ViewGestureController.cpp:75
> +FloatPoint ViewGestureController::scaledMagnificationOrigin(FloatPoint origin, double scale)

const FloatPoint&?

> Source/WebKit2/UIProcess/ViewGestureController.cpp:84
> +void ViewGestureController::didBeginTransientZoom(FloatRect visibleContentRect)

const FloatRect&

> Source/WebKit2/UIProcess/ViewGestureController.cpp:91
> +void ViewGestureController::handleMagnificationGesture(double scale, FloatPoint origin)

const FloatPoint&?

[Tim Horton]

(In reply to comment #14)
> (From update of attachment 219023 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review
> 
> const FloatPoint&?
> 
> > Source/WebKit2/UIProcess/ViewGestureController.cpp:84
> > +void ViewGestureController::didBeginTransientZoom(FloatRect visibleContentRect)
> 
> const FloatRect&
> 
> > Source/WebKit2/UIProcess/ViewGestureController.cpp:91
> > +void ViewGestureController::handleMagnificationGesture(double scale, FloatPoint origin)
> 
> const FloatPoint&?

Anders says no because they fit in a register :|

[Tim Horton]

Comment on attachment 219023 [details]
address review comments (except GraphicsLayer bit)

View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:97
> +- (BOOL)_shouldHandleMagnificationGesture;

anders/mitz say allowsMagnification

they also want a method to adjust the magnification (setMagnification:centeredAtPoint:)
and (magnification) property

[Tim Horton]

Comment on attachment 219023 [details]
address review comments (except GraphicsLayer bit)

View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:97
> +- (BOOL)_shouldHandleMagnificationGesture;

anders/mitz say allowsMagnification

they also want a method to adjust the magnification (setMagnification:centeredAtPoint:)
and (magnification) property

[Tim Horton]

(In reply to comment #17)
> (From update of attachment 219023 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:97
> > +- (BOOL)_shouldHandleMagnificationGesture;
> 
> anders/mitz say allowsMagnification
> 
> they also want a method to adjust the magnification (setMagnification:centeredAtPoint:)
> and (magnification) property
 (which uses the center as the origin)

[Tim Horton]

(In reply to comment #17)
> (From update of attachment 219023 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219023&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:97
> > +- (BOOL)_shouldHandleMagnificationGesture;
> 
> anders/mitz say allowsMagnification

and should use target conditionals to not appear on iOS

[Tim Horton]

Created attachment 219132 [details]
take 4 - addressing lots of review comments

[Tim Horton]

Comment on attachment 219132 [details]
take 4 - addressing lots of review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=219132&action=review

> Source/WebCore/WebCore.exp.in:780
> +__ZN7WebCore20TransformationMatrix9translateEdd
> +__ZN7WebCore20TransformationMatrix5scaleEd

these should be swapped

[Simon Fraser (smfr)]

Comment on attachment 219132 [details]
take 4 - addressing lots of review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=219132&action=review

> Source/WebKit2/UIProcess/API/mac/WKView.mm:3302
> +- (void)setMagnification:(double)magnification
> +{
> +    FloatPoint viewCenter(NSMidX([self bounds]), NSMidY([self bounds]));
> +    _data->_page->scalePage(magnification, roundedIntPoint(viewCenter));
> +}

Seems weird to arbitrarily zoom to the center.

> Source/WebKit2/UIProcess/mac/ViewGestureController.h:68
> +    double m_magnification;
> +    WebCore::FloatPoint m_magnificationOrigin;
> +    WebCore::FloatRect m_visibleContentRect;
> +    bool m_visibleContentRectIsValid;
> +
> +    WebPageProxy& m_webPageProxy;

Re-order to save (tiny) space.

[Tim Horton]

(In reply to comment #22)
> (From update of attachment 219132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219132&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:3302
> > +- (void)setMagnification:(double)magnification
> > +{
> > +    FloatPoint viewCenter(NSMidX([self bounds]), NSMidY([self bounds]));
> > +    _data->_page->scalePage(magnification, roundedIntPoint(viewCenter));
> > +}
> 
> Seems weird to arbitrarily zoom to the center.

I dunno. At first thought I assumed 0,0 was right, but:

a) NSScrollView uses the center
b) Imagine if Cmd-+ used this. Wouldn't it seem most reasonable if it zoomed to the center?

> > Source/WebKit2/UIProcess/mac/ViewGestureController.h:68
> > +    double m_magnification;
> > +    WebCore::FloatPoint m_magnificationOrigin;
> > +    WebCore::FloatRect m_visibleContentRect;
> > +    bool m_visibleContentRectIsValid;
> > +
> > +    WebPageProxy& m_webPageProxy;
> 
> Re-order to save (tiny) space.

[Tim Horton]

http://trac.webkit.org/changeset/160520

[Tim Horton]

Created attachment 219178 [details]
tiny mistake - not forwarding the event to superclasses if disabled

[Tim Horton]

reopen for tiny followup patch

[Tim Horton]

http://trac.webkit.org/changeset/160554

[Tim Horton]

<rdar://problem/15664239>