Bug 86881 – [WK2][WKTR] WebKitTestRunner needs eventSender.contextClick()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Caio Marcelo de Oliveira Filho]

Tests that depend on this according to grep:

editing/pasteboard/copy-standalone-image-crash.html:50:    actionitems = eventSender.contextClick();
editing/selection/5354455-1.html:23:    eventSender.contextClick();
editing/selection/5354455-2.html:33:    eventSender.contextClick();
editing/selection/5354455-2.html:40:    eventSender.contextClick();
editing/selection/button-right-click.html:22:    eventSender.contextClick();
editing/selection/context-menu-on-text.html:22:    var items = eventSender.contextClick();
editing/selection/context-menu-text-selection.html:22:    eventSender.contextClick();
editing/selection/empty-cell-right-click.html:23:    eventSender.contextClick();
editing/shadow/rightclick-on-meter-in-shadow-crash.html:15:    eventSender.contextClick();
editing/spelling/context-menu-suggestions.html:25:    var itemNamesWithoutSpellChecking = eventSender.contextClick();
editing/spelling/context-menu-suggestions.html:30:    var itemNamesWithSpellChecking = eventSender.contextClick();
editing/spelling/spellcheck-input-search-crash.html:22:    eventSender.contextClick();
fast/events/context-no-deselect.html:19:    eventSender.contextClick();
fast/events/context-onmousedown-event.html:16:     eventSender.contextClick();
fast/events/contextmenu-scrolled-page-with-frame.html:26:        eventSender.contextClick();
fast/events/right-click-focus.html:19:    eventSender.contextClick();
fast/events/selectstart-prevent-selection-on-right-click.html:27:    eventSender.contextClick();
media/context-menu-actions.html:26:                items = eventSender.contextClick();
media/controls-right-click-on-timebar.html:25:                    eventSender.contextClick();

[Jussi Kukkonen (jku)]

I could take a stab at this to get my feet wet in webkit2... Some hints would be welcome though.

It looks like it should go like this:
* add EventSendingController::contextClick() that builds a message and posts it
* EventSenderProxy::contextClick() gets called in UI process, fakes a context
  button click, and returns the menu item titles in the context menu that was 
  opened

Does it sound reasonable that the contextClick message payload (for the "IPC API") is a list of menu item title strings?

I'm currently looking at the port specific stuff for gtk (it's not entirely clear to me how I should be getting the data from a gtkmenu (in WebContextMenuProxyGtk it seems) into EventSenderProxy). I'll post a patch for comments once I have that figured out.

[Jussi Kukkonen (jku)]

(In reply to comment #0)
> Tests that depend on this according to grep:

Also, many tests in this list don't really need contextClick: they could as well use a regular button #2 click.

[Wojciech Bielawski]

Tests:
editing/pasteboard/copy-standalone-image-crash.html:50:    actionitems = eventSender.contextClick();
editing/selection/context-menu-on-text.html:22:    var items = eventSender.contextClick();
media/context-menu-actions.html:26:                items = eventSender.contextClick();

are suggesting that contextClick should return an array of menu items objects with at least:
- "title" field of string type 
- "click()" method

[Wojciech Bielawski]

Hello Jussi,
I have almost finished a partial solution (only menu item titles are returned) for this bug. Have you done something related in the code? If not I'll provide a patch in a few days.

[Jussi Kukkonen (jku)]

(In reply to comment #4)
> I have almost finished a partial solution (only menu item titles are returned) for this bug. Have you done something related in the code? If not I'll provide a patch in a few days.

Oh, sorry for not updating this: feel free, I got distracted by shinier things.

[Wojciech Bielawski]

Created attachment 165329 [details]
eventSender.contextClick() method partially implemented.

This patch provides a part of eventSender.contextClick() functionality. This method returns array of objects with property "title" where menu item title is stored. Each object should also provide "click()" method but is not implemented in this patch.

[Chris Dumez]

missing [WK2][WKTR] in bug title.

[Chris Dumez]

Comment on attachment 165329 [details]
eventSender.contextClick() method partially implemented.

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

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

WKBundlePageCopyContextMenuEntriesNames() ? Since the caller needs to free the returned array and its items.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:155
> +    WebContextMenu* cm = toImpl(pageRef)->contextMenu();

cm? please do not use abbreviations (as per coding style). "contextMenu" is probably more appropriate here.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:158
> +    for (size_t i = 0; i < items.size(); ++i) {

It is a good idea to cache items.size() before the loop.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:398
> +WK_EXPORT WKArrayRef WKBundlePageGetContextMenuEntriesNames(WKBundlePageRef pageRef);

If it is only needed by WebKitTestRunner, we can probably move it to private header?

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:120
> +    return proposedMenu;

nit: new line before return statement.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:326
> +    mouseDown(2, 0);

No mouseUp()?

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:332
> +    WKRetainPtr<WKArrayRef> entriesNames = WKBundlePageGetContextMenuEntriesNames(page);

You should adoptWK() here? Would be clearer if the function was named "Copy" instead of "Get".

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:333
> +    JSStringRef jsPropertyName = JSStringCreateWithUTF8CString("title");

Missing adopt ?

[Build Bot]

Comment on attachment 165329 [details]
eventSender.contextClick() method partially implemented.

Attachment 165329 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14002321

[Wojciech Bielawski]

Created attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

Update after  Christophe Dumez review.

[Chris Dumez]

Comment on attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

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

> Tools/ChangeLog:10
> +        context menu etry. According to some tests menu items objects shall also support 'click()' method, but it's not

"entry"

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:333
> +    WKRetainPtr<WKArrayRef> entriesNames = WKBundlePageCopyContextMenuEntriesNames(page);

Missing adopt ?
WKRetainPtr<WKArrayRef> entriesNames = adoptWK(WKBundlePageCopyContextMenuEntriesNames(page));

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:334
> +    JSStringRef jsPropertyName = JSStringCreateWithUTF8CString("title");

Missing adopt?

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:336
> +    JSValueRef jsValuesArray[entriesSize];

This won't build with MSVC. You will probably need dynamic allocation to make win-ews happy.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:341
> +        JSValueRef jsEntryName = JSValueMakeString(context, WKStringCopyJSString(wkEntryName));

missing adopt for value returned by WKStringCopyJSString()?

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:345
> +    JSStringRelease(jsPropertyName);

Would not be needed if you used JSRetainPtr<> and adopt

[Build Bot]

Comment on attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

Attachment 165364 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13992617

[Chris Dumez]

Comment on attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

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

> LayoutTests/platform/efl/editing/selection/5354455-2-expected.txt:1
> +layer at (0,0) size 800x600

This test is skipped in efl/TestExpectations, you should probably move it to efl-wk1/TestExpectations and update its expectation if it now passes for WK2 EFL.

> LayoutTests/platform/efl/fast/events/context-no-deselect-expected.txt:1
> +layer at (0,0) size 800x600

Ditto.

[Build Bot]

Comment on attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

Attachment 165364 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13996688

[Gyuyoung Kim]

Comment on attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

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

>> LayoutTests/platform/efl/editing/selection/5354455-2-expected.txt:1
>> +layer at (0,0) size 800x600
> 
> This test is skipped in efl/TestExpectations, you should probably move it to efl-wk1/TestExpectations and update its expectation if it now passes for WK2 EFL.

Why not add png file to support pixel-test as well ?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:170
> +

Unneeded a new line.

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:123
> +

ditto.

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:348
> +    return JSValueMakeUndefined(context);

Where is *context* defined ? It looks this is defined in 331 line.

[Wojciech Bielawski]

Created attachment 167064 [details]
eventSender.contextClick() method partially implemented.

[Gyuyoung Kim]

Comment on attachment 167064 [details]
eventSender.contextClick() method partially implemented.

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

Why not set r?

> LayoutTests/ChangeLog:3
> +        WebKitTestRunner needs eventSender.contextClick()

This is different from bug title.

[WK2][WKTR] WebKitTestRunner needs eventSender.contextClick()

[Wojciech Bielawski]

Created attachment 167078 [details]
 eventSender.contextClick() method partially implemented.

[Wojciech Bielawski]

Created attachment 167104 [details]
eventSender.contextClick() method partially implemented.

[Gyuyoung Kim]

Comment on attachment 167104 [details]
eventSender.contextClick() method partially implemented.

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

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:100
> +    Vector<WebContextMenuItemData> proposedMenu;

This is C-ism. Move this to 114 line.

[Wojciech Bielawski]

(In reply to comment #20)
> (From update of attachment 167104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167104&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:100
> > +    Vector<WebContextMenuItemData> proposedMenu;
> 
> This is C-ism. Move this to 114 line.

I use proposedMenu variable to return empty vector in lines 103 and 106.

[Gyuyoung Kim]

Comment on attachment 167104 [details]
eventSender.contextClick() method partially implemented.

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

BTW, it looks this patch covers other ports as well. Did you check other ports ?

>>> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:100
>>> +    Vector<WebContextMenuItemData> proposedMenu;
>> 
>> This is C-ism. Move this to 114 line.
> 
> I use proposedMenu variable to return empty vector in lines 103 and 106.

Right. I missed it.

[Michal Pakula vel Rutka]

> LayoutTests/platform/efl-wk1/TestExpectations:153
> +webkit.org/b/86091 fast/events/context-no-deselect.html [ Missing ]

You have already added test expectations for these two tests, they should be marked as failure.

> LayoutTests/platform/efl/TestExpectations:1249
> +Bug(EFL) webkit.org/b/86091 editing/selection/5354455-1.html [ Failure ]

I think 3 tests above should rather land in efl-wk2/TestExpectations, as for now we cannot check failure reasons in WebKit1 for now.

[Wojciech Bielawski]

All reported problems were fixed. Gyuyoung would you take a look on it?

[Wojciech Bielawski]

Created attachment 168909 [details]
eventSender.contextClick() method partially implemented.

Corrections after Michal's review added.

[Wojciech Bielawski]

Created attachment 168928 [details]
eventSender.contextClick() method partially implemented.

More corrections after next Michal's review.

[Wojciech Bielawski]

Created attachment 170381 [details]
eventSender.contextClick() method partially implemented.

Added gardening related with contextClick to GTK platform.

[Wojciech Bielawski]

Created attachment 170593 [details]
eventSender.contextClick() method partially implemented.

Previous path was wrongly prepared.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/efl-wk1/TestExpectations:315:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/efl-wk2/TestExpectations:274:  expecting "[", "#", or end of line instead of "editing/pasteboard/copy-standalone-image-crash.html"  [test/expectations] [5]
LayoutTests/platform/efl-wk2/TestExpectations:274:  Path does not exist.  [test/expectations] [5]
Total errors found: 3 in 15 files


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

[Wojciech Bielawski]

Created attachment 170634 [details]
eventSender.contextClick() method partially implemented.

Rebased without webkit style problems.

[Jussi Kukkonen (jku)]

Comment on attachment 170634 [details]
eventSender.contextClick() method partially implemented.

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

Sorry for not commenting earlier. Code looks ok to me, but LayoutTests/ files have some minor issues.

> LayoutTests/ChangeLog:12
> +        Added expected results for two tests for EFL platform.
> +
> +        * platform/efl/editing/selection/5354455-2-expected.txt: Added.
> +        * platform/efl/fast/events/context-no-deselect-expected.txt: Added.
> +

This is not up to date: you're changing various TestExpectations as well.

> LayoutTests/platform/efl-wk1/TestExpectations:319
> +webkit.org/b/86091 editing/selection/5354455-2.html [ Failure ]

Did you add expected results for this for a reason?

> LayoutTests/platform/efl-wk1/TestExpectations:327
> +webkit.org/b/86091 fast/events/context-no-deselect.html [ Failure ]

Ditto here.

> LayoutTests/platform/gtk-wk2/TestExpectations:585
> +# EFL's EventSender contextClick should return objects that implements click() method

Not sure if the issue is port specific, but at least it shouldn't be EFL in a GTK file.

[Jussi Kukkonen (jku)]

(In reply to comment #31)
> > LayoutTests/platform/efl-wk1/TestExpectations:319
> > +webkit.org/b/86091 editing/selection/5354455-2.html [ Failure ]
> 
> Did you add expected results for this for a reason?
> 
> > LayoutTests/platform/efl-wk1/TestExpectations:327
> > +webkit.org/b/86091 fast/events/context-no-deselect.html [ Failure ]
> 
> Ditto here.

Ah sorry, I was blind -- the results are from efl-wk2.

[Wojciech Bielawski]

Created attachment 170844 [details]
eventSender.contextClick() method partially implemented.

Corrections after Jussi Kukkonen review.

[Wojciech Bielawski]

Created attachment 171449 [details]
eventSender.contextClick() method partially implemented.

I've missed last remark from Jussi. Now all remarks are corrected.

[Wojciech Bielawski]

Created attachment 171659 [details]
eventSender.contextClick() method partially implemented.

Rebased with newest changes (contextClick for webkit1)

[Chris Dumez]

Comment on attachment 171659 [details]
eventSender.contextClick() method partially implemented.

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

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

How about WKBundlePageCopyMenuItemTitles() ?

At least mac and Qt ports seems to return "<separator>" for menu separators in DRT.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:80
> +WK_EXPORT WKArrayRef WKBundlePageCopyContextMenuEntriesNames(WKBundlePageRef pageRef);

argument name does not bring anything here.

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:104
> +    ContextMenu* menu = controller->contextMenu();

we usually have an empty line after return statement.

[Mikhail Pozdnyakov]

Comment on attachment 171659 [details]
eventSender.contextClick() method partially implemented.

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

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:98
> +Vector<WebContextMenuItemData> WebContextMenu::items() const

isn't most of this function's body code same as in WebContextMenu::show()? guess it can be shared.

[Wojciech Bielawski]

Created attachment 174396 [details]
eventSender.contextClick() method partially implemented.

Corrections after review.

[Wojciech Bielawski]

Created attachment 174413 [details]
eventSender.contextClick() method partially implemented.

Rebased with newest changes

[Kenneth Rohde Christiansen]

Comment on attachment 174413 [details]
eventSender.contextClick() method partially implemented.

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

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:109
>          proposedMenu = newMenu;
> -
> -    WebHitTestResult::Data webHitTestResultData(controller->hitTestResult());
> -
> -    // Mark the WebPage has having a shown context menu then notify the UIProcess.
> -    m_page->contextMenuShowing();
> -    m_page->send(Messages::WebPageProxy::ShowContextMenu(view->contentsToWindow(controller->hitTestResult().roundedPoint()), webHitTestResultData, proposedMenu, InjectedBundleUserMessageEncoder(userData.get())));
> +    menuItems = proposedMenu;

You are moving code to another method and this is not described in the changelog, nor why it is needed and result in the same behavior

[Mikhail Pozdnyakov]

Comment on attachment 174413 [details]
eventSender.contextClick() method partially implemented.

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:157
> +    Vector<WebContextMenuItemData> items = contextMenu->items();

nit: can be const Vector<WebContextMenuItemData>& items so that copying is obviated.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:158
> +    OwnArrayPtr<WKTypeRef> itemNames = adoptArrayPtr(new WKTypeRef[items.size()]);

arrayLength can be defined earlier and used already here

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:161
> +        RefPtr<WebString> name = WebString::create(items[i].title());

C API would look more natural here imho

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.cpp:112
> +Vector<WebContextMenuItemData> WebContextMenu::items()

const method?

[Wojciech Bielawski]

Created attachment 175428 [details]
eventSender.contextClick() method partially implemented.

Corrections after review(s).

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:163:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 14 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 175428 [details]
eventSender.contextClick() method partially implemented.

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

> Source/WebKit2/ChangeLog:11
> +        Common code in WebContextMenu.cpp moved to searate method menuItemsWithUserData().

separate*

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:163
> +    for (size_t i = 0; i < arrayLength; ++i) {
> +        itemNames[i] = WKStringCreateWithUTF8CString(items[i].title().utf8().data());
> +    }

should not use braces

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:337
> +    for (size_t i = 0; i < entriesSize; ++i) {
> +        ASSERT(WKGetTypeID(WKArrayGetItemAtIndex(entriesNames.get(), i)) == WKStringGetTypeID());

I would like a newline after the assert

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:339
> +        JSObjectRef jsItemObject = JSObjectMake(context, 0, 0);

Those 0 (nulls) mean what? we normally add a comment before like /* page */ etc

[Wojciech Bielawski]

Created attachment 175431 [details]
eventSender.contextClick() method partially implemented.

Style corrected.

[Wojciech Bielawski]

Created attachment 175435 [details]
eventSender.contextClick() method partially implemented.

Corrections after Kenneth review.

[Thiago Marcos P. Santos]

Comment on attachment 175435 [details]
eventSender.contextClick() method partially implemented.

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:164
> +    return WKArrayCreate(itemNames.get(), arrayLength);

Looks to me that this should be WKArrayCreateAdoptingValues, otherwise the strings are leaking. I don't see them being adopted.

> Source/WebKit2/WebProcess/WebPage/WebContextMenu.h:50
> +    void menuItemsWithUserData(Vector<WebContextMenuItemData> &, RefPtr<APIObject>&) const;

Indentation nit.

[Wojciech Bielawski]

Created attachment 175772 [details]
eventSender.contextClick() method partially implemented.

Corrections after Thiago review

[WebKit Review Bot]

Comment on attachment 175772 [details]
eventSender.contextClick() method partially implemented.

Rejecting attachment 175772 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
hing file Source/WebKit2/WebProcess/WebPage/WebContextMenu.h
patching file Tools/ChangeLog
patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/EventSendingController.idl
patching file Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/EventSendingController.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14950734

[Wojciech Bielawski]

Created attachment 175779 [details]
eventSender.contextClick() method partially implemented.

Rebased with latest changes

[WebKit Review Bot]

Comment on attachment 175779 [details]
eventSender.contextClick() method partially implemented.

Clearing flags on attachment: 175779

Committed r135595: <http://trac.webkit.org/changeset/135595>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.