Bug 86091 - [EFL][DRT] EventSender requires contextClick implementation .
Summary: [EFL][DRT] EventSender requires contextClick implementation .
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Michal Pakula vel Rutka
URL:
Keywords:
Depends on: 94925
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 06:16 PDT by Krzysztof Czech
Modified: 2012-10-31 01:59 PDT (History)
7 users (show)

See Also:


Attachments
patch (18.75 KB, patch)
2012-09-18 04:08 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
rebased patch (18.74 KB, patch)
2012-09-18 04:16 PDT, Michal Pakula vel Rutka
gyuyoung.kim: review-
Details | Formatted Diff | Diff
unit tests added (24.95 KB, patch)
2012-09-20 03:41 PDT, Michal Pakula vel Rutka
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
rebased patch (27.46 KB, patch)
2012-10-03 03:35 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
rebased again (27.42 KB, patch)
2012-10-03 03:54 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
fixes (27.44 KB, patch)
2012-10-03 06:59 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
fixed patch (27.83 KB, patch)
2012-10-15 07:00 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
rebased patch (32.85 KB, patch)
2012-10-30 04:05 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
fixed patch (32.96 KB, patch)
2012-10-30 09:19 PDT, Michal Pakula vel Rutka
gyuyoung.kim: review+
Details | Formatted Diff | Diff
patch for landing (33.01 KB, patch)
2012-10-31 00:28 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2012-05-10 06:16:56 PDT
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
Comment 1 Michal Pakula vel Rutka 2012-09-18 04:08:19 PDT
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.
Comment 2 Michal Pakula vel Rutka 2012-09-18 04:16:49 PDT
Created attachment 164531 [details]
rebased patch
Comment 3 Gyuyoung Kim 2012-09-18 04:18:21 PDT
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.
Comment 4 Michal Pakula vel Rutka 2012-09-18 04:41:52 PDT
(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
Comment 5 Michal Pakula vel Rutka 2012-09-20 03:41:59 PDT
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.
Comment 6 Gyuyoung Kim 2012-09-20 03:47:44 PDT
Comment on attachment 164885 [details]
unit tests added

Attachment 164885 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13961004
Comment 7 Grzegorz Czajkowski 2012-09-20 06:25:01 PDT
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.
Comment 8 Michal Pakula vel Rutka 2012-10-03 03:35:14 PDT
Created attachment 166846 [details]
rebased patch
Comment 9 Michal Pakula vel Rutka 2012-10-03 03:54:24 PDT
Created attachment 166851 [details]
rebased again
Comment 10 Gyuyoung Kim 2012-10-03 06:18:26 PDT
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 ?
Comment 11 Michal Pakula vel Rutka 2012-10-03 06:59:20 PDT
Created attachment 166886 [details]
fixes

Fixed according to Gyuyoung comments.
Comment 12 Chris Dumez 2012-10-15 06:00:00 PDT
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.
Comment 13 Michal Pakula vel Rutka 2012-10-15 06:47:33 PDT
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
Comment 14 Michal Pakula vel Rutka 2012-10-15 07:00:00 PDT
Created attachment 168705 [details]
fixed patch
Comment 15 Michal Pakula vel Rutka 2012-10-30 04:05:27 PDT
Created attachment 171413 [details]
rebased patch

rebased patch with one pixel test expectation added and TestExpectations file updated due spellcheck changes
Comment 16 Chris Dumez 2012-10-30 08:04:44 PDT
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.
Comment 17 Michal Pakula vel Rutka 2012-10-30 08:49:47 PDT
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
Comment 18 Michal Pakula vel Rutka 2012-10-30 09:19:24 PDT
Created attachment 171465 [details]
fixed patch

Fixed patch according to Christophe's comments.
Comment 19 Chris Dumez 2012-10-30 10:26:43 PDT
Comment on attachment 171465 [details]
fixed patch

LGTM.
Comment 20 Chris Dumez 2012-10-30 10:28:31 PDT
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 (.)
Comment 21 Michal Pakula vel Rutka 2012-10-31 00:28:24 PDT
Created attachment 171591 [details]
patch for landing
Comment 22 WebKit Review Bot 2012-10-31 01:59:19 PDT
Comment on attachment 171591 [details]
patch for landing

Clearing flags on attachment: 171591

Committed r133000: <http://trac.webkit.org/changeset/133000>
Comment 23 WebKit Review Bot 2012-10-31 01:59:25 PDT
All reviewed patches have been landed.  Closing bug.