[webkit-reviews] review granted: [Bug 129146] WebKit2 View Gestures (Smart Magnification): Support for iOS : [Attachment 225329] patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 129146: WebKit2 View Gestures (Smart Magnification): Support for iOS
https://bugs.webkit.org/show_bug.cgi?id=129146

Attachment 225329: patch
https://bugs.webkit.org/attachment.cgi?id=225329&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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?