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
Created attachment 183944 [details] click() method for objects returned by eventSender.clontextClick()
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.
Otherwise, the change looks sane to me.
Created attachment 184219 [details] click() method for objects returned by eventSender.clontextClick()
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.
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?
Created attachment 188874 [details] click() method for objects returned by eventSender.clontextClick()
(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
(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?
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?
(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
Created attachment 195328 [details] click() method for objects returned by eventSender.clontextClick()
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 > +2013-03-27 Seokju Kwon <seokju.kwon@gmail.com> > + > + [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.
Created attachment 202271 [details] click() method for objects returned by eventSender.clontextClick()
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?
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
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
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
Created attachment 203687 [details] click() method for objects returned by eventSender.clontextClick()
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
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
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
Created attachment 203689 [details] click() method for objects returned by eventSender.clontextClick()
Created attachment 203846 [details] click() method for objects returned by eventSender.clontextClick() Rebased to satisfy dependencies
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.
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.
Created attachment 209087 [details] click() method for objects returned by eventSender.contextClick() Added casting from object returned by click() method to jsString.
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.
Created attachment 209330 [details] click() method for objects returned by eventSender.contextClick()
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
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
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>
All reviewed patches have been landed. Closing bug.
*** Bug 99071 has been marked as a duplicate of this bug. ***