Bug 86881 - [WK2][WKTR] WebKitTestRunner needs eventSender.contextClick()
Summary: [WK2][WKTR] WebKitTestRunner needs eventSender.contextClick()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wojciech Bielawski
URL:
Keywords:
Depends on:
Blocks: 42194
  Show dependency treegraph
 
Reported: 2012-05-18 12:06 PDT by Caio Marcelo de Oliveira Filho
Modified: 2012-11-23 04:56 PST (History)
9 users (show)

See Also:


Attachments
eventSender.contextClick() method partially implemented. (13.16 KB, patch)
2012-09-24 01:31 PDT, Wojciech Bielawski
buildbot: commit-queue-
Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (13.12 KB, patch)
2012-09-24 06:07 PDT, Wojciech Bielawski
buildbot: commit-queue-
Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (23.29 KB, patch)
2012-10-04 03:21 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (17.81 KB, patch)
2012-10-04 05:14 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (17.57 KB, patch)
2012-10-04 07:58 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (18.01 KB, patch)
2012-10-16 03:31 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (17.96 KB, patch)
2012-10-16 05:17 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (25.30 KB, patch)
2012-10-24 06:32 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (21.11 KB, patch)
2012-10-25 02:18 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (21.09 KB, patch)
2012-10-25 06:53 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (21.40 KB, patch)
2012-10-26 01:14 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (21.40 KB, patch)
2012-10-30 07:31 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (17.39 KB, patch)
2012-10-31 07:44 PDT, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (18.97 KB, patch)
2012-11-15 03:45 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (19.02 KB, patch)
2012-11-15 05:28 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (18.72 KB, patch)
2012-11-21 06:34 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (18.72 KB, patch)
2012-11-21 06:45 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (18.75 KB, patch)
2012-11-21 07:04 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (19.38 KB, patch)
2012-11-23 03:45 PST, Wojciech Bielawski
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
eventSender.contextClick() method partially implemented. (19.49 KB, patch)
2012-11-23 04:38 PST, Wojciech Bielawski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2012-05-18 12:06:30 PDT
Tests that depend on this according to grep:

