Bug 50762 – [EFL] ContextMenu implementation is broken

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Lucas De Marchi]

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

[Leandro Pereira]

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?

[Brian Weinstein]

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.

[Lucas De Marchi]

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

[Lucas De Marchi]

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

[Brian Weinstein]

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

[Leandro Pereira]

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

[Eric Seidel (no email)]

Comment on attachment 76537 [details]
Possible solution

Needs a ChangeLog.

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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

[Lucas De Marchi]

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?

[Gyuyoung Kim]

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

[Lucas De Marchi]

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

[Gyuyoung Kim]

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

[Lucas De Marchi]

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

[Gyuyoung Kim]

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

[Lucas De Marchi]

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

[Gyuyoung Kim]

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

[Lucas De Marchi]

Committed r78047: <http://trac.webkit.org/changeset/78047>