Bug 86091 – [EFL][DRT] EventSender requires contextClick implementation .

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Czech]

EFL' EventSender requires contextClick implementation so that following tests might be removed from Skipped list:

editing/pasteboard/copy-standalone-image-crash.html
editing/selection/5354455-1.html
editing/selection/5354455-2.html
editing/selection/button-right-click.html
editing/selection/context-menu-on-text.html
editing/selection/context-menu-text-selection.html
editing/selection/empty-cell-right-click.html
editing/selection/extend-after-mouse-selection.html
editing/selection/shift-click.html
editing/spelling/context-menu-suggestions.html
editing/spelling/spellcheck-input-search-crash.html
fast/events/context-no-deselect.html
fast/events/context-onmousedown-event.html
fast/events/contextmenu-scrolled-page-with-frame.html
fast/events/menu-keydown-on-hidden-element.html
fast/events/right-click-focus.html
fast/events/selectstart-prevent-selection-on-right-click.html
media/context-menu-actions.html
media/controls-right-click-on-timebar.html

[Michal Pakula vel Rutka]

Created attachment 164529 [details]
patch

This patch contains an contextClick implementation basing on a WebKit GTK one. A change in ewk_context_menu was introduced to allow selecting context menu items without having a pointer to parent menu object.

[Michal Pakula vel Rutka]

Created attachment 164531 [details]
rebased patch

[Gyuyoung Kim]

Comment on attachment 164529 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164529&action=review

We started to add unit test cases for WK1 public API too. Please support it as well.

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56
> +    Ewk_Context_Menu* parentmenu;

Missing comment.

> Source/WebKit/efl/ewk/ewk_contextmenu.h:329
> + * @return a context menu object

You need to mention on failure case as well.

> Source/WebKit/efl/ewk/ewk_contextmenu.h:331
> +EAPI Ewk_Context_Menu           *ewk_context_menu_item_parent_get(Ewk_Context_Menu_Item *o);

Missing const keyword.

> Source/WebKit/efl/ewk/ewk_view.h:2751
> +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(Evas_Object* ewkView);

Missing const keyword for _get function.

[Michal Pakula vel Rutka]

(In reply to comment #3)
> (From update of attachment 164529 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164529&action=review
> 
> We started to add unit test cases for WK1 public API too. Please support it as well.
> 

OK I will add two cases after 94925 will be updated

> > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56
> > +    Ewk_Context_Menu* parentmenu;
> 
> Missing comment.
> 
> > Source/WebKit/efl/ewk/ewk_contextmenu.h:329
> > + * @return a context menu object
> 
> You need to mention on failure case as well.
> 
> > Source/WebKit/efl/ewk/ewk_contextmenu.h:331
> > +EAPI Ewk_Context_Menu           *ewk_context_menu_item_parent_get(Ewk_Context_Menu_Item *o);
> 
> Missing const keyword.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2751
> > +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(Evas_Object* ewkView);
> 
> Missing const keyword for _get function.

Done, I will also add some documentation to ewk_view_context_menu_get

[Michal Pakula vel Rutka]

Created attachment 164885 [details]
unit tests added

Added const modifiers, documentation and unit tests for new and changed API functions. Patch will not build until https://bugs.webkit.org/show_bug.cgi?id=94925 will land again.

[Gyuyoung Kim]

Comment on attachment 164885 [details]
unit tests added

Attachment 164885 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13961004

[Grzegorz Czajkowski]

Comment on attachment 164885 [details]
unit tests added

View in context: https://bugs.webkit.org/attachment.cgi?id=164885&action=review

> LayoutTests/platform/efl-wk2/TestExpectations:206
> +// EFL's EventSender does not implement contextClick yet

// WebKitTestRunner needs an implementation for eventSender.contextClick (https://bugs.webkit.org/show_bug.cgi?id=86881)

> Source/WebKit/PlatformEfl.cmake:331
> +    test_ewk_contextmenu

Please keep alphabetical order.

> Source/WebKit/efl/ewk/ewk_contextmenu.h:326
> + * Gets the parent menu for context menu item

Missing comma.

[Michal Pakula vel Rutka]

Created attachment 166846 [details]
rebased patch

[Michal Pakula vel Rutka]

Created attachment 166851 [details]
rebased again

[Gyuyoung Kim]

Comment on attachment 166851 [details]
rebased again

View in context: https://bugs.webkit.org/attachment.cgi?id=166851&action=review

> LayoutTests/platform/efl/TestExpectations:1291
> +webkit.org/b/86633 editing/spelling/context-menu-suggestions.html [ Failure ]

Wrong alphabetical order.

> Source/WebKit/efl/tests/test_ewk_contextmenu.cpp:67
> +    Eina_Bool itemChecked = EINA_FALSE;

Please use standard boolean. We decided to use standard boolean for test case lately.

> Tools/DumpRenderTree/efl/EventSender.cpp:323
> +static JSValueRef contextClickCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef*    exception)

JSValueRef*    exception -> JSValueRef* exception ?

[Michal Pakula vel Rutka]

Created attachment 166886 [details]
fixes

Fixed according to Gyuyoung comments.

[Chris Dumez]

Comment on attachment 166886 [details]
fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=166886&action=review

> LayoutTests/platform/efl/editing/selection/5354455-2-expected.txt:1
> +layer at (0,0) size 800x600

Pixel expectation?

> LayoutTests/platform/efl/fast/events/context-no-deselect-expected.txt:1
> +layer at (0,0) size 800x600

Pixel expectation?

> Source/WebKit/efl/ewk/ewk_contextmenu.h:203
> +EAPI Ewk_Context_Menu_Item      *ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Action action, Ewk_Context_Menu *submenu, Ewk_Context_Menu *parentmenu, const char *title, Eina_Bool checked, Eina_Bool enabled);

