Bug 105106 – Add SPI to WebKit1 WebFrame for hit testing

• Status: NEW
• Component: WebKit API

[Alice Liu]

WebFrame doesn't have any hit testing-related API.  And it doesn't have way to convert that DOMNode to a JSValueRef.   This patch adds that functionality to WebFrame, as SPI in WebFramePrivate.  I'm not sure if it should be SPI or API.  i could use some feedback on that, especially. 

<rdar://problem/12880594>

[Alice Liu]

Created attachment 179613 [details]
patch

[Alice Liu]

Created attachment 179616 [details]
patch created from correct directory level

Comment on attachment 179616 [details]
patch created from correct directory level

View in context: https://bugs.webkit.org/attachment.cgi?id=179616&action=review

> Source/WebKit/mac/ChangeLog:3
> +        Add API or SPI to WebFrame for hit testing and node conversion to JSValueRef

It’s odd to have these two APIs lumped together into one bug and one patch, even if your intentions are to use them together. Please file a separate bug for one of these.

> Source/WebKit/mac/WebView/WebFrame.mm:1505
> +    Frame* coreFrame = _private->coreFrame;
> +    if (!coreFrame)
> +        return nil;
> +    return [[[WebElementDictionary alloc] initWithHitTestResult:coreFrame->eventHandler()->hitTestResultAtPoint(IntPoint(point), false, true)] autorelease];

This does a poor job for non-WebHTMLView frames (the only built-in example of which that is left is WebPDFView). You can see how -[WebView _elementAtWindowPoint:] handles this more gracefully and generally. I’d say it’s OK to go with this simplified version, but a generic version would be fine, too.

> Source/WebKit/mac/WebView/WebFrame.mm:1508
> +- (JSValueRef)jsWrapper:(DOMNode *)node forWorld:(WebScriptWorld *)world

I think a name like
    -jsWrapperForNode:inWorld:
would be more Cocoa-y and accurate.

[Build Bot]

Comment on attachment 179616 [details]
patch created from correct directory level

Attachment 179616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15366188

[Sam Weinig]

Comment on attachment 179616 [details]
patch created from correct directory level

I agree with mitz.

[Alice Liu]

Created attachment 179641 [details]
patch fixed name

[Alice Liu]

(In reply to comment #3)
> (From update of attachment 179616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179616&action=review
> 
> > Source/WebKit/mac/ChangeLog:3
> > +        Add API or SPI to WebFrame for hit testing and node conversion to JSValueRef
> 
> It’s odd to have these two APIs lumped together into one bug and one patch, even if your intentions are to use them together. Please file a separate bug for one of these.
> 
filed bug 105262 for node conversion.  re-designating this bug as hit-testing-related only. 

> > Source/WebKit/mac/WebView/WebFrame.mm:1505
> > +    Frame* coreFrame = _private->coreFrame;
> > +    if (!coreFrame)
> > +        return nil;
> > +    return [[[WebElementDictionary alloc] initWithHitTestResult:coreFrame->eventHandler()->hitTestResultAtPoint(IntPoint(point), false, true)] autorelease];
> 
> This does a poor job for non-WebHTMLView frames (the only built-in example of which that is left is WebPDFView). You can see how -[WebView _elementAtWindowPoint:] handles this more gracefully and generally. I’d say it’s OK to go with this simplified version, but a generic version would be fine, too.
> 
Thanks for pointing that out.  The key thing i heard there was that you are OK with this simplified version. 

> > Source/WebKit/mac/WebView/WebFrame.mm:1508
> > +- (JSValueRef)jsWrapper:(DOMNode *)node forWorld:(WebScriptWorld *)world
> 
> I think a name like
>     -jsWrapperForNode:inWorld:
> would be more Cocoa-y and accurate.

addressed in patch for bug 105262. 

thank you. -alice

[Alice Liu]

Created attachment 179897 [details]
patch, just elementAtPoint

[Alice Liu]

Created attachment 179900 [details]
patch

[Alice Liu]

Are the failures on the chromium-ews and mac-ews bots expected?  they have nothing to do with applied patch.

Comment on attachment 179900 [details]
patch

Thanks for splitting this up and adding an API test! A few suggestions below.

(In reply to comment #11)
> (From update of attachment 179900 [details])
> Thanks for splitting this up and adding an API test! A few suggestions below.

The review form ate my suggestions. I’m going to try again.

Comment on attachment 179900 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179900&action=review

> Source/WebKit/mac/WebView/WebFramePrivate.h:141
> +- (NSDictionary *)elementAtPoint:(NSPoint)point;

Though we’re not terribly consistent about this, many of our SPI methods are prefixed with an underscore. I would prefer that.

Comment on attachment 179900 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179900&action=review

> Tools/TestWebKitAPI/Tests/mac/ElementAtPointInWebFrame.mm:32
> +@interface ElementAtPointFrameLoadDelegate : NSObject {
> +}

I think you can leave off the braces when there are no ivars.

> Tools/TestWebKitAPI/Tests/mac/ElementAtPointInWebFrame.mm:50
> +    RetainPtr<WebView> webView(AdoptNS, [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 120, 200) frameName:nil groupName:nil]);

One of the features of this SPI (which separates it from the elementFromPoint DOM API) is that it can hit test outside the frame bounds. It would be neat if you e.g. made the frame narrower and then hit tested a point whose x coordinate was beyond the frame’s width.

> Tools/TestWebKitAPI/Tests/mac/ElementAtPointInWebFrame.mm:63
> +    EXPECT_TRUE([[domElement getAttribute:@"name"] isEqualToString:@"first"]);

Rather than EXPECT_TRUE, you should use the macro that compares NSStrings, EXPECT_WK_STREQ.

> Tools/TestWebKitAPI/Tests/mac/ElementAtPointInWebFrame.mm:67
> +    EXPECT_TRUE([[domElement getAttribute:@"name"] isEqualToString:@"second"]);

Ditto.

> Tools/TestWebKitAPI/Tests/mac/ElementAtPointInWebFrame.mm:71
> +    EXPECT_TRUE([[domElement tagName] isEqualToString:@"BODY"]);

Ditto.