Bug 134279 - [iOS]: WK2 Inspector Node Search
Summary: [iOS]: WK2 Inspector Node Search
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-24 16:07 PDT by Joseph Pecoraro
Modified: 2014-06-25 16:13 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (17.58 KB, patch)
2014-06-24 16:17 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (With Gesture) (30.89 KB, patch)
2014-06-24 18:01 PDT, Joseph Pecoraro
benjamin: review-
Details | Formatted Diff | Diff
[PATCH] Addressed Review Comments (33.23 KB, patch)
2014-06-25 12:29 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Addressed Review Comments (33.17 KB, patch)
2014-06-25 14:32 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-06-24 16:07:45 PDT
Web Inspector's "Inspect" button for node search doesn't work in WK2.
Comment 1 Joseph Pecoraro 2014-06-24 16:08:34 PDT
<rdar://problem/16691017>
Comment 2 Joseph Pecoraro 2014-06-24 16:17:35 PDT
Created attachment 233759 [details]
[PATCH] Proposed Fix

Patch probably won't apply because it is on top of other patches.
Comment 3 Joseph Pecoraro 2014-06-24 17:12:36 PDT
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.
Comment 4 Joseph Pecoraro 2014-06-24 18:01:18 PDT
Created attachment 233771 [details]
[PATCH] Proposed Fix (With Gesture)
Comment 5 Benjamin Poulain 2014-06-24 18:30:19 PDT
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.
Comment 6 Joseph Pecoraro 2014-06-25 12:29:46 PDT
Created attachment 233829 [details]
[PATCH] Addressed Review Comments
Comment 7 Joseph Pecoraro 2014-06-25 12:30:42 PDT
> > 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.
Comment 8 Joseph Pecoraro 2014-06-25 14:32:04 PDT
Created attachment 233837 [details]
[PATCH] Addressed Review Comments
Comment 9 WebKit Commit Bot 2014-06-25 16:13:49 PDT
Comment on attachment 233837 [details]
[PATCH] Addressed Review Comments

Clearing flags on attachment: 233837

Committed r170445: <http://trac.webkit.org/changeset/170445>
Comment 10 WebKit Commit Bot 2014-06-25 16:13:54 PDT
All reviewed patches have been landed.  Closing bug.