Bug 129146 – WebKit2 View Gestures (Smart Magnification): Support for iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

<rdar://problem/16032668>

[Tim Horton]

Created attachment 224899 [details]
patch

[Tim Horton]

Created attachment 224902 [details]
rebase

[Tim Horton]

Created attachment 224903 [details]
rebase

[Tim Horton]

Created attachment 224904 [details]
zomgrebase

[WebKit Commit Bot]

Attachment 224904 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/ViewGestureController.h:96:  The parameter name "scrollView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 224904 [details]
zomgrebase

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

> Source/WebKit2/UIProcess/API/ios/WKContentView.mm:330
> +    [_interactionView _setDoubleTapGestureRecognizer:recognizer];

Nope!

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:137
> +    toImpl([_contentView _pageRef])->setShouldRecordNavigationSnapshots(allowsBackForwardNavigationGestures);

Space after [. This is now [_contentView page]->

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:262
> +    WebPageProxy *webPageProxy = toImpl([_contentView _pageRef]);

Changed.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:265
> +    UIGestureRecognizer *recognizer = _gestureController->installSmartMagnificationHandler(_contentView.get(), _scrollView.get());

Odd that we call it magnification not zoom.

> Source/WebKit2/UIProcess/mac/ViewGestureController.h:164
> +    RetainPtr<WKSmartMagnificationGestureController> m_smartMagnificationGestureController;
> +    RetainPtr<UIScrollView> m_smartMagnificationScrollView;
> +    RetainPtr<UIView> m_smartMagnificationTargetView;

Who owns the gesture controller? Are we creating ref cycles here?

[Benjamin Poulain]

Comment on attachment 224904 [details]
zomgrebase

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

I'd like to discuss if there is a way to untangle stuff if you have 5 minutes.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:175
> +#if PLATFORM(IOS)
> +    _gestureController = std::make_unique<WebKit::ViewGestureController>(*_page);
> +
> +    UIGestureRecognizer *recognizer = _gestureController->installSmartMagnificationHandler(_contentView.get(), _scrollView.get());
> +    [_contentView _setDoubleTapGestureRecognizer:recognizer];
> +#endif
> +

I don't think this is the right place. Why would the smart magnification know anything about the scrollview?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:475
> +

Blank line.

> Source/WebKit2/UIProcess/API/ios/WKContentView.mm:332
> +- (void)_setDoubleTapGestureRecognizer:(UIGestureRecognizer *)recognizer
> +{
> +    [_interactionView _setDoubleTapGestureRecognizer:recognizer];
> +}
> +

You can remove this, no _interactionView anymore :)

> Source/WebKit2/UIProcess/API/ios/WKContentViewInternal.h:79
> +- (void)_setDoubleTapGestureRecognizer:(UIGestureRecognizer *)recognizer;

This is the wrong header. WKContentViewInternal is/was for API used from bellow (WebPageProxy, PageClient, etc). WKContentView.h is the API for WKView/WKWebView.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:33
> -#import "WKContentView.h"
> +#import "WKContentViewInternal.h"

Nope, see above.

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 224904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224904&action=review
> 
> I'd like to discuss if there is a way to untangle stuff if you have 5 minutes.

I think we have a plan.

[Benjamin Poulain]

Something I forgot to mention, you will need to call [WKContentView willStartUserTriggeredZoom] before starting the animation. That way the future WebProcess scale update will not force the initial zoom on the page.

[Tim Horton]

Created attachment 225278 [details]
new approach

[Tim Horton]

Created attachment 225295 [details]
fix mac build

[Benjamin Poulain]

Comment on attachment 225295 [details]
fix mac build

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:325
> +- (RetainPtr<CGImageRef>)_takeViewSnapshot
> +{
> +    // FIXME: We should be able to use acquire an IOSurface directly, instead of going to CGImage here and back in ViewSnapshotStore.
> +    UIGraphicsBeginImageContextWithOptions(self.bounds.size, YES, self.window.screen.scale);
> +    [self drawViewHierarchyInRect:[self bounds] afterScreenUpdates:NO];
> +    UIImage *image = UIGraphicsGetImageFromCurrentImageContext();
> +    UIGraphicsEndImageContext();
> +    return image.CGImage;
> +}

