[webkit-reviews] review granted: [Bug 54898] InjectedBundleNodeHandle dies too early in WKBundleHitTestResultGetNodeHandle : [Attachment 83402] API test

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Alice Liu
<alice.liu at apple.com>'s request for review:
Bug 54898: InjectedBundleNodeHandle dies too early in
WKBundleHitTestResultGetNodeHandle
https://bugs.webkit.org/show_bug.cgi?id=54898

Attachment 83402: API test
https://bugs.webkit.org/attachment.cgi?id=83402&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83402&action=review

Awesome that you got a test working!

> Tools/TestWebKitAPI/PlatformWebView.h:65
> +    void simulateRightClick(CGPoint pt);

No need for "pt" here. (If you did need an argument name, "point" would be
better.) Maybe it should be a const CGPoint&?

It's slightly unfortunate to use CG types in cross-platform code, as this will
make it harder for other ports to build TestWebKitAPI. Maybe two unsigneds
would be better.

> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:70
> +    WKContextInjectedBundleClient injectedBundleClient;
> +    memset(&injectedBundleClient, 0, sizeof(injectedBundleClient));
> +    injectedBundleClient.version = 0;
> +    injectedBundleClient.clientInfo = 0;
> +    injectedBundleClient.didReceiveMessageFromInjectedBundle =
didReceiveMessageFromInjectedBundle;
> +    WKContextSetInjectedBundleClient(context.get(), &injectedBundleClient);

A separate setInjectedBundleClient function would make the main test logic
easier to follow.

> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:82
> +    TEST_ASSERT(done);

No need for this assertion, as it will never be reached if done is still false.


> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle_Bundle.cpp:49
> +	   WKRetainPtr<WKStringRef> doneMessageName(AdoptWK,
WKStringCreateWithUTF8CString("HitTestResultNodeHandleTestDoneMessageName"));
> +	   WKRetainPtr<WKStringRef> doneMessageBody(AdoptWK,
WKStringCreateWithUTF8CString("HitTestResultNodeHandleTestDoneMessageBody"));

You can use the Util::toWK function here instead.

> Tools/TestWebKitAPI/win/PlatformWebViewWin.cpp:109
> +    ::SendMessage(window, WM_RBUTTONDOWN, 0, MAKELPARAM(pt.x, pt.y));
> +    ::SendMessage(window, WM_RBUTTONUP, 0, MAKELPARAM(pt.x, pt.y));

Please use ::SendMessageW.