Bug 96200 - [EFL][WK2] Support Context Menu
Summary: [EFL][WK2] Support Context Menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 102065
Blocks: 61838 102932
  Show dependency treegraph
 
Reported: 2012-09-09 02:01 PDT by Gyuyoung Kim
Modified: 2012-11-21 06:25 PST (History)
7 users (show)

See Also:


Attachments
Working in Progress - please do not review. (6.89 KB, patch)
2012-09-26 22:24 PDT, Gyuyoung Kim
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Working in Progress - 2 (36.70 KB, patch)
2012-11-02 04:56 PDT, Gyuyoung Kim
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Working in Progress - 3 (53.87 KB, patch)
2012-11-06 04:39 PST, Gyuyoung Kim
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Working in Progress - 4 (60.60 KB, patch)
2012-11-12 04:55 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (67.74 KB, patch)
2012-11-15 19:40 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (69.76 KB, patch)
2012-11-16 06:26 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (67.53 KB, patch)
2012-11-19 00:24 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (67.57 KB, patch)
2012-11-19 00:37 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (68.12 KB, patch)
2012-11-19 02:40 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (68.13 KB, patch)
2012-11-19 02:44 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (68.62 KB, patch)
2012-11-19 03:10 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (68.57 KB, patch)
2012-11-19 03:21 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (74.27 KB, patch)
2012-11-20 00:01 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (74.67 KB, patch)
2012-11-21 00:28 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2012-09-09 02:01:17 PDT
EFL WK2 needs to support WebContextMenuProxy for context menu. Patch is coming.
Comment 1 Gyuyoung Kim 2012-09-26 22:24:20 PDT
Created attachment 165928 [details]
Working in Progress - please do not review.
Comment 2 Chris Dumez 2012-10-30 12:17:34 PDT
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.
Comment 3 Gyuyoung Kim 2012-10-30 15:31:15 PDT
No, patch is coming within 1 ~ 2 weeks.
Comment 4 Gyuyoung Kim 2012-11-02 04:56:06 PDT
Created attachment 172039 [details]
Working in Progress - 2
Comment 5 Gyuyoung Kim 2012-11-06 04:39:37 PST
Created attachment 172551 [details]
Working in Progress - 3

Probably, I might to request review after finishing unit test.
Comment 6 Gyuyoung Kim 2012-11-12 04:55:32 PST
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.
Comment 7 Gyuyoung Kim 2012-11-15 19:40:55 PST
Created attachment 174593 [details]
Patch
Comment 8 Chris Dumez 2012-11-15 23:04:25 PST
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.
Comment 9 Gyuyoung Kim 2012-11-16 06:26:23 PST
Created attachment 174670 [details]
Patch
Comment 10 Gyuyoung Kim 2012-11-16 06:28:07 PST
All nits are fixed as well as some minor stuffs.
Comment 11 Chris Dumez 2012-11-18 22:20:59 PST
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.
Comment 12 Gyuyoung Kim 2012-11-19 00:24:53 PST
Created attachment 174905 [details]
Patch
Comment 13 Gyuyoung Kim 2012-11-19 00:37:17 PST
Created attachment 174907 [details]
Patch
Comment 14 Chris Dumez 2012-11-19 01:01:54 PST
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.
Comment 15 Chris Dumez 2012-11-19 01:07:08 PST
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.
Comment 16 Gyuyoung Kim 2012-11-19 02:40:04 PST
Created attachment 174928 [details]
Patch
Comment 17 Gyuyoung Kim 2012-11-19 02:44:17 PST
Created attachment 174929 [details]
Patch
Comment 18 Chris Dumez 2012-11-19 02:57:25 PST
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()
Comment 19 Gyuyoung Kim 2012-11-19 03:10:57 PST
Created attachment 174935 [details]
Patch
Comment 20 Gyuyoung Kim 2012-11-19 03:21:08 PST
Created attachment 174936 [details]
Patch
Comment 21 Gyuyoung Kim 2012-11-19 03:24:19 PST
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.
Comment 22 Chris Dumez 2012-11-19 03:43:29 PST
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.
Comment 23 Chris Dumez 2012-11-19 03:47:21 PST
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
Comment 24 Gyuyoung Kim 2012-11-20 00:01:27 PST
Created attachment 175162 [details]
Patch
Comment 25 Gyuyoung Kim 2012-11-20 00:07:06 PST
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.
Comment 26 Laszlo Gombos 2012-11-20 21:25:22 PST
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 ?
Comment 27 Chris Dumez 2012-11-20 21:50:50 PST
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.
Comment 28 Gyuyoung Kim 2012-11-20 22:01:25 PST
(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
Comment 29 Gyuyoung Kim 2012-11-21 00:28:54 PST
Created attachment 175358 [details]
Patch
Comment 30 Gyuyoung Kim 2012-11-21 00:30:19 PST
(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.
Comment 31 WebKit Review Bot 2012-11-21 01:23:31 PST
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
Comment 32 Laszlo Gombos 2012-11-21 06:05:59 PST
Comment on attachment 175358 [details]
Patch

lgtm now, r=me.
Comment 33 WebKit Review Bot 2012-11-21 06:25:36 PST
Comment on attachment 175358 [details]
Patch

Clearing flags on attachment: 175358

Committed r135394: <http://trac.webkit.org/changeset/135394>
Comment 34 WebKit Review Bot 2012-11-21 06:25:42 PST
All reviewed patches have been landed.  Closing bug.