[EFL][WK2] Support Context Menu

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

EFL WK2 needs to support WebContextMenuProxy for context menu. Patch is coming.

[Gyuyoung Kim]

Created attachment 165928 [details]
Working in Progress - please do not review.

[Chris Dumez]

Any progress on this? I was planning to work on context menu support for WK2 EFL and then I found this bug. I'm happy to take over if you're busy with other things.

[Gyuyoung Kim]

No, patch is coming within 1 ~ 2 weeks.

[Gyuyoung Kim]

Created attachment 172039 [details]
Working in Progress - 2

[Gyuyoung Kim]

Created attachment 172551 [details]
Working in Progress - 3

Probably, I might to request review after finishing unit test.

[Gyuyoung Kim]

Created attachment 173621 [details]
Working in Progress - 4

Sorry for late patch. Unfortunately, I don't finish to support API test yet. Mouse right button simulation doesn't work well.

[Gyuyoung Kim]

Created attachment 174593 [details]
Patch

[Chris Dumez]

Comment on attachment 174593 [details]
Patch

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

Looks good overall, with a few minor comments:

> Source/WebKit2/ChangeLog:8
> +        This patch supports WK2 EFL's context menu as WK1 implemenation because we can't use elementary in WebKit inside.

"implementation", "inside WebKit."

> Source/WebKit2/ChangeLog:9
> +        Basic functionalities of context menu are only supportted.

"Only basic functionalities of context menu are supported."

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:39
> +Ewk_Context_Menu::Ewk_Context_Menu(EwkViewImpl* viewImpl, WebContextMenuProxyEfl* contextMenuProxy, const Vector<WebKit::WebContextMenuItemData>& items)

I think we stopped using underscore in C++ class names, didn't we?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:50
> +{

You should probably free m_contextMenuItems and its items in the destructor.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:94
> +const Eina_List *ewk_context_menu_items_get(const Ewk_Context_Menu* menu)

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:109
> +    WebContextMenuItemData newItem(type, action, item->title(), item->enabled(), item->checked());

We should avoid useless temporary object and pass "WebContextMenuItemData(type, action, item->title(), item->enabled(), item->checked())" in argument to contextMenuItemSelected().

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:36
> +Ewk_Context_Menu_Item::Ewk_Context_Menu_Item(const WebKit::WebContextMenuItemData& item)

I think we stopped using underscore in C++ class names
"WebKit::" is not needed here

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:40
> +    , m_isChecked(item.checked())

Shouldn't we initialize m_parentMenu and m_subMenu?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:45
> +Ewk_Context_Menu_Item::Ewk_Context_Menu_Item(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Item_Action action, Ewk_Context_Menu* parentMenu, Ewk_Context_Menu* subMenu, const char* title, Eina_Bool checked, Eina_Bool enabled)

Looks weird to use Eina_Bool in C++ API

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:140
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(item, false);

0 not false

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.h:199
> +EAPI const char *ewk_context_menu_item_title_set(Ewk_Context_Menu_Item *o, const char *title);

Why are we returning the title here instead of Eina_Bool like usually?

> Source/WebKit2/UIProcess/efl/WebContextMenuProxyEfl.h:50
> +    virtual void showContextMenu(const WebCore::IntPoint&, const Vector<WebContextMenuItemData>&);

This method should probably not be virtual, especially if the destructor isn't.

> Source/WebKit2/UIProcess/efl/WebContextMenuProxyEfl.h:51
> +    virtual void hideContextMenu();

Ditto.

[Gyuyoung Kim]

Created attachment 174670 [details]
Patch

[Gyuyoung Kim]

All nits are fixed as well as some minor stuffs.

[Chris Dumez]

Comment on attachment 174670 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Only basic functionalities of context menu are supportted.

"supported"

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:47
> +        m_contextMenuItems = eina_list_append(m_contextMenuItems, ewk_object_ref(item.get()));

We should really avoid mixing ref'ing functions and smart pointers. Just use release().leakRef()

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:73
> +    RefPtr<Ewk_Context_Menu_Item> newItem = EwkContextMenuItem::create(type, action, ewkParentMenu, ewkSubMenu, title, checked, enabled).get();

looks like the .get() in the end should be removed.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:75
> +    return ewk_object_ref(newItem.get());

Same comment as before, should be release().leakRef(). And actually, you don't need a temporary variable so it can be:
return EwkContextMenuItem::create(type, action, ewkParentMenu, ewkSubMenu, title, checked, enabled).leakRef();

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:78
> +void ewk_context_menu_item_free(Ewk_Context_Menu_Item* item)

Why a free function if the object is ref counted?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:36
> +class EwkContextMenuItem : public Ewk_Object {

Does it really need to be ref counted? Since you're using a free function in the public API, it does not seem so. If you subclass Ewk_Object, the client can call ewk_object_ref / unref() on this object. In my opinion, you should stop subclassing the Ewk_Object and keep the public free function.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_private.h:42
> +class EwkContextMenu : public Ewk_Object {

sane here, does the context menu really need to be ref counted? It belongs to the view.

[Gyuyoung Kim]

Created attachment 174905 [details]
Patch

[Gyuyoung Kim]

Created attachment 174907 [details]
Patch

[Chris Dumez]

Comment on attachment 174907 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:63
> +    EwkContextMenuItem* newItem = EwkContextMenuItem::create(type, action, parentMenu, subMenu, title, checked, enabled).leakPtr();

Note: We try to avoid these useless temporary variables to facilitate return value optimization.

It would be better to return EwkContextMenuItem::create(type, action, parentMenu, subMenu, title, checked, enabled).leakPtr() directly.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:58
> +EAPI Ewk_Context_Menu_Item *ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Item_Action action, Ewk_Context_Menu *parent_menu, Ewk_Context_Menu *sub_menu, const char *title, Eina_Bool checked, Eina_Bool enabled);

Why is this function part of ewk_context_menu.h and not ewk_context_menu_item.h?
Also, why do we need this public API? Once the browser constructs a context menu item, what can it do with it? I don't see any API to add a context menu item to an existing menu, and this function does not seem to alter the parent context menu internally.

Maybe it should be called ewk_context_menu_item_add() and the implementation should update the parent menu internally (i.e. m_contextMenuItems)? Also, in practice, the parent menu should probably own the memory so I'm not sure we need a public free function.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:63
> + * @param item the item to destory

"destroy"

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:68
> +EAPI void ewk_context_menu_item_free(Ewk_Context_Menu_Item *item);

Ditto.

[Chris Dumez]

Checking GTK port, it seems they have the following public API:
  webkit_context_menu_prepend(menu, item)
  webkit_context_menu_append(menu, item)
  webkit_context_menu_insert(menu, item, position)
  webkit_context_menu_remove(menu, item)

May be nice to have something similar. Currently we can construct an item but not add it or remove it from a menu it seems.

[Gyuyoung Kim]

Created attachment 174928 [details]
Patch

[Gyuyoung Kim]

Created attachment 174929 [details]
Patch

[Chris Dumez]

Comment on attachment 174929 [details]
Patch

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

> Source/WebKit2/ChangeLog:25
> +        (ewk_context_menu_item_new):

Changelog looks outdated?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:56
> +EAPI Ewk_Context_Menu_Item *ewk_context_menu_item_append(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Item_Action action, const char *title, Eina_Bool checked, Eina_Bool enabled, Ewk_Context_Menu *parent_menu, Ewk_Context_Menu *sub_menu);

I don't think it is a good idea to hide the item constructor inside the append() function. I believe we should have 2 separate functions:
ewk_context_menu_item_new()
ewk_context_menu_item_append(item)

This is more extensible because later on we will likely add more (convenience) item constructors, e.g.:
ewk_context_menu_item_separator_new()
ewk_context_menu_item_with_submenu_new()

[Gyuyoung Kim]

Created attachment 174935 [details]
Patch

[Gyuyoung Kim]

Created attachment 174936 [details]
Patch

[Gyuyoung Kim]

I also thought to implement this patch as GTK port. There was some faults in previous patch. I think ewk_context_menu.h is proper place for _new, _append and _remove because there is Ewk_Context_Menu dependency.

[Chris Dumez]

Comment on attachment 174936 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:56
> +EAPI Ewk_Context_Menu_Item *ewk_context_menu_item_new(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Item_Action action, const char *title, Eina_Bool checked, Eina_Bool enabled, Ewk_Context_Menu *parent_menu, Ewk_Context_Menu *sub_menu);

I don't understand why this is part of ewk_context_menu.h header. This is API to construct a context menu ITEM so it should be part of ewk_context_menu_item.h. Also, I don't think we should pass parent_menu.
The item should be added using ewk_context_menu_item_append() and parent_menu can be populated then.

[Chris Dumez]

Comment on attachment 174936 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:78
> +    Ewk_Context_Menu_Item* newItem = Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, parentMenu, subMenu).leakPtr();

Same comment as before: why not return the value directly. We usually avoid this kind of useless temporary variables in WebKit AFAIK.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:85
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(menu, 0);

false not 0

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:94
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(menu, 0);

false not 0

[Gyuyoung Kim]

Created attachment 175162 [details]
Patch

[Gyuyoung Kim]

I also think GTK implementation makes sense. So, I add below APIs to make submenu ,

- EAPI Ewk_Context_Menu *ewk_context_menu_new(void);
- EAPI Ewk_Context_Menu *ewk_context_menu_new_with_items(Eina_List *items);

Application can make submenu, then can add it to new item by ewk_context_menu_item_new_with_submenu.

Though there are missing APIs compare to GTK port, I think current APIs can cover basic functionality. As GTK port, we can add new item to parentMenu by  ewk_context_menu_item_append, Or remove it via ewk_context_menu_item_remove.

[Laszlo Gombos]

Comment on attachment 175162 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:50
> +EAPI Ewk_Context_Menu *ewk_context_menu_new(void);

Can the test case include a call for this API function as well ?

[Chris Dumez]

Comment on attachment 175162 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_defines.h:27
> + * @file    ewk_defines.h

Why this extra file? Can we get rid of it somehow? It seems really overkill to create a new public header just for defining ewk struct typedefs.

[Gyuyoung Kim]

(In reply to comment #27)
> (From update of attachment 175162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175162&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_defines.h:27
> > + * @file    ewk_defines.h
> 
> Why this extra file? Can we get rid of it somehow? It seems really overkill to create a new public header just for defining ewk struct typedefs.

I talked this with Laszlo. Alternative method is we move Ewk_Context_Menu to ewk_context_menu_item.h. But, it looks this doesn't make sense as well. If you have good solution, please let me know. In GTK port case, they are using webkitdefines.h to solve this issue.

It looks there was discussion for this issue before.
http://stackoverflow.com/questions/621356/undefined-c-struct-forward-declaration

[Gyuyoung Kim]

Created attachment 175358 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #26)
> (From update of attachment 175162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175162&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context_menu.h:50
> > +EAPI Ewk_Context_Menu *ewk_context_menu_new(void);
> 
> Can the test case include a call for this API function as well ?

I add a test code to test it as well as to fix wrong test case code you pointed out.

[WebKit Review Bot]

Comment on attachment 175358 [details]
Patch

Attachment 175358 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14933650

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Laszlo Gombos]

Comment on attachment 175358 [details]
Patch

lgtm now, r=me.

[WebKit Review Bot]

Comment on attachment 175358 [details]
Patch

Clearing flags on attachment: 175358

Committed r135394: <http://trac.webkit.org/changeset/135394>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.