Bug 98410 – [WK2][WKTR] WebKitTestRunner's eventSender.contextClick() returns objects without implemented click() method

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Wojciech Bielawski]

Patch for bug https://bugs.webkit.org/show_bug.cgi?id=86881 provides only partial implementation of eventSender.contextClick(). This method returns array of objects. Each returned object is menu entry representative and it should support click() method.

Below layout tests requires such method:
editing/pasteboard/copy-standalone-image-crash.html
media/context-menu-actions.html

[Wojciech Bielawski]

Created attachment 183944 [details]
click() method for objects returned by eventSender.clontextClick()

[Chris Dumez]

Comment on attachment 183944 [details]
click() method for objects returned by eventSender.clontextClick()

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

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:62
> +    return JSValueMakeString(context, jsTitleCopy.get());

You could use toJS() from StringFunctions.h to reduce the code a bit.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:409
> +        JSObjectRef jsItemObject = JSObjectMake(context, getMenuItemClass(), privateData);

Useless temporary object (jsItemObject). You could assign to jsValuesArray[i] directly.

[Chris Dumez]

Otherwise, the change looks sane to me.

[Wojciech Bielawski]

Created attachment 184219 [details]
click() method for objects returned by eventSender.clontextClick()

[Benjamin Poulain]

Comment on attachment 184219 [details]
click() method for objects returned by eventSender.clontextClick()

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.

[Wojciech Bielawski]

Comment on attachment 184219 [details]
click() method for objects returned by eventSender.clontextClick()

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

>> LayoutTests/ChangeLog:11
>> +        * 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?

I'm not able to test it on other platforms. Is this change is sufficient?

[Wojciech Bielawski]

Created attachment 188874 [details]
click() method for objects returned by eventSender.clontextClick()

[Gyuyoung Kim]

