Bug 102932 – [EFL][WK2] Implement context menu in MiniBrowser

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

EFL MiniBrowser needs to support context menu.

[Michal Pakula vel Rutka]

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.

[Chris Dumez]

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?

[Gyuyoung Kim]

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.

[Michal Pakula vel Rutka]

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.

[Chris Dumez]

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.

[Michal Pakula vel Rutka]

Created attachment 183917 [details]
minibrowser-only patch

I have split this patch into two: Minibrowser only and ewk context menu API: bug 107510.

[Michal Pakula vel Rutka]

Created attachment 183919 [details]
minibrowser-only patch2

[Kenneth Rohde Christiansen]

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?

[Michal Pakula vel Rutka]

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

[Michal Pakula vel Rutka]

Created attachment 184726 [details]
fixed patch

added guard in on_context_menu_hide

[EFL EWS Bot]

Comment on attachment 184726 [details]
fixed patch

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

[Michal Pakula vel Rutka]

Created attachment 198392 [details]
rebased patch

[Gyuyoung Kim]

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 ?

[Michal Pakula vel Rutka]

Created attachment 198475 [details]
fixes after review

changed fill_context_popup_items to context_popup_populate, added logs

[Gyuyoung Kim]

Comment on attachment 198475 [details]
fixes after review

LGTM. But, someone might have a final look.

[Chris Dumez]

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.

[Michal Pakula vel Rutka]

Created attachment 198506 [details]
fixes

[Chris Dumez]

Comment on attachment 198506 [details]
fixes

LGTM.

[WebKit Commit Bot]

Comment on attachment 198506 [details]
fixes

Clearing flags on attachment: 198506

Committed r148609: <http://trac.webkit.org/changeset/148609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.