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.
(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.
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
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.
(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.
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.
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 ?
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
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.
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.
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()
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.
> 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.
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.
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.
(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.
Created attachment 171449[details]
eventSender.contextClick() method partially implemented.
I've missed last remark from Jussi. Now all remarks are corrected.
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.
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.
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
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?
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.
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
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.
2012-09-24 01:31 PDT, Wojciech Bielawski
2012-09-24 06:07 PDT, Wojciech Bielawski
2012-10-04 03:21 PDT, Wojciech Bielawski
2012-10-04 05:14 PDT, Wojciech Bielawski
2012-10-04 07:58 PDT, Wojciech Bielawski
2012-10-16 03:31 PDT, Wojciech Bielawski
2012-10-16 05:17 PDT, Wojciech Bielawski
2012-10-24 06:32 PDT, Wojciech Bielawski
2012-10-25 02:18 PDT, Wojciech Bielawski
2012-10-25 06:53 PDT, Wojciech Bielawski
2012-10-26 01:14 PDT, Wojciech Bielawski
2012-10-30 07:31 PDT, Wojciech Bielawski
2012-10-31 07:44 PDT, Wojciech Bielawski
2012-11-15 03:45 PST, Wojciech Bielawski
2012-11-15 05:28 PST, Wojciech Bielawski
2012-11-21 06:34 PST, Wojciech Bielawski
2012-11-21 06:45 PST, Wojciech Bielawski
2012-11-21 07:04 PST, Wojciech Bielawski
2012-11-23 03:45 PST, Wojciech Bielawski
webkit.review.bot: commit-queue-
2012-11-23 04:38 PST, Wojciech Bielawski