Bug 96200

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

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.

No, patch is coming within 1 ~ 2 weeks.

Created attachment 172039 [details] Working in Progress - 2

Created attachment 172551 [details] Working in Progress - 3 Probably, I might to request review after finishing unit test.

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.

Created attachment 174593 [details] Patch

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.

Created attachment 174670 [details] Patch

All nits are fixed as well as some minor stuffs.

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.

Created attachment 174905 [details] Patch

Created attachment 174907 [details] Patch

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.

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.

Created attachment 174928 [details] Patch

Created attachment 174929 [details] Patch

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()

Created attachment 174935 [details] Patch

Created attachment 174936 [details] Patch

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 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 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

Created attachment 175162 [details] Patch

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 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 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.

(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

Created attachment 175358 [details] Patch

(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 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 on attachment 175358 [details] Patch lgtm now, r=me.

Comment on attachment 175358 [details] Patch Clearing flags on attachment: 175358 Committed r135394: <http://trac.webkit.org/changeset/135394>

All reviewed patches have been landed. Closing bug.