Missing doc for parent menu argument.
Why is parentmenu passed *after* submenu? That seems a bit odd.

> Source/WebKit/efl/ewk/ewk_view.h:2784
> +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(const Evas_Object* o);

Stars on wrong side.

[Michal Pakula vel Rutka]

Comment on attachment 166886 [details]
fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=166886&action=review

>> LayoutTests/platform/efl/editing/selection/5354455-2-expected.txt:1
>> +layer at (0,0) size 800x600
> 
> Pixel expectation?

already did by Raphael

>> LayoutTests/platform/efl/fast/events/context-no-deselect-expected.txt:1
>> +layer at (0,0) size 800x600
> 
> Pixel expectation?

same as above

>> Source/WebKit/efl/ewk/ewk_contextmenu.h:203
>> +EAPI Ewk_Context_Menu_Item      *ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Action action, Ewk_Context_Menu *submenu, Ewk_Context_Menu *parentmenu, const char *title, Eina_Bool checked, Eina_Bool enabled);
> 
> Missing doc for parent menu argument.
> Why is parentmenu passed *after* submenu? That seems a bit odd.

doc added, I will move parentmenu before submenu

>> Source/WebKit/efl/ewk/ewk_view.h:2784
>> +EAPI Ewk_Context_Menu* ewk_view_context_menu_get(const Evas_Object* o);
> 
> Stars on wrong side.

fixed

[Michal Pakula vel Rutka]

Created attachment 168705 [details]
fixed patch

[Michal Pakula vel Rutka]

Created attachment 171413 [details]
rebased patch

rebased patch with one pixel test expectation added and TestExpectations file updated due spellcheck changes

[Chris Dumez]

Comment on attachment 171413 [details]
rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171413&action=review

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56
> +    Ewk_Context_Menu* parentmenu; /**< contains the pointer to parent menu of the item */

parent_menu would be the usual EFL style, right?

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:102
> +Ewk_Context_Menu_Item* ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Action action, Ewk_Context_Menu* parentmenu,

I would prefer "parentMenu" to follow WebKit coding style.

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:210
> +    return item->parentmenu;

We usually have a blank line before return statements.

> Source/WebKit/efl/ewk/ewk_view.cpp:4740
> +    return priv->contextMenu;

blank line before.

> Source/WebKit/efl/tests/test_ewk_contextmenu.cpp:69
> +    Ewk_Context_Menu_Item* contextMenuItem = ewk_context_menu_item_new(itemType, itemAction, contextMenu, 0, itemTitle, itemChecked, itemEnabled);

never freed?

> Tools/DumpRenderTree/efl/EventSender.cpp:364
> +    return valueRef;

Blank line before return.

[Michal Pakula vel Rutka]

Comment on attachment 171413 [details]
rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171413&action=review

>> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:56
>> +    Ewk_Context_Menu* parentmenu; /**< contains the pointer to parent menu of the item */
> 
> parent_menu would be the usual EFL style, right?

as it is "private" structure I will use parentMenu as below

[Michal Pakula vel Rutka]

Created attachment 171465 [details]
fixed patch

Fixed patch according to Christophe's comments.

[Chris Dumez]

Comment on attachment 171465 [details]
fixed patch

LGTM.

[Chris Dumez]

Comment on attachment 171465 [details]
fixed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171465&action=review

> LayoutTests/ChangeLog:3
> +        [EFL][DRT] EventSender requires contextClick implementation.

Sorry I did not notice that before but we do not usually end bug title with a period (.)

[Michal Pakula vel Rutka]

Created attachment 171591 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 171591 [details]
patch for landing

Clearing flags on attachment: 171591

Committed r133000: <http://trac.webkit.org/changeset/133000>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.