[Webkit-unassigned] [Bug 134279] [iOS]: WK2 Inspector Node Search

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=134279


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #233771|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #5 from Benjamin Poulain <benjamin at webkit.org>  2014-06-24 18:30:19 PST ---
(From update of attachment 233771)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.