|
Description
Wojciech Bielawski
2012-10-04 07:51:25 PDT
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. *** |