[webkit-reviews] review denied: [Bug 134279] [iOS]: WK2 Inspector Node Search : [Attachment 233771] [PATCH] Proposed Fix (With Gesture)

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 134279: [iOS]: WK2 Inspector Node Search
https://bugs.webkit.org/show_bug.cgi?id=134279

Attachment 233771: [PATCH] Proposed Fix (With Gesture)
https://bugs.webkit.org/attachment.cgi?id=233771&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233771&action=review


Some small comments below, I would like to see the V2.

There is one more thing you need to do: handle crash correctly. You need to
reset into the baseline state when that happen.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:661
> +- (void)_enableInspectorNodeSearch
> +{
> +    [_contentView _enableInspectorNodeSearch];
> +}
> +
> +- (void)_disableInspectorNodeSearch
> +{
> +    [_contentView _disableInspectorNodeSearch];
> +}

You should not do this here. The page client can talk to either WKWebView or
WKContentView.
All the "document related" things go to WKContentView, all the "scrollview
related" things go to WKWebView.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:100
> +- (void)_enableInspectorNodeSearch;
> +- (void)_disableInspectorNodeSearch;

This should be directly on the WKContentViewInteraction's interface.

> Source/WebKit2/UIProcess/WebPageProxy.h:848
> +    void inspectorNodeSearchMoveToPosition(const WebCore::FloatPoint&);

Move->Moved?

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:351
> +    EnableInspectorNodeSearch()
> +    DisableInspectorNodeSearch()

It is rather funny we are being told that from the WebProcess. Nothing wrong
with that though.

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:515
> +{
> +    [m_webView _enableInspectorNodeSearch];
> +}
> +
> +void PageClientImpl::disableInspectorNodeSearch()
> +{
> +    [m_webView _disableInspectorNodeSearch];

Here you can access m_contentView.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:333
> +    _inspectorNodeSearchGestureRecognizer =
[[WKInspectorNodeSearchGestureRecognizer alloc] initWithTarget:self
action:@selector(_inspectorNodeSearchRecognized:)];

You forgot the adoptNS() here.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:432
> +    case UIGestureRecognizerStateEnded:
> +    default: // To ensure we turn off node search.
> +	   _page->inspectorNodeSearchEndedAtPosition(point);
> +	   break;

You should also handle cancel correctly here. Send a message clearing the
NodeSearch in the WebProcess.

> Source/WebKit2/UIProcess/ios/WKInspectorNodeSearchGestureRecognizer.mm:51
> +    for (UITouch *touch in touches) {
> +	   if (touch == _touch.get()) {
> +	       self.state = newState;
> +	       break;
> +	   }
> +    }

You do not need to iterate over all the object to get the target touch, UIKit
gives us a NSSet :)

> Source/WebKit2/UIProcess/ios/WKInspectorNodeSearchGestureRecognizer.mm:77
> +    [self _processTouches:touches state:UIGestureRecognizerStateEnded];

I think you should cancel the gesture in that case.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:584
> +    WKBeginObservingContentChanges(true);

I am pretty sure you don't want that.