Bug 114729 – [EFL][WK2] Support sub menu in ewk context menus

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Michal Pakula vel Rutka]

SSIA

[Michal Pakula vel Rutka]

Created attachment 198696 [details]
proposed patch

[Mikhail Pozdnyakov]

Comment on attachment 198696 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:46
> +    , m_subMenu()

this should be omitted, default ctor will be invoked anyway.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:62
> +    m_subMenu = adoptPtr(subMenu);

adopting of passing args in the ctor is opaque and error prone, argument is better to be PassOwnPtr<> and adopted on caller side.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_private.h:61
> +    EwkView* viewImpl() const { return m_viewImpl; }

ewkView() is better.

[Mikhail Pozdnyakov]

Comment on attachment 198696 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:70
> +    EwkContextMenu* subMenu() const { return m_subMenu.get(); }

don't think it should be const, you're returning class-owned data.

[Michal Pakula vel Rutka]

Created attachment 199394 [details]
fixes

[Mikhail Pozdnyakov]

Comment on attachment 199394 [details]
fixes

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:49
> +        return adoptPtr(new EwkContextMenuItem(type, action, title, checked, enabled, adoptPtr(subMenu), parentMenu));

who calls it? can create() function pass PassOwnPtr<EwkContextMenu> as well?

[Michal Pakula vel Rutka]

Comment on attachment 199394 [details]
fixes

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

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:49
>> +        return adoptPtr(new EwkContextMenuItem(type, action, title, checked, enabled, adoptPtr(subMenu), parentMenu));
> 
> who calls it? can create() function pass PassOwnPtr<EwkContextMenu> as well?

This create() is called by API functions: ewk_context_menu_item_new or new_with_submenu. I can move adoption of subMenu there (http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp#L60).

[Michal Pakula vel Rutka]

Created attachment 199620 [details]
fixes

moved adoption of submenu pointer to ewk_context_menu_item.cpp

[Mikhail Pozdnyakov]

Comment on attachment 199620 [details]
fixes

Agreed on IRC that menu is better to be shareable (ewk object)

[Michal Pakula vel Rutka]

Created attachment 199812 [details]
changed EwkContextMenu to EwkObject

[Mikhail Pozdnyakov]

Comment on attachment 199812 [details]
changed EwkContextMenu to EwkObject

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:71
> +    return Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, adoptRef(subMenu)).leakPtr();

no need to adopt since it's shared now, right?

[Michal Pakula vel Rutka]

Created attachment 201817 [details]
patch

changing EwkContextMenu to EwkObject moved to patch 116097

[Mikhail Pozdnyakov]

Comment on attachment 201817 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:66
> +    return Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, PassRefPtr<EwkContextMenu>()).leakPtr();

it's better to keep '0'

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:73
> +    return Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, PassRefPtr<EwkContextMenu>(subMenuImpl)).leakPtr();

should be simply subMenuImpl

[Mikhail Pozdnyakov]

Comment on attachment 201817 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:167
> +    return const_cast<Ewk_Context_Menu_Item*>(item)->subMenu();

maybe Ewk_Context_Menu_Item should also be an EwkObject? and we probably should get rid of "ewk_defines.h" file

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:86
>      EwkContextMenu* m_parentMenu;

shouldn't it be RefPtr also?

[Chris Dumez]

Comment on attachment 201817 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:66
> +    return Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, PassRefPtr<EwkContextMenu>()).leakPtr();

I would prefer if we omit the submenu argument.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:160
>      return const_cast<Ewk_Context_Menu_Item*>(item)->parentMenu();

const_cast is not actually needed here. Do you mind fixing it at the same time?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:49
> +    static PassOwnPtr<EwkContextMenuItem> create(Ewk_Context_Menu_Item_Type type, Ewk_Context_Menu_Item_Action action, const char* title, Eina_Bool checked, Eina_Bool enabled, PassRefPtr<EwkContextMenu> subMenu, EwkContextMenu* parentMenu = 0)

Could you add a default value for the subMenu argument?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:72
> +    EwkContextMenu* subMenu() { return m_subMenu.get(); }

I would make this getter const. Mikhail won't like it but it is consistent with parentMenu() above and it avoids the const_cast() in the caller.

[Michal Pakula vel Rutka]

Comment on attachment 201817 [details]
patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:66
>>> +    return Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, PassRefPtr<EwkContextMenu>()).leakPtr();
>> 
>> it's better to keep '0'
> 
> I would prefer if we omit the submenu argument.

I will change change ::create method, by adding default 0 value for submenu too.

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:73
>> +    return Ewk_Context_Menu_Item::create(type, action, title, checked, enabled, PassRefPtr<EwkContextMenu>(subMenuImpl)).leakPtr();
> 
> should be simply subMenuImpl

ok

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:160
>>      return const_cast<Ewk_Context_Menu_Item*>(item)->parentMenu();
> 
> const_cast is not actually needed here. Do you mind fixing it at the same time?

to be removed

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:167
>> +    return const_cast<Ewk_Context_Menu_Item*>(item)->subMenu();
> 
> maybe Ewk_Context_Menu_Item should also be an EwkObject? and we probably should get rid of "ewk_defines.h" file

Will be it okay to do it in the next patch?

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:72
>> +    EwkContextMenu* subMenu() { return m_subMenu.get(); }
> 
> I would make this getter const. Mikhail won't like it but it is consistent with parentMenu() above and it avoids the const_cast() in the caller.

OK, I will change it to const and remove const_cast from both *menu_get functions.

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item_private.h:86
>>      EwkContextMenu* m_parentMenu;
> 
> shouldn't it be RefPtr also?

I will change it to RefPtr too.

[Michal Pakula vel Rutka]

Created attachment 202061 [details]
fixes after review

[Chris Dumez]

Comment on attachment 202061 [details]
fixes after review

Looks fine. r=me but please let Mikhail take a final look before landing.

[Michal Pakula vel Rutka]

Comment on attachment 202061 [details]
fixes after review

Mikhail could you take a look and set cq+ if the patch is good?

[WebKit Commit Bot]

Comment on attachment 202061 [details]
fixes after review

Clearing flags on attachment: 202061

Committed r150254: <http://trac.webkit.org/changeset/150254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.