Let's fix everything at once, otherwise it is just ugly.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:331
> +    const double maximumZoomDuration = 0.4;
> +    const double minimumZoomDuration = 0.1;
> +    const double zoomDurationFactor = 0.3;

I think Darin would prefer no const here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:341
> +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);

You could move this bellow, just before it is needed.
Shouldn't the type be CGSize or something?

I don't think the scrollview's bounds is ever correct when you want to show something. You should use the unobscuredRect.

Fun fact, the unobscuredRect can change in response to the zoom. This means the centering is not strictly correct. This is also wrong on WebKit1.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:343
> +    double currentScaleFactor = [_scrollView zoomScale];
> +    double relativeScale = scale / currentScaleFactor;

Shouldn't those be CGFloat?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:346
> +    WebCore::FloatRect targetRectInScrollView = [_scrollView convertRect:targetRect fromView:_contentView.get()];
> +    WebCore::FloatRect scaledTargetRectInScrollView = targetRectInScrollView;

CGRect?

I prefer the name to say in which coordinate system the rect is. For example, targetRectInScrollViewCoordinate.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:347
> +    scaledTargetRectInScrollView.scale(relativeScale, relativeScale);

targetRectInScrollViewAfterZoom or something like that?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:356
> +    if (scaledTargetRectInScrollView.width() > scrollViewSize.width())
> +        zoomCenter.setX(origin.x());
> +    if (scaledTargetRectInScrollView.height() > scrollViewSize.height())
> +        zoomCenter.setY(origin.y());

I think you should use the unobscured rect here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:365
> +    contentOffset = contentOffset.expandedTo(WebCore::FloatPoint());

???

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:373
> +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);
> +    WebCore::FloatSize contentSize([_scrollView contentSize]);
> +    WebCore::FloatPoint currentContentOffset([_scrollView contentOffset]);

I think you should use unobscuredRect for the scrollViewSize and currentContentOffset, and use WKContentView's size instead of [_scrollView contentSize] instead.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:376
> +    WebCore::FloatPoint originInScrollView([_scrollView convertPoint:origin fromView:_contentView.get()]);
> +    WebCore::FloatRect targetRectInScrollView([_scrollView convertRect:targetRect fromView:_contentView.get()]);

inScrollViewCoordinates.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> +    float scrollDistance = (currentContentOffset - newContentOffset).diagonalLength();

float? :)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:420
> +    CGSize scrollViewSize = [_scrollView bounds].size;

[_scrollView bounds] is probably incorrect.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:452
> +    if (!layerTreeTransaction.scaleWasSetByUIProcess() && ![_scrollView isZooming] && ![_scrollView isZoomBouncing] && ![_scrollView _isAnimatingZoom])

Sorry!

> Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:50
> +static const CGFloat smartMagnificationPanScrollThresholdZoomedOut = 60;
> +static const CGFloat smartMagnificationPanScrollThresholdIPhone = 100;
> +static const CGFloat smartMagnificationPanScrollThresholdIPad = 150;
> +static const CGFloat smartMagnificationElementPadding = 0.05;
> +
> +static const double smartMagnificationMaximumScale = 1.6;
> +static const double smartMagnificationMinimumScale = 0;

CGFloat and double?

[Tim Horton]

(In reply to comment #12)
> (From update of attachment 225295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225295&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:365
> > +    contentOffset = contentOffset.expandedTo(WebCore::FloatPoint());
> 
> ???

This is basically point clamping. We do it in a bunch of places.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:373
> > +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);
> > +    WebCore::FloatSize contentSize([_scrollView contentSize]);
> > +    WebCore::FloatPoint currentContentOffset([_scrollView contentOffset]);
> 
> I think you should use unobscuredRect for the scrollViewSize and currentContentOffset, and use WKContentView's size instead of [_scrollView contentSize] instead.

Is the offset I send to setContentOffset the same as the origin of the unobscuredRect? I think not, I think it's the actual scroll view content offset. So I'll have to move the new content offset by the difference between unobscuredRect and scrollView contentOffset?

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> > +    float scrollDistance = (currentContentOffset - newContentOffset).diagonalLength();
> 
> float? :)

Is the return type of that function :)

[Tim Horton]

Created attachment 225328 [details]
patch

[Tim Horton]

Created attachment 225329 [details]
patch

[Benjamin Poulain]

Comment on attachment 225329 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:359
> +    [_contentView willStartUserTriggeredZoom];
> +    [_scrollView _zoomToCenter:point scale:scale duration:duration];

Should this check if "scale != [_scrollview zoomScale]"? If not at runtime, an assertion would be useful. 
Otherwise, if you call [WKContentView willStartUserTriggeredZoom] and stay at the initialScale, the scale will be wrong on load or rotation.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:370
> +    CGSize targetRectSizeInScrollViewCoordinatesAfterZoom = [_scrollView convertRect:targetRect fromView:_contentView.get()].size;
> +    targetRectSizeInScrollViewCoordinatesAfterZoom.width *= relativeScale;
> +    targetRectSizeInScrollViewCoordinatesAfterZoom.height *= relativeScale;

I think this is no longer good. Now all the other computation are done in content coordinates. I think this rect is in the wrong coordinate system.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:384
> +static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOffset, WebCore::FloatSize contentSize, WebCore::FloatSize scrollViewSize)

Maybe rename scrollViewSize to unobscuredSomethingSomething?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> +    WebCore::FloatPoint originInScrollViewCoordinates([_scrollView convertPoint:origin fromView:_contentView.get()]);
> +    WebCore::FloatRect targetRectInScrollViewCoordinates([_scrollView convertRect:targetRect fromView:_contentView.get()]);

I think this may be incorrect too now. Previously you were comparing to scrollview size, which is why you needed this conversion.
Now you are comparing to unobscuredContentRect, which is content coordinate.

You can either get unobscuredRect in WKWebView coordinates and compare to that, or do the centering in content coordinates.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:442
> +    if (![_scrollView isScrollEnabled] || ![_scrollView isZoomEnabled])
> +        return false;

Not sure we need this, we don't support it anywhere else.

We can add them everywhere if that becomes needed.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:231
> +    _contentView = adoptNS([[WKContentView alloc] initWithFrame:bounds context:*toImpl(contextRef) configuration:std::move(webPageConfiguration) webView:nullptr]);

ahahahahah

> Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:79
> +    if (frameHandlesMagnificationGesture)
> +        return;

Why does the WebProcess call us back if frameHandlesMagnificationGesture is false? Shouldn't it just ignore the handleSmartMagnificationGesture()?

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:83
> +    HitTestResult hitTestResult = HitTestResult(originInContentsSpace);

Do we need the fancy iOS hit testing here? No clue what WebKit1 does.

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:87
> +    if (Node *node = hitTestResult.innerNode()) {

Node*
Maybe const too?

[Tim Horton]

Thanks, Ben! I'll fix things + do more testing.

(In reply to comment #16)
> (From update of attachment 225329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225329&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:359
> > +    [_contentView willStartUserTriggeredZoom];
> > +    [_scrollView _zoomToCenter:point scale:scale duration:duration];
> 
> Should this check if "scale != [_scrollview zoomScale]"? If not at runtime, an assertion would be useful. 
> Otherwise, if you call [WKContentView willStartUserTriggeredZoom] and stay at the initialScale, the scale will be wrong on load or rotation.

Oh! Interesting. I'll do something like that.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:442
> > +    if (![_scrollView isScrollEnabled] || ![_scrollView isZoomEnabled])
> > +        return false;
> 
> Not sure we need this, we don't support it anywhere else.
> 
> We can add them everywhere if that becomes needed.

OK, that's fine. I was just matching WK1 (ish).

> > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:231
> > +    _contentView = adoptNS([[WKContentView alloc] initWithFrame:bounds context:*toImpl(contextRef) configuration:std::move(webPageConfiguration) webView:nullptr]);
> 
> ahahahahah

Yeah :|

> > Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:79
> > +    if (frameHandlesMagnificationGesture)
> > +        return;
> 
> Why does the WebProcess call us back if frameHandlesMagnificationGesture is false? Shouldn't it just ignore the handleSmartMagnificationGesture()?

That's a pretty good idea :D

> > Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:83
> > +    HitTestResult hitTestResult = HitTestResult(originInContentsSpace);
> 
> Do we need the fancy iOS hit testing here? No clue what WebKit1 does.

I believe this is equivalent to what WebKit1 does.

[Tim Horton]

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