Bug 125752

Summary: WebKit2 View Gestures: Implement smartMagnifyWithEvent: and make it work
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, mitz, sam, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch andersca: review+

Description Tim Horton 2013-12-14 20:14:10 PST
This is 'double-tap-to-zoom-on-an-element'.
Comment 1 Tim Horton 2013-12-14 20:39:21 PST
<rdar://problem/15664245>
Comment 2 Tim Horton 2013-12-20 04:25:28 PST
Created attachment 219750 [details]
patch
Comment 3 Anders Carlsson 2013-12-20 12:45:23 PST
Comment on attachment 219750 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.h:30
> +#include <WebCore/FloatPoint.h>

No need to include this here, just forward declare FloatPoint.
Comment 4 Simon Fraser (smfr) 2013-12-20 12:51:48 PST
Comment on attachment 219750 [details]
patch

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

> Source/WebKit2/UIProcess/mac/ViewGestureController.cpp:142
> +static float maximumRectangleComponentDelta(FloatRect a, FloatRect b)

Does not compute.

> Source/WebKit2/UIProcess/mac/ViewGestureController.cpp:147
> +void ViewGestureController::didCollectGeometryForSmartMagnificationGesture(FloatPoint origin, FloatRect renderRect, FloatRect visibleContentRect, bool elementIsReplaced)

Maybe isReplacedElement to make it a bit clearer that this is using the CSS "replaced" terminology.

> Source/WebKit2/UIProcess/mac/ViewGestureController.cpp:149
> +    double currentMagnification = m_webPageProxy.pageScaleFactor();

currentScaleFactor?

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:74
> +    HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowShadowContent);
> +    HitTestResult hitTestResult = HitTestResult(scrolledOrigin);
> +    m_webPage.mainFrameView()->renderView()->hitTest(request, hitTestResult);
> +
> +    if (hitTestResult.innerNode()) {
> +        bool isReplaced;
> +        FloatRect renderRect = hitTestResult.innerNode()->renderRect(&isReplaced);
> +        m_webPage.send(Messages::ViewGestureController::DidCollectGeometryForSmartMagnificationGesture(origin, renderRect, visibleContentRect, isReplaced));
> +    }

Does this find things in subframes?
Comment 5 Tim Horton 2013-12-20 13:08:29 PST
(In reply to comment #4)
> (From update of attachment 219750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219750&action=review
> 
> > Source/WebKit2/UIProcess/mac/ViewGestureController.cpp:142
> > +static float maximumRectangleComponentDelta(FloatRect a, FloatRect b)
> 
> Does not compute.

We talked about this on IRC. I agree!

> > Source/WebKit2/UIProcess/mac/ViewGestureController.cpp:147
> > +void ViewGestureController::didCollectGeometryForSmartMagnificationGesture(FloatPoint origin, FloatRect renderRect, FloatRect visibleContentRect, bool elementIsReplaced)
> 
> Maybe isReplacedElement to make it a bit clearer that this is using the CSS "replaced" terminology.

Sure.

> > Source/WebKit2/UIProcess/mac/ViewGestureController.cpp:149
> > +    double currentMagnification = m_webPageProxy.pageScaleFactor();
> 
> currentScaleFactor?

Sure.

> > Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:74
> > +    HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowShadowContent);
> > +    HitTestResult hitTestResult = HitTestResult(scrolledOrigin);
> > +    m_webPage.mainFrameView()->renderView()->hitTest(request, hitTestResult);
> > +
> > +    if (hitTestResult.innerNode()) {
> > +        bool isReplaced;
> > +        FloatRect renderRect = hitTestResult.innerNode()->renderRect(&isReplaced);
> > +        m_webPage.send(Messages::ViewGestureController::DidCollectGeometryForSmartMagnificationGesture(origin, renderRect, visibleContentRect, isReplaced));
> > +    }
> 
> Does this find things in subframes?

No, nor does the existing implementation. We just zoom on the frame instead. Possible room for improvement! (I actually had it returning subframe results at some point, but not mapping the points into the root document space, so it would zoom somewhere wacky).
Comment 6 Tim Horton 2013-12-20 13:58:23 PST
http://trac.webkit.org/changeset/160923