(In reply to comment #6)
> (From update of attachment 184219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184219&action=review
> 
> >> LayoutTests/ChangeLog:11
> >> +        * 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?
> 
> I'm not able to test it on other platforms. Is this change is sufficient?

When I did grep this test case in TestExpectations files, almost ports skip this test. I think it would be good if you unskip this case for other ports. 

efl-wk2/TestExpectations:webkit.org/b/98410 editing/pasteboard/copy-standalone-image-crash.html [ Failure ]
gtk-wk2/TestExpectations:webkit.org/b/98410 editing/pasteboard/copy-standalone-image-crash.html [ Failure ]
mac-wk2/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
qt-5.0-wk2/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
win/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
wincairo/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
win-wk2/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html

[Wojciech Bielawski]

(In reply to comment #8)
> When I did grep this test case in TestExpectations files, almost ports skip this test. I think it would be good if you unskip this case for other ports. 
> 
> efl-wk2/TestExpectations:webkit.org/b/98410 editing/pasteboard/copy-standalone-image-crash.html [ Failure ]
> gtk-wk2/TestExpectations:webkit.org/b/98410 editing/pasteboard/copy-standalone-image-crash.html [ Failure ]
> mac-wk2/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
> qt-5.0-wk2/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
> win/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
> wincairo/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html
> win-wk2/TestExpectations:editing/pasteboard/copy-standalone-image-crash.html

TestExpectations for efl-wk2 and gtk-wk2 are already corrected in patch.
It seems that mac-wk2, win-wk2 and qt-5.0-wk2 ports don't support eventSender (https://bugs.webkit.org/show_bug.cgi?id=42194), so I'll leave it skipped.
win/TestExpectations concerns wk1, so I'll not modify it also.

Do you know does wincario platform support wk2?

[Mikhail Pozdnyakov]

Comment on attachment 188874 [details]
click() method for objects returned by eventSender.clontextClick()

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:151
> +WKStringRef WKBundlePageCopyContextMenuItemTitle(WKContextMenuItemRef item)

why this is part of page API and not WKContextMenuItem API?

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:46
> +    WKRetainPtr<WKBundlePageRef> page;

so MenuItemPrivateData retains page ownership?

[Mikhail Pozdnyakov]

(In reply to comment #10)
> (From update of attachment 188874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188874&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:151
> > +WKStringRef WKBundlePageCopyContextMenuItemTitle(WKContextMenuItemRef item)
> 
> why this is part of page API and not WKContextMenuItem API?

btw, there is already WK_EXPORT WKStringRef WKContextMenuItemCopyTitle(WKContextMenuItemRef);

at Source/WebKit2/Shared/API/c/WKContextMenuItem.h

[Wojciech Bielawski]

Created attachment 195328 [details]
click() method for objects returned by eventSender.clontextClick()

[Benjamin Poulain]

Comment on attachment 195328 [details]
click() method for objects returned by eventSender.clontextClick()

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

Why doesn't that unskip tests on the Mac port. This code is common code.

> LayoutTests/ChangeLog:21
> [phone]  Seokju Kwon  <[e-mail]>
> +
> +        [EFL] Remove a duplicated test from TestExpectations
> +        https://bugs.webkit.org/show_bug.cgi?id=113385
> +
> +        Unreviewed, EFL gardening.
> +
> +        * platform/efl/TestExpectations:
> +

Trouble with your changelog.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:45
> +typedef struct MenuItemPrivateData {

Why do you use typedef here?

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:46
> +    // This object has to own menu item because it is not possible to inform JavaScript when context menu is closing

Missing period.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:49
> +    MenuItemPrivateData(WKBundlePageRef _page, WKContextMenuItemRef _item) : page(_page), item(_item) { }

Incorrect arguments names.

[Wojciech Bielawski]

Created attachment 202271 [details]
click() method for objects returned by eventSender.clontextClick()

[Wojciech Bielawski]

I'm not able to test my changes with MAC port. Could some help me and test the patch on MAC with unskipped media/context-menu-actions.html in platform/mac/TestExpectations?

[Early Warning System Bot]

Comment on attachment 202271 [details]
click() method for objects returned by eventSender.clontextClick()

Attachment 202271 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/521019

[Build Bot]

Comment on attachment 202271 [details]
click() method for objects returned by eventSender.clontextClick()

Attachment 202271 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/405238

[Build Bot]

Comment on attachment 202271 [details]
click() method for objects returned by eventSender.clontextClick()

Attachment 202271 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/438384

[Wojciech Bielawski]

Created attachment 203687 [details]
click() method for objects returned by eventSender.clontextClick()

[Early Warning System Bot]

Comment on attachment 203687 [details]
click() method for objects returned by eventSender.clontextClick()

Attachment 203687 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/673480

[Build Bot]

Comment on attachment 203687 [details]
click() method for objects returned by eventSender.clontextClick()

Attachment 203687 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/717809

[Build Bot]

Comment on attachment 203687 [details]
click() method for objects returned by eventSender.clontextClick()

Attachment 203687 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/673485

[Wojciech Bielawski]

Created attachment 203689 [details]
click() method for objects returned by eventSender.clontextClick()

[Wojciech Bielawski]

Created attachment 203846 [details]
click() method for objects returned by eventSender.clontextClick()

Rebased to satisfy dependencies

[Benjamin Poulain]

Comment on attachment 203846 [details]
click() method for objects returned by eventSender.clontextClick()

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

Looks good. Please enable on OS X and check the comments bellow.

Interestingly, this API does not exist on Mac or Qt.
Given that DRT returns a list of string on OS X and Qt, I think your JSObject should have toString() that returns the item's title.

> LayoutTests/ChangeLog:13
> +        * platform/efl-wk2/TestExpectations: Corrected bug number for tests:
> +         editing/pasteboard/can-read-in-copy-and-cut-events.html
> +         editing/pasteboard/can-read-in-dragstart-event.html
> +        * platform/gtk-wk2/TestExpectations:

Please at least enable Mac. The EWS bots will help you find what test changes as a result.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:46
> +    // This object has to own menu item because it is not possible to inform JavaScript when context menu is closing.

This comment is misleading. Please remove it.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:49
> +    MenuItemPrivateData(WKBundlePageRef page, WKContextMenuItemRef item) : m_page(page), m_item(item) { }

Style:
-Make this the first member of the definition.
-Split the initialization over multiple lines.

[Wojciech Bielawski]

Created attachment 207309 [details]
click() method for objects returned by eventSender.clontextClick()

This patch covers incjected bundle only for WTR. For DRT there exist separate implementation.

[Wojciech Bielawski]

Created attachment 209087 [details]
click() method for objects returned by eventSender.contextClick()

Added casting from object returned by click() method to jsString.

[WebKit Commit Bot]

Attachment 209087 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/efl-wk2/TestExpectations', u'LayoutTests/platform/gtk-wk2/TestExpectations', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h', u'Tools/ChangeLog', u'Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp']" exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:87:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wojciech Bielawski]

Created attachment 209330 [details]
click() method for objects returned by eventSender.contextClick()

[Build Bot]

Comment on attachment 209330 [details]
click() method for objects returned by eventSender.contextClick()

Attachment 209330 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1426039

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209338 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[WebKit Commit Bot]

Comment on attachment 209330 [details]
click() method for objects returned by eventSender.contextClick()

Clearing flags on attachment: 209330

Committed r155847: <http://trac.webkit.org/changeset/155847>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alberto Garcia]

*** Bug 99071 has been marked as a duplicate of this bug. ***