editing/pasteboard/copy-standalone-image-crash.html:50:    actionitems = eventSender.contextClick();
editing/selection/5354455-1.html:23:    eventSender.contextClick();
editing/selection/5354455-2.html:33:    eventSender.contextClick();
editing/selection/5354455-2.html:40:    eventSender.contextClick();
editing/selection/button-right-click.html:22:    eventSender.contextClick();
editing/selection/context-menu-on-text.html:22:    var items = eventSender.contextClick();
editing/selection/context-menu-text-selection.html:22:    eventSender.contextClick();
editing/selection/empty-cell-right-click.html:23:    eventSender.contextClick();
editing/shadow/rightclick-on-meter-in-shadow-crash.html:15:    eventSender.contextClick();
editing/spelling/context-menu-suggestions.html:25:    var itemNamesWithoutSpellChecking = eventSender.contextClick();
editing/spelling/context-menu-suggestions.html:30:    var itemNamesWithSpellChecking = eventSender.contextClick();
editing/spelling/spellcheck-input-search-crash.html:22:    eventSender.contextClick();
fast/events/context-no-deselect.html:19:    eventSender.contextClick();
fast/events/context-onmousedown-event.html:16:     eventSender.contextClick();
fast/events/contextmenu-scrolled-page-with-frame.html:26:        eventSender.contextClick();
fast/events/right-click-focus.html:19:    eventSender.contextClick();
fast/events/selectstart-prevent-selection-on-right-click.html:27:    eventSender.contextClick();
media/context-menu-actions.html:26:                items = eventSender.contextClick();
media/controls-right-click-on-timebar.html:25:                    eventSender.contextClick();
Comment 1 Jussi Kukkonen (jku) 2012-06-08 02:56:32 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.
Comment 2 Jussi Kukkonen (jku) 2012-06-08 03:49:15 PDT
(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.
Comment 3 Wojciech Bielawski 2012-09-11 03:29:50 PDT
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
Comment 4 Wojciech Bielawski 2012-09-21 01:34:29 PDT
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.
Comment 5 Jussi Kukkonen (jku) 2012-09-21 03:09:02 PDT
(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.
Comment 6 Wojciech Bielawski 2012-09-24 01:31:35 PDT
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.
Comment 7 Chris Dumez 2012-09-24 01:55:41 PDT
missing [WK2][WKTR] in bug title.
Comment 8 Chris Dumez 2012-09-24 02:02:36 PDT
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 9 Build Bot 2012-09-24 02:21:54 PDT
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
Comment 10 Wojciech Bielawski 2012-09-24 06:07:03 PDT
Created attachment 165364 [details]
 eventSender.contextClick() method partially implemented.

Update after  Christophe Dumez review.
Comment 11 Chris Dumez 2012-09-24 06:15:35 PDT
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 12 Build Bot 2012-09-24 06:25:42 PDT
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 13 Chris Dumez 2012-09-24 06:54:37 PDT
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 14 Build Bot 2012-09-24 07:20:42 PDT
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 15 Gyuyoung Kim 2012-09-24 17:56:27 PDT
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.
Comment 16 Wojciech Bielawski 2012-10-04 03:21:35 PDT
Created attachment 167064 [details]
eventSender.contextClick() method partially implemented.
Comment 17 Gyuyoung Kim 2012-10-04 03:30:43 PDT
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()
Comment 18 Wojciech Bielawski 2012-10-04 05:14:07 PDT
Created attachment 167078 [details]
 eventSender.contextClick() method partially implemented.
Comment 19 Wojciech Bielawski 2012-10-04 07:58:02 PDT
Created attachment 167104 [details]
eventSender.contextClick() method partially implemented.
Comment 20 Gyuyoung Kim 2012-10-04 18:16:58 PDT
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.
Comment 21 Wojciech Bielawski 2012-10-05 02:04:49 PDT
(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 22 Gyuyoung Kim 2012-10-05 04:12:45 PDT
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.
Comment 23 Michal Pakula vel Rutka 2012-10-05 07:57:52 PDT
> 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.
Comment 24 Wojciech Bielawski 2012-10-16 02:48:36 PDT
All reported problems were fixed. Gyuyoung would you take a look on it?
Comment 25 Wojciech Bielawski 2012-10-16 03:31:47 PDT
Created attachment 168909 [details]
eventSender.contextClick() method partially implemented.

Corrections after Michal's review added.
Comment 26 Wojciech Bielawski 2012-10-16 05:17:41 PDT
Created attachment 168928 [details]
eventSender.contextClick() method partially implemented.

More corrections after next Michal's review.
Comment 27 Wojciech Bielawski 2012-10-24 06:32:25 PDT
Created attachment 170381 [details]
eventSender.contextClick() method partially implemented.

Added gardening related with contextClick to GTK platform.
Comment 28 Wojciech Bielawski 2012-10-25 02:18:32 PDT
Created attachment 170593 [details]
eventSender.contextClick() method partially implemented.

Previous path was wrongly prepared.
Comment 29 WebKit Review Bot 2012-10-25 02:23:27 PDT
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.
Comment 30 Wojciech Bielawski 2012-10-25 06:53:35 PDT
Created attachment 170634 [details]
eventSender.contextClick() method partially implemented.

Rebased without webkit style problems.
Comment 31 Jussi Kukkonen (jku) 2012-10-26 00:40:24 PDT
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.
Comment 32 Jussi Kukkonen (jku) 2012-10-26 00:42:25 PDT
(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.
Comment 33 Wojciech Bielawski 2012-10-26 01:14:48 PDT
Created attachment 170844 [details]
eventSender.contextClick() method partially implemented.

Corrections after Jussi Kukkonen review.
Comment 34 Wojciech Bielawski 2012-10-30 07:31:22 PDT
Created attachment 171449 [details]
eventSender.contextClick() method partially implemented.

I've missed last remark from Jussi. Now all remarks are corrected.
Comment 35 Wojciech Bielawski 2012-10-31 07:44:27 PDT
Created attachment 171659 [details]
eventSender.contextClick() method partially implemented.

Rebased with newest changes (contextClick for webkit1)
Comment 36 Chris Dumez 2012-11-05 05:41:30 PST
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 37 Mikhail Pozdnyakov 2012-11-05 05:46:15 PST
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.
Comment 38 Wojciech Bielawski 2012-11-15 03:45:11 PST
Created attachment 174396 [details]
eventSender.contextClick() method partially implemented.

Corrections after review.
Comment 39 Wojciech Bielawski 2012-11-15 05:28:37 PST
Created attachment 174413 [details]
eventSender.contextClick() method partially implemented.

Rebased with newest changes
Comment 40 Kenneth Rohde Christiansen 2012-11-21 02:08:09 PST
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 41 Mikhail Pozdnyakov 2012-11-21 03:33:40 PST
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?
Comment 42 Wojciech Bielawski 2012-11-21 06:34:37 PST
Created attachment 175428 [details]
eventSender.contextClick() method partially implemented.

Corrections after review(s).
Comment 43 WebKit Review Bot 2012-11-21 06:36:55 PST
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 44 Kenneth Rohde Christiansen 2012-11-21 06:39:33 PST
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
Comment 45 Wojciech Bielawski 2012-11-21 06:45:43 PST
Created attachment 175431 [details]
eventSender.contextClick() method partially implemented.

Style corrected.
Comment 46 Wojciech Bielawski 2012-11-21 07:04:52 PST
Created attachment 175435 [details]
eventSender.contextClick() method partially implemented.

Corrections after Kenneth review.
Comment 47 Thiago Marcos P. Santos 2012-11-22 08:06:42 PST
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.
Comment 48 Wojciech Bielawski 2012-11-23 03:45:55 PST
Created attachment 175772 [details]
eventSender.contextClick() method partially implemented.

Corrections after Thiago review
Comment 49 WebKit Review Bot 2012-11-23 04:31:02 PST
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
Comment 50 Wojciech Bielawski 2012-11-23 04:38:10 PST
Created attachment 175779 [details]
eventSender.contextClick() method partially implemented.

Rebased with latest changes
Comment 51 WebKit Review Bot 2012-11-23 04:56:31 PST
Comment on attachment 175779 [details]
eventSender.contextClick() method partially implemented.

Clearing flags on attachment: 175779

Committed r135595: <http://trac.webkit.org/changeset/135595>
Comment 52 WebKit Review Bot 2012-11-23 04:56:38 PST
All reviewed patches have been landed.  Closing bug.