Bug 50762 - [EFL] ContextMenu implementation is broken
Summary: [EFL] ContextMenu implementation is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 44609
  Show dependency treegraph
 
Reported: 2010-12-09 07:56 PST by Lucas De Marchi
Modified: 2011-02-09 05:56 PST (History)
5 users (show)

See Also:


Attachments
Possible solution (8.07 KB, patch)
2010-12-14 07:53 PST, Leandro Pereira
eric: review-
leandro: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas De Marchi 2010-12-09 07:56:14 PST
After patch in bug 73535 has landed, current implementation of context menus in EFL port is broken.

This also affects bug 44609 because the to-be-landed patch depends on symbols from WebCore::ContextMenu class, which has been fixed by the commit that broke current EFL implementation.

The error when compiling is:

/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp: In constructor ‘WebCore::ContextMenu::ContextMenu()’:
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp:34: error: ‘m_view’ was not declared in this scope
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp:34: error: ‘menu’ was not declared in this scope
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp:34: error: ‘ewk_context_menu_new’ was not declared in this scope
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp: In destructor ‘WebCore::ContextMenu::~ContextMenu()’:
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp:45: error: ‘ewk_context_menu_free’ was not declared in this scope
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp: In member function ‘void WebCore::ContextMenu::appendItem(WebCore::ContextMenuItem&)’:
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp:50: error: ‘ewk_context_menu_item_append’ was not declared in this scope
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp: In member function ‘void WebCore::ContextMenu::setPlatformDescription(void*)’:
/mnt/buildbot/efl-linux-slave-1/efl-linux-release/build/WebCore/platform/efl/ContextMenuEfl.cpp:58: error: ‘ewk_context_menu_show’ was not declared in this scope
make[2]: *** [WebCore/CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuEfl.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [WebCore/CMakeFiles/webcore_efl.dir/all] Error 2
make: *** [all] Error 2
Failed to build Efl port
Comment 1 Leandro Pereira 2010-12-14 07:53:01 PST
Created attachment 76537 [details]
Possible solution

The attached patch is a possible solution.

The reason it does not have a ChangeLog entry is that I'd like some opinion on it before going further. The EFL port menu implementation is a bit tricky since we delegate menu creation to the browser (mobile and desktop have different UIs and needs different widgets) -- thus we can't call EFL directly from our WebCore classes.

Although this patch is filled with #ifdefs, EFL tree is currently red and we're out of time this year to move to the new context menu API. Is this patch acceptable, even if temporarily while we work on updating the port to use the new API?
Comment 2 Brian Weinstein 2010-12-14 20:57:15 PST
Comment on attachment 76537 [details]
Possible solution

Is there a way you can do this without passing a ContextMenuController to the ContextMenu constructor? That is a layering violation in itself, ContextMenu can't know about ContextMenuController.
Comment 3 Lucas De Marchi 2010-12-15 02:55:31 PST
(In reply to comment #2)
> (From update of attachment 76537 [details])
> Is there a way you can do this without passing a ContextMenuController to the ContextMenu constructor? That is a layering violation in itself, ContextMenu can't know about ContextMenuController.

The two places in which it's needed inside our APIs are (WebKit/efl/ewk_contextmenu.c):

Eina_Bool ewk_context_menu_destroy(Ewk_Context_Menu* menu)
Eina_Bool ewk_context_menu_item_select(Ewk_Context_Menu* menu, Ewk_Context_Menu_Item* item)


With these functions we can respectively tell WebCore to destroy de ContextMenu and select an item. How would this be accomplished without access to the controller?

Should I go through page->contextMenuController()? I'm almost sure I can do this because there's a ref to webview in WebCore::ContextMenuClientEfl, but I need to check.
Comment 4 Lucas De Marchi 2010-12-15 03:43:49 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 76537 [details] [details])
> > Is there a way you can do this without passing a ContextMenuController to the ContextMenu constructor? That is a layering violation in itself, ContextMenu can't know about ContextMenuController.
> 
> The two places in which it's needed inside our APIs are (WebKit/efl/ewk_contextmenu.c):
> 
> Eina_Bool ewk_context_menu_destroy(Ewk_Context_Menu* menu)
> Eina_Bool ewk_context_menu_item_select(Ewk_Context_Menu* menu, Ewk_Context_Menu_Item* item)
> 
> 
> With these functions we can respectively tell WebCore to destroy de ContextMenu and select an item. How would this be accomplished without access to the controller?
> 
> Should I go through page->contextMenuController()? I'm almost sure I can do this because there's a ref to webview in WebCore::ContextMenuClientEfl, but I need to check.

Checking again, the problem with this is that we need access to ContextMenuClient from within ContextMenu. When we create the ContextMenu, we give the ContextMenuClient (through the controller) because we don't create the widgets. We need to forward this call to browser so it can actually create them.
Comment 5 Brian Weinstein 2010-12-15 10:50:40 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 76537 [details] [details] [details])
> > > Is there a way you can do this without passing a ContextMenuController to the ContextMenu constructor? That is a layering violation in itself, ContextMenu can't know about ContextMenuController.
> > 
> > The two places in which it's needed inside our APIs are (WebKit/efl/ewk_contextmenu.c):
> > 
> > Eina_Bool ewk_context_menu_destroy(Ewk_Context_Menu* menu)
> > Eina_Bool ewk_context_menu_item_select(Ewk_Context_Menu* menu, Ewk_Context_Menu_Item* item)
> > 
> > 
> > With these functions we can respectively tell WebCore to destroy de ContextMenu and select an item. How would this be accomplished without access to the controller?
> > 
> > Should I go through page->contextMenuController()? I'm almost sure I can do this because there's a ref to webview in WebCore::ContextMenuClientEfl, but I need to check.
> 
> Checking again, the problem with this is that we need access to ContextMenuClient from within ContextMenu. When we create the ContextMenu, we give the ContextMenuClient (through the controller) because we don't create the widgets. We need to forward this call to browser so it can actually create them.

Could you pass the ContextMenuClient instead of the ContextMenuController? I think that would be okay, but would need to see a patch to know for sure.

One option would be to hook into the new USE(CROSS_PLATFORM_CONTEXT_MENUS) macro, which gives you a representation of ContextMenus and ContextMenuItems not using native types, and all you need to do is write functions that convert from ContextMenu <-> Native EFL Menus, and ContextMenuItem <-> Native EFL Menu Items. If you want to see how that is done currently, look at ContextMenu{.h|.cpp}, ContextMenuItem{.h|.cpp}, ContextMenuItemWin.cpp and ContextMenuWin.cpp.

All you would need to do is to write ContextMenuItemEfl.cpp and ContextMenuEfl.cpp, which convert to/from the native types.
Comment 6 Leandro Pereira 2010-12-15 11:14:24 PST
(In reply to comment #5)
> 
> Could you pass the ContextMenuClient instead of the ContextMenuController? I 
> think that would be okay, but would need to see a patch to know for sure.
> 

This seems possible, yes. I'll give it a try.

> One option would be to hook into the new USE(CROSS_PLATFORM_CONTEXT_MENUS) 
> macro, which gives you a representation of ContextMenus and ContextMenuItems 
> not using native types, and all you need to do is write functions that convert 
> from ContextMenu <-> Native EFL Menus, and ContextMenuItem <-> Native EFL Menu 
> Items. If you want to see how that is done currently, look at
> ContextMenu{.h|.cpp}, ContextMenuItem{.h|.cpp}, ContextMenuItemWin.cpp and 
> ContextMenuWin.cpp.
> 

Already looked at it. The problem is: EFL does not have a native widget set. 

Our current implementation delegates the responsibility to create the menu UI to the browser, so it can use whatever UI toolkit it is already using, by choosing the appropriate widget for the task (pop-up menus on desktop, lists on mobile/touch screen devices, etc).

This delegation is currently implemented on EWK (short for EFL-WebKit), so we can't really expose those functions to WebCore as it would be way nastier than HitTestResult being there :)

I'll see if I can do that with ContextMenuClient, by creating a façade to our menu-delegating functions in EWK.

Does this sound OK?

If so, to get rid of all those #ifdefs inside platform-independent code I could add a ContextMenuClient* to ContextMenu; most ports don't use this, however, so I'm on a bit of impasse here.
Comment 7 Eric Seidel (no email) 2010-12-15 15:21:42 PST
Comment on attachment 76537 [details]
Possible solution

Needs a ChangeLog.
Comment 8 Gyuyoung Kim 2010-12-16 16:26:31 PST
(In reply to comment #7)
> (From update of attachment 76537 [details])
> Needs a ChangeLog.

Leandro, 

Could you please add ChangeLog to this patch ? As you know, WebKit EFL have failed to build since last week. I think this patch should be landed to mainline as soon as possible.
Comment 9 Gyuyoung Kim 2010-12-16 16:43:35 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 76537 [details] [details])
> > Needs a ChangeLog.
> 
> Leandro, 
> 
> Could you please add ChangeLog to this patch ? As you know, WebKit EFL have failed to build since last week. I think this patch should be landed to mainline as soon as possible.

I just check you fix this build break for a while. Thank you.
Comment 10 Lucas De Marchi 2011-02-07 17:44:57 PST
After some code changes last year, it's broken again :(. I think the EFL builders are green because they are not cleaning the tree between builds.

Gyuyoung Kim: are you compiling the latest svn revision?
Comment 11 Gyuyoung Kim 2011-02-07 17:59:26 PST
(In reply to comment #10)
> After some code changes last year, it's broken again :(. I think the EFL builders are green because they are not cleaning the tree between builds.
> 
> Gyuyoung Kim: are you compiling the latest svn revision?

Yes, in my linux box, there is no build breaks in latest svn revision. I remember leandro fixed this build break without review. Do you have build break due to the context menu ?
Comment 12 Lucas De Marchi 2011-02-08 04:50:08 PST
(In reply to comment #11)
> (In reply to comment #10)
> > After some code changes last year, it's broken again :(. I think the EFL builders are green because they are not cleaning the tree between builds.
> > 
> > Gyuyoung Kim: are you compiling the latest svn revision?
> 
> Yes, in my linux box, there is no build breaks in latest svn revision. I remember leandro fixed this build break without review. Do you have build break due to the context menu ?

Yes. See below:

Linking CXX shared library libwebcore_efl.so
CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuEfl.cpp.o: In function `WebCore::ContextMenu::ContextMenu()':
ContextMenuEfl.cpp:(.text+0x0): multiple definition of `WebCore::ContextMenu::ContextMenu()'
CMakeFiles/webcore_efl.dir/platform/ContextMenu.cpp.o:ContextMenu.cpp:(.text+0x560): first defined here
CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuEfl.cpp.o: In function `WebCore::ContextMenu::ContextMenu()':
ContextMenuEfl.cpp:(.text+0x0): multiple definition of `WebCore::ContextMenu::ContextMenu()'
CMakeFiles/webcore_efl.dir/platform/ContextMenu.cpp.o:ContextMenu.cpp:(.text+0x560): first defined here
CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuItemEfl.cpp.o: In function `WebCore::ContextMenuItem::ContextMenuItem(WebCore::ContextMenuItemType, WebCore::ContextMenuAction, WTF::String const&, WebCore::ContextMenu*)':
...

I've just noticed this will only occur when configuring with SHARED_CORE=ON. This is how I'm configure webkit:

cmake ../Source -DPORT=Efl \
	-DCMAKE_INSTALL_PREFIX=$HOME/dev/e/ \
	-DENABLE_JIT=ON \
	-DSHARED_CORE=ON \
	-DNETWORK_BACKEND=curl \
	-DENABLE_GLIB_SUPPORT=ON \
	-DENABLE_VIDEO=ON \
	-DENABLE_PROGRESS_TAG=ON

Can you reproduce?
Comment 13 Gyuyoung Kim 2011-02-08 16:47:08 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > After some code changes last year, it's broken again :(. I think the EFL builders are green because they are not cleaning the tree between builds.
> > > 
> > > Gyuyoung Kim: are you compiling the latest svn revision?
> > 
> > Yes, in my linux box, there is no build breaks in latest svn revision. I remember leandro fixed this build break without review. Do you have build break due to the context menu ?
> 
> Yes. See below:
> 
> Linking CXX shared library libwebcore_efl.so
> CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuEfl.cpp.o: In function `WebCore::ContextMenu::ContextMenu()':
> ContextMenuEfl.cpp:(.text+0x0): multiple definition of `WebCore::ContextMenu::ContextMenu()'
> CMakeFiles/webcore_efl.dir/platform/ContextMenu.cpp.o:ContextMenu.cpp:(.text+0x560): first defined here
> CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuEfl.cpp.o: In function `WebCore::ContextMenu::ContextMenu()':
> ContextMenuEfl.cpp:(.text+0x0): multiple definition of `WebCore::ContextMenu::ContextMenu()'
> CMakeFiles/webcore_efl.dir/platform/ContextMenu.cpp.o:ContextMenu.cpp:(.text+0x560): first defined here
> CMakeFiles/webcore_efl.dir/platform/efl/ContextMenuItemEfl.cpp.o: In function `WebCore::ContextMenuItem::ContextMenuItem(WebCore::ContextMenuItemType, WebCore::ContextMenuAction, WTF::String const&, WebCore::ContextMenu*)':
> ...
> 
> I've just noticed this will only occur when configuring with SHARED_CORE=ON. This is how I'm configure webkit:
> 
> cmake ../Source -DPORT=Efl \
>     -DCMAKE_INSTALL_PREFIX=$HOME/dev/e/ \
>     -DENABLE_JIT=ON \
>     -DSHARED_CORE=ON \
>     -DNETWORK_BACKEND=curl \
>     -DENABLE_GLIB_SUPPORT=ON \
>     -DENABLE_VIDEO=ON \
>     -DENABLE_PROGRESS_TAG=ON
> 
> Can you reproduce?

Yes, I can reproduce this build break. However, when I build with this simple command "cmake ../Source -DPORT=Efl", there are no build breaks. 
 It seems the build configuration makes problems.
Comment 14 Lucas De Marchi 2011-02-08 17:29:13 PST
(In reply to comment #13)
> Yes, I can reproduce this build break. However, when I build with this simple command "cmake ../Source -DPORT=Efl", there are no build breaks. 
>  It seems the build configuration makes problems.

The problem is building it with shared core enabled, since the symbols regarding the context menu will be present both in webkit *and* webcore.
Comment 15 Gyuyoung Kim 2011-02-08 17:33:42 PST
(In reply to comment #14)
> (In reply to comment #13)
> > Yes, I can reproduce this build break. However, when I build with this simple command "cmake ../Source -DPORT=Efl", there are no build breaks. 
> >  It seems the build configuration makes problems.
> 
> The problem is building it with shared core enabled, since the symbols regarding the context menu will be present both in webkit *and* webcore.

Will you make a patch for this problem ?
Comment 16 Lucas De Marchi 2011-02-08 17:39:12 PST
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Yes, I can reproduce this build break. However, when I build with this simple command "cmake ../Source -DPORT=Efl", there are no build breaks. 
> > >  It seems the build configuration makes problems.
> > 
> > The problem is building it with shared core enabled, since the symbols regarding the context menu will be present both in webkit *and* webcore.
> 
> Will you make a patch for this problem ?

Can you take a look on this? I'm running out of time now.
Comment 17 Gyuyoung Kim 2011-02-08 17:42:06 PST
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > Yes, I can reproduce this build break. However, when I build with this simple command "cmake ../Source -DPORT=Efl", there are no build breaks. 
> > > >  It seems the build configuration makes problems.
> > > 
> > > The problem is building it with shared core enabled, since the symbols regarding the context menu will be present both in webkit *and* webcore.
> > 
> > Will you make a patch for this problem ?
> 
> Can you take a look on this? I'm running out of time now.

Now, I can't look on this. But, if this is not fixed till I have free time, I will fix it. :-)
Comment 18 Lucas De Marchi 2011-02-09 05:56:35 PST
Committed r78047: <http://trac.webkit.org/changeset/78047>