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
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.
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?
(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
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.
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 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 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
2013-01-22 02:40 PST, Wojciech Bielawski
2013-01-23 06:33 PST, Wojciech Bielawski
2013-02-18 06:32 PST, Wojciech Bielawski
2013-03-27 08:14 PDT, Wojciech Bielawski
2013-05-20 05:19 PDT, Wojciech Bielawski
2013-06-04 06:02 PDT, Wojciech Bielawski
2013-06-04 06:44 PDT, Wojciech Bielawski
2013-06-05 06:46 PDT, Wojciech Bielawski
2013-07-23 01:20 PDT, Wojciech Bielawski
2013-08-19 07:48 PDT, Wojciech Bielawski
2013-08-22 00:44 PDT, Wojciech Bielawski
2013-08-22 03:34 PDT, Build Bot