|
Description
Caio Marcelo de Oliveira Filho
2012-05-18 12:06:30 PDT
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.
missing [WK2][WKTR] in bug title. 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 165329 [details] eventSender.contextClick() method partially implemented. Attachment 165329 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14002321 Created attachment 165364 [details]
eventSender.contextClick() method partially implemented.
Update after Christophe Dumez review.
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. Attachment 165364 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13992617 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. Attachment 165364 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13996688 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. Created attachment 167064 [details]
eventSender.contextClick() method partially implemented.
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() Created attachment 167078 [details]
eventSender.contextClick() method partially implemented.
Created attachment 167104 [details]
eventSender.contextClick() method partially implemented.
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. (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. 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. All reported problems were fixed. Gyuyoung would you take a look on it? Created attachment 168909 [details]
eventSender.contextClick() method partially implemented.
Corrections after Michal's review added.
Created attachment 168928 [details]
eventSender.contextClick() method partially implemented.
More corrections after next Michal's review.
Created attachment 170381 [details]
eventSender.contextClick() method partially implemented.
Added gardening related with contextClick to GTK platform.
Created attachment 170593 [details]
eventSender.contextClick() method partially implemented.
Previous path was wrongly prepared.
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.
Created attachment 170634 [details]
eventSender.contextClick() method partially implemented.
Rebased without webkit style problems.
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 170844 [details]
eventSender.contextClick() method partially implemented.
Corrections after Jussi Kukkonen review.
Created attachment 171449 [details]
eventSender.contextClick() method partially implemented.
I've missed last remark from Jussi. Now all remarks are corrected.
Created attachment 171659 [details]
eventSender.contextClick() method partially implemented.
Rebased with newest changes (contextClick for webkit1)
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. Created attachment 174396 [details]
eventSender.contextClick() method partially implemented.
Corrections after review.
Created attachment 174413 [details]
eventSender.contextClick() method partially implemented.
Rebased with newest changes
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? Created attachment 175428 [details]
eventSender.contextClick() method partially implemented.
Corrections after review(s).
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 Created attachment 175431 [details]
eventSender.contextClick() method partially implemented.
Style corrected.
Created attachment 175435 [details]
eventSender.contextClick() method partially implemented.
Corrections after Kenneth review.
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. Created attachment 175772 [details]
eventSender.contextClick() method partially implemented.
Corrections after Thiago review
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 Created attachment 175779 [details]
eventSender.contextClick() method partially implemented.
Rebased with latest changes
Comment on attachment 175779 [details] eventSender.contextClick() method partially implemented. Clearing flags on attachment: 175779 Committed r135595: <http://trac.webkit.org/changeset/135595> All reviewed patches have been landed. Closing bug. |