| Summary: | [wk2] Handle pinch-to-zoom gesture | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||
| Component: | WebKit2 | Assignee: | Tim Horton <thorton> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | andersca, bdakin, buildbot, commit-queue, esprehn+autocc, glenn, kondapallykalyan, mitz, mkwst, rniwa, sam, simon.fraser, webkit-bug-importer | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Tim Horton
2013-12-11 15:05:09 PST
Created attachment 219013 [details]
take 1
Created attachment 219017 [details]
maybe take 2 will apply
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&. 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? (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. 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 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. (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. (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. Created attachment 219023 [details]
address review comments (except GraphicsLayer bit)
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 (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). 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&? (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 :| 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 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 (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) (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 Created attachment 219132 [details]
take 4 - addressing lots of review comments
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 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. (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. Created attachment 219178 [details]
tiny mistake - not forwarding the event to superclasses if disabled
reopen for tiny followup patch |