[webkit-reviews] review requested: [Bug 129146] WebKit2 View Gestures (Smart Magnification): Support for iOS : [Attachment 224904] zomgrebase

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has asked  for review:
Bug 129146: WebKit2 View Gestures (Smart Magnification): Support for iOS
https://bugs.webkit.org/show_bug.cgi?id=129146

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

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