Bug 102932

Created attachment 183799 [details] proposed patch Elementary context popup after selecting an item, fires a callback which returns data set by user while creating an context popup item. Ewk context menu API when selecting an item, needs two pointers to be passed: a pointer to a menu and to an item. To make implementation easier I have added an API to receive Ewk_Context_Menu_Item's parent menu, so only Ewk_Context_Menu_Item have to be passed to a callback fired when context popup item was selected.

Comment on attachment 183799 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=183799&action=review I think it would be nicer to split WK2 API changes from MiniBrowser changes. > Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:66 > + EwkContextMenu* parentMenu() const { return m_parentMenu; } WebKit coding style: The getter should not be const if it returns a non-const pointer. > Tools/MiniBrowser/efl/main.c:94 > + Evas_Object *context_popup; This should probably be initialized to NULL in window_create(). > Tools/MiniBrowser/efl/main.c:910 > + Ewk_Context_Menu_Item *ewk_item = (Ewk_Context_Menu_Item *)data; extra space after = > Tools/MiniBrowser/efl/main.c:1286 > + ewk_settings_continuous_spell_checking_enabled_set(EINA_TRUE); seems unrelated?

Comment on attachment 183799 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=183799&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.h:273 > +EAPI Ewk_Context_Menu *ewk_context_menu_item_parent_menu_get(Ewk_Context_Menu_Item *o); Missing *const* keyword in parameter. > Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:70 > + explicit EwkContextMenuItem(const WebKit::WebContextMenuItemData&, EwkContextMenu* parentMenu); Please remove explicit keyword.

Comment on attachment 183799 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=183799&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.h:273 >> +EAPI Ewk_Context_Menu *ewk_context_menu_item_parent_menu_get(Ewk_Context_Menu_Item *o); > > Missing *const* keyword in parameter. Added. >> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:66 >> + EwkContextMenu* parentMenu() const { return m_parentMenu; } > > WebKit coding style: The getter should not be const if it returns a non-const pointer. If I have added const to ewk_context_menu_item_parent_menu_get it has to be left here. >> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:70 >> + explicit EwkContextMenuItem(const WebKit::WebContextMenuItemData&, EwkContextMenu* parentMenu); > > Please remove explicit keyword. Removed. >> Tools/MiniBrowser/efl/main.c:94 >> + Evas_Object *context_popup; > > This should probably be initialized to NULL in window_create(). Added. >> Tools/MiniBrowser/efl/main.c:910 >> + Ewk_Context_Menu_Item *ewk_item = (Ewk_Context_Menu_Item *)data; > > extra space after = OK >> Tools/MiniBrowser/efl/main.c:1286 >> + ewk_settings_continuous_spell_checking_enabled_set(EINA_TRUE); > > seems unrelated? I forgot to remove it after tests.

Comment on attachment 183799 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=183799&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:66 >>> + EwkContextMenu* parentMenu() const { return m_parentMenu; } >> >> WebKit coding style: The getter should not be const if it returns a non-const pointer. > > If I have added const to ewk_context_menu_item_parent_menu_get it has to be left here. No, you don't have to leave const here and you should not. Just use const cast in ewk_context_menu_item_parent_menu_get() if needed.

Created attachment 183917 [details] minibrowser-only patch I have split this patch into two: Minibrowser only and ewk context menu API: bug 107510.

Created attachment 183919 [details] minibrowser-only patch2

Comment on attachment 183917 [details] minibrowser-only patch View in context: https://bugs.webkit.org/attachment.cgi?id=183917&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:153 > EINA_SAFETY_ON_NULL_RETURN_VAL(item, 0); > > - return item->parentMenu(); > + return const_cast<Ewk_Context_Menu_Item*>(item)->parentMenu(); > } This code needs review by WebKit2 owner, so could it be in a separate opatch somehow?

(In reply to comment #8) > (From update of attachment 183917 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183917&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:153 > > EINA_SAFETY_ON_NULL_RETURN_VAL(item, 0); > > > > - return item->parentMenu(); > > + return const_cast<Ewk_Context_Menu_Item*>(item)->parentMenu(); > > } > > This code needs review by WebKit2 owner, so could it be in a separate opatch somehow? I posted this patch accidentally and it is obsolete, please take a look at 183919. This code is moved to bug 107510.

Created attachment 184726 [details] fixed patch added guard in on_context_menu_hide

Comment on attachment 184726 [details] fixed patch Attachment 184726 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16367762

Created attachment 198392 [details] rebased patch

Comment on attachment 198392 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=198392&action=review > Tools/MiniBrowser/efl/main.c:1107 > + Unneeded line. > Tools/MiniBrowser/efl/main.c:1123 > +fill_context_popup_items(Browser_Window *window, Ewk_Context_Menu *ewk_menu) verb is placed at the end of function in EFL coding style. See also, popup_menu_populate(...) in main.cp > Tools/MiniBrowser/efl/main.c:1157 > + if (!window || !menu) It would be good if you add a log using info(...). > Tools/MiniBrowser/efl/main.c:1178 > + if (!window || !window->context_popup) ditto ?

Created attachment 198475 [details] fixes after review changed fill_context_popup_items to context_popup_populate, added logs

Comment on attachment 198475 [details] fixes after review LGTM. But, someone might have a final look.

Comment on attachment 198475 [details] fixes after review View in context: https://bugs.webkit.org/attachment.cgi?id=198475&action=review > Tools/MiniBrowser/efl/main.c:1389 > + window->context_popup = NULL; Please double check but I don't believe this is needed. If I remember correctly the window memory is calloc'd.

Created attachment 198506 [details] fixes

Comment on attachment 198506 [details] fixes LGTM.

Comment on attachment 198506 [details] fixes Clearing flags on attachment: 198506 Committed r148609: <http://trac.webkit.org/changeset/148609>

All reviewed patches have been landed. Closing bug.