[webkit-reviews] review denied: [Bug 98410] [WK2][WKTR] WebKitTestRunner's eventSender.contextClick() returns objects without implemented click() method : [Attachment 184219] click() method for objects returned by eventSender.clontextClick()

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Wojciech Bielawski
<w.bielawski at samsung.com>'s request for review:
Bug 98410: [WK2][WKTR] WebKitTestRunner's eventSender.contextClick() returns
objects without implemented click() method
https://bugs.webkit.org/show_bug.cgi?id=98410

Attachment 184219: click() method for objects returned by
eventSender.clontextClick()
https://bugs.webkit.org/attachment.cgi?id=184219&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184219&action=review


Thanks for looking at the tests!
I need to have better look at the tests before I can make a proper review. Some
comments bellow:

Any reason why testing is done that way instead of through the UIProcess?

> LayoutTests/ChangeLog:11
> +	   * platform/efl-wk2/TestExpectations:
> +	   * platform/gtk-wk2/TestExpectations:

Those two tests are skipped on all wk2 platforms. The patch does not have
anything platform specific.

Why are you only updating 2 TestExpectations?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:171
> +WKArrayRef WKBundlePageGetContextMenuItems(WKBundlePageRef pageRef)

Create and Copy return a reference to a owned object.

Here, the name is a Get, but you are returning a new object. That is an open
invitation for a leak :(

> Tools/ChangeLog:12
> +	   (WTR):

You can remove that line.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:398
> +    WKRetainPtr<WKArrayRef> menuEntries =
adoptWK(WKBundlePageGetContextMenuItems(page));

This is the problem, you adopt a "Get" method.