Bug 98410 (contextClick().click) - [WK2][WKTR] WebKitTestRunner's eventSender.contextClick() returns objects without implemented click() method
Summary: [WK2][WKTR] WebKitTestRunner's eventSender.contextClick() returns objects wit...
Status: RESOLVED FIXED
Alias: contextClick().click
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wojciech Bielawski
URL:
Keywords:
: 99071 (view as bug list)
Depends on: 117198
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-04 07:51 PDT by Wojciech Bielawski
Modified: 2013-09-26 09:15 PDT (History)
19 users (show)

See Also:


Attachments
click() method for objects returned by eventSender.clontextClick() (11.21 KB, patch)
2013-01-22 02:40 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.20 KB, patch)
2013-01-23 06:33 PST, Wojciech Bielawski
benjamin: review-
Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.11 KB, patch)
2013-02-18 06:32 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (12.39 KB, patch)
2013-03-27 08:14 PDT, Wojciech Bielawski
benjamin: review-
Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.75 KB, patch)
2013-05-20 05:19 PDT, Wojciech Bielawski
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.78 KB, patch)
2013-06-04 06:02 PDT, Wojciech Bielawski
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.78 KB, patch)
2013-06-04 06:44 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.80 KB, patch)
2013-06-05 06:46 PDT, Wojciech Bielawski
benjamin: review-
Details | Formatted Diff | Diff
click() method for objects returned by eventSender.clontextClick() (11.66 KB, patch)
2013-07-23 01:20 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
click() method for objects returned by eventSender.contextClick() (12.01 KB, patch)
2013-08-19 07:48 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
click() method for objects returned by eventSender.contextClick() (11.92 KB, patch)
2013-08-22 00:44 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (525.96 KB, application/zip)
2013-08-22 03:34 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wojciech Bielawski 2012-10-04 07:51:25 PDT
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 1 Wojciech Bielawski 2013-01-22 02:40:45 PST
Created attachment 183944 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 2 Chris Dumez 2013-01-22 03:25:46 PST
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 3 Chris Dumez 2013-01-22 03:26:16 PST
Otherwise, the change looks sane to me.
Comment 4 Wojciech Bielawski 2013-01-23 06:33:26 PST
Created attachment 184219 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 5 Benjamin Poulain 2013-01-23 23:59:09 PST
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 6 Wojciech Bielawski 2013-02-18 06:31:03 PST
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?
Comment 7 Wojciech Bielawski 2013-02-18 06:32:04 PST
Created attachment 188874 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 8 Gyuyoung Kim 2013-03-13 03:31:14 PDT
(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
Comment 9 Wojciech Bielawski 2013-03-13 04:41:45 PDT
(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 10 Mikhail Pozdnyakov 2013-03-26 04:30:44 PDT
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?
Comment 11 Mikhail Pozdnyakov 2013-03-26 04:59:16 PDT
(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 12 Wojciech Bielawski 2013-03-27 08:14:05 PDT
Created attachment 195328 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 13 Benjamin Poulain 2013-05-16 11:54:32 PDT
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.
Comment 14 Wojciech Bielawski 2013-05-20 05:19:30 PDT
Created attachment 202271 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 15 Wojciech Bielawski 2013-05-20 05:21:57 PDT
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 16 Early Warning System Bot 2013-05-20 05:25:59 PDT
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 17 Build Bot 2013-05-20 05:45:02 PDT
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 18 Build Bot 2013-05-20 06:11:40 PDT
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
Comment 19 Wojciech Bielawski 2013-06-04 06:02:35 PDT
Created attachment 203687 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 20 Early Warning System Bot 2013-06-04 06:09:48 PDT
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 21 Build Bot 2013-06-04 06:27:07 PDT
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 22 Build Bot 2013-06-04 06:33:08 PDT
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
Comment 23 Wojciech Bielawski 2013-06-04 06:44:40 PDT
Created attachment 203689 [details]
click() method for objects returned by eventSender.clontextClick()
Comment 24 Wojciech Bielawski 2013-06-05 06:46:41 PDT
Created attachment 203846 [details]
click() method for objects returned by eventSender.clontextClick()

Rebased to satisfy dependencies
Comment 25 Benjamin Poulain 2013-06-17 14:31:19 PDT
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.
Comment 26 Wojciech Bielawski 2013-07-23 01:20:51 PDT
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.
Comment 27 Wojciech Bielawski 2013-08-19 07:48:14 PDT
Created attachment 209087 [details]
click() method for objects returned by eventSender.contextClick()

Added casting from object returned by click() method to jsString.
Comment 28 WebKit Commit Bot 2013-08-19 07:50:18 PDT
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.
Comment 29 Wojciech Bielawski 2013-08-22 00:44:00 PDT
Created attachment 209330 [details]
click() method for objects returned by eventSender.contextClick()
Comment 30 Build Bot 2013-08-22 03:34:09 PDT
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
Comment 31 Build Bot 2013-08-22 03:34:12 PDT
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 32 WebKit Commit Bot 2013-09-16 02:26:16 PDT
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>
Comment 33 WebKit Commit Bot 2013-09-16 02:26:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Alberto Garcia 2013-09-26 09:15:11 PDT
*** Bug 99071 has been marked as a duplicate of this bug. ***