Bug 134279 – [iOS]: WK2 Inspector Node Search

• Component: WebKit2
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Web Inspector's "Inspect" button for node search doesn't work in WK2.

[Joseph Pecoraro]

<rdar://problem/16691017>

[Joseph Pecoraro]

Created attachment 233759 [details]
[PATCH] Proposed Fix

Patch probably won't apply because it is on top of other patches.

[Joseph Pecoraro]

I have an even better patch in progress that will allow users to scrub their finger across the page, updating the highlight, and finally inspecting the node when they release their finger. I'll clean that up and test edge cases.

[Joseph Pecoraro]

Created attachment 233771 [details]
[PATCH] Proposed Fix (With Gesture)

[Benjamin Poulain]

Comment on attachment 233771 [details]
[PATCH] Proposed Fix (With Gesture)

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.

[Joseph Pecoraro]

Created attachment 233829 [details]
[PATCH] Addressed Review Comments

[Joseph Pecoraro]

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:584
> > +    WKBeginObservingContentChanges(true);
> 
> I am pretty sure you don't want that.

Oops I missed this comment. Let me look into that.

[Joseph Pecoraro]

Created attachment 233837 [details]
[PATCH] Addressed Review Comments

[WebKit Commit Bot]

Comment on attachment 233837 [details]
[PATCH] Addressed Review Comments

Clearing flags on attachment: 233837

Committed r170445: <http://trac.webkit.org/changeset/170445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.