Summary: | [EFL][WK2] Support sub menu in ewk context menus | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rakuco | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | 116097 | ||||||||||||||||
Bug Blocks: | 114728 | ||||||||||||||||
Attachments: |
|
Description
Michal Pakula vel Rutka
2013-04-17 02:27:46 PDT
Created attachment 198696 [details]
proposed patch
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. 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. Created attachment 199394 [details]
fixes
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? 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). Created attachment 199620 [details]
fixes
moved adoption of submenu pointer to ewk_context_menu_item.cpp
Comment on attachment 199620 [details]
fixes
Agreed on IRC that menu is better to be shareable (ewk object)
Created attachment 199812 [details]
changed EwkContextMenu to EwkObject
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? Created attachment 201817 [details]
patch
changing EwkContextMenu to EwkObject moved to patch 116097
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 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? 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. 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. Created attachment 202061 [details]
fixes after review
Comment on attachment 202061 [details]
fixes after review
Looks fine. r=me but please let Mikhail take a final look before landing.
Comment on attachment 202061 [details]
fixes after review
Mikhail could you take a look and set cq+ if the patch is good?
Comment on attachment 202061 [details] fixes after review Clearing flags on attachment: 202061 Committed r150254: <http://trac.webkit.org/changeset/150254> All reviewed patches have been landed. Closing bug. |