Bug 56290 – [EFL] Add ContextMenuClientEfl to efl port

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Gyuyoung Kim]

ContextMenuClientEfl.cpp / h were removed because of build errors. I add them again. But, there are some build breaks because of "CROSS_PLATFORM_CONTEXT_MENUS". I think CROSS_PLATFORM_CONTEXT_MENUS and CONTEXT_MENU are conflict. So, I make an option to solve the conflict.

[Gyuyoung Kim]

Created attachment 85650 [details]
Proposed Patch

[Gyuyoung Kim]

Created attachment 85653 [details]
Patch

[Lucas De Marchi]

Comment on attachment 85653 [details]
Patch

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

We moved from the previous implementation to the CROSS_PLATFORM one because our build was broken and because with the previous implementation there was a dependency in WebCore from symbols in WebKit.

In my opinion the right thing to do is to fix the current implementation, not going back to the previous one.

> Source/cmake/OptionsEfl.cmake:170
> +IF (ENABLE_CONTEXT_MENUS)
> +    SET(WTF_USE_CROSS_PLATFORM_CONTEXT_MENUS 0)
> +    ADD_DEFINITIONS(-DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=0)
> +ELSE()
> +    SET(WTF_USE_CROSS_PLATFORM_CONTEXT_MENUS 1)
> +    ADD_DEFINITIONS(-DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1)
> +ENDIF()
> +

If you don't have ContextMenu enabled, you won't want CROSS_PLATFORM_CONTEXT_MENU neither. It's just a different way to implement the same thing, that should be transparent to user.

[Gyuyoung Kim]

Created attachment 85771 [details]
EFL EWS Build Test Patch

This patch just tests if efl ews is working well. So, this patch has an obvious build error in order to test efl ews.

[Gyuyoung Kim]

Created attachment 85772 [details]
EFL EWS Build Test Patch

Oops, again.

[Gyuyoung Kim]

Created attachment 85773 [details]
EFL EWS Build Test Patch - 2

EFL EWS has a problem in previous patch.

[Gyuyoung Kim]

Created attachment 85774 [details]
EFL EWS Build Test Patch - 3

Sorry for frequent test.

[Gyuyoung Kim]

Created attachment 85783 [details]
Patch

Hello Lucas,

Is USE_CROSS_PLATFORM_CONTEXT_MENUS feature working ? It doesn't looks like to be implemented completely. GTK port also doesn't enable the macro as well.
I think we need to have ContextMenuClientEfl. Because, we need to use functions of ContextMenuClient.h for context menu.

#if USE(CROSS_PLATFORM_CONTEXT_MENUS)
        virtual PassOwnPtr<ContextMenu> customizeMenu(PassOwnPtr<ContextMenu>) = 0;
#else
        virtual PlatformMenuDescription getCustomMenuFromDefaultItems(ContextMenu*) = 0;
#endif

        virtual void contextMenuItemSelected(ContextMenuItem*, const ContextMenu*) = 0;

        virtual void downloadURL(const KURL& url) = 0;
        virtual void searchWithGoogle(const Frame*) = 0;
        virtual void lookUpInDictionary(Frame*) = 0;
        virtual bool isSpeaking() = 0;
        virtual void speak(const String&) = 0;
        virtual void stopSpeaking() = 0;

If we start to use the CROSS_PLATFORM_CONTEXT_MENUS feature, we can't use above functions. As you said, if the CROSS_PLATFORM_CONTEXT_MENU replaces context menu client implementation, why don't we enable it after finishing it's implementation ? I revise my patch.


BTW, I fix a build break regarding SHARED_CORE. Now you can build EWebKit with cmake ../Source -DPORT=Efl -DSHARED_CORE=ON

[Lucas De Marchi]

(In reply to comment #8)
> Created an attachment (id=85783) [details]
> Patch
> 
> Hello Lucas,
> 
> Is USE_CROSS_PLATFORM_CONTEXT_MENUS feature working ? It doesn't looks like

No, it's not.

> to be implemented completely. GTK port also doesn't enable the macro as well.

Because they didn't migrate (yet) to this cross platform solution.

> I think we need to have ContextMenuClientEfl. Because, we need to use functions of ContextMenuClient.h for context menu.
> 
> #if USE(CROSS_PLATFORM_CONTEXT_MENUS)
>         virtual PassOwnPtr<ContextMenu> customizeMenu(PassOwnPtr<ContextMenu>) = 0;
> #else
>         virtual PlatformMenuDescription getCustomMenuFromDefaultItems(ContextMenu*) = 0;
> #endif
> 
>         virtual void contextMenuItemSelected(ContextMenuItem*, const ContextMenu*) = 0;
> 
>         virtual void downloadURL(const KURL& url) = 0;
>         virtual void searchWithGoogle(const Frame*) = 0;
>         virtual void lookUpInDictionary(Frame*) = 0;
>         virtual bool isSpeaking() = 0;
>         virtual void speak(const String&) = 0;
>         virtual void stopSpeaking() = 0;
> 
> If we start to use the CROSS_PLATFORM_CONTEXT_MENUS feature, we can't use above functions. As you said, if the CROSS_PLATFORM_CONTEXT_MENU replaces context menu client implementation, why don't we enable it after finishing it's implementation ? I revise my patch.

Because in the end of last year (when the CROSS_PLATFORM_CONTEXT_MENUS were introduced) we was in a point in which none of the implementations were working and our build was broken. Then Leandro removed the previous implementation and added stub for the CROSS_PLATFORM_CONTEXT_MENUS one. Sadly, AFAIR the needed bits for having it completely implemented was not accepted because it needs some changes in WebCore that is not common to other platforms (i.e. the fact that in EFL we don't use a native context_menu widget and we just forward events to the application).

I'm CC'ing Brian Weinstein on this.


Brian, the following statement you wrote on logs does not apply for EFL port:

 [ For other ports to use CROSS_PLATFORM_CONTEXT_MENUS, all they need to do is a ContextMenu <-> native menu type, and ContextMenuItem <-> native menu item in ContextMenuWin.cpp and ContextMenuItemWin.cpp. ]

We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?


> 
> 
> BTW, I fix a build break regarding SHARED_CORE. Now you can build EWebKit with cmake ../Source -DPORT=Efl -DSHARED_CORE=ON

It's working because the context menu implementation was removed.

[Lucas De Marchi]

Gyuyoung Kim, you might want to check bug 50762 for a previous attempt to fix this, in which Brian already commented, but didn't solve the problem that we don't have (and we don't want) native context menus.

[Gyuyoung Kim]

>> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?


Hello Brian Weinstein,

I'd like to listen your comment about this issue.

[Brian Weinstein]

(In reply to comment #11)
> >> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?
> 
> 
> Hello Brian Weinstein,
> 
> I'd like to listen your comment about this issue.

If you don't use native widgets in your context menus, then the CROSS_PLATFORM_CONTEXT_MENUs define probably doesn't make sense, because that is based around the idea that you will want to convert everything to native widget types.

It looks like this patch turns off CROSS_PLATFORM_CONTEXT_MENU for EFL, and re-adds the stubs needed for it to build with that define turned off, if I am reading it correctly?

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > >> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?
> > 
> > 
> > Hello Brian Weinstein,
> > 
> > I'd like to listen your comment about this issue.
> 
> If you don't use native widgets in your context menus, then the CROSS_PLATFORM_CONTEXT_MENUs define probably doesn't make sense, because that is based around the idea that you will want to convert everything to native widget types.
> 
> It looks like this patch turns off CROSS_PLATFORM_CONTEXT_MENU for EFL, and re-adds the stubs needed for it to build with that define turned off, if I am reading it correctly?

Yes, right. I think EFL port doesn't prepare CROSS_PLATFORM_CONTEXT_MENU completely yet. So, I turn CROSS_PLATFORM_CONTEXT_MENU off, turn stubs needed on. I'd like to enable CROSS_PLATFORM_CONTEXT_MENU when it is prepared completely.

[Brian Weinstein]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > >> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?
> > > 
> > > 
> > > Hello Brian Weinstein,
> > > 
> > > I'd like to listen your comment about this issue.
> > 
> > If you don't use native widgets in your context menus, then the CROSS_PLATFORM_CONTEXT_MENUs define probably doesn't make sense, because that is based around the idea that you will want to convert everything to native widget types.
> > 
> > It looks like this patch turns off CROSS_PLATFORM_CONTEXT_MENU for EFL, and re-adds the stubs needed for it to build with that define turned off, if I am reading it correctly?
> 
> Yes, right. I think EFL port doesn't prepare CROSS_PLATFORM_CONTEXT_MENU completely yet. So, I turn CROSS_PLATFORM_CONTEXT_MENU off, turn stubs needed on. I'd like to enable CROSS_PLATFORM_CONTEXT_MENU when it is prepared completely.

Great, I will take a look at this patch later tonight, or tomorrow morning, if that's okay.

[Gyuyoung Kim]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > >> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?
> > > > 
> > > > 
> > > > Hello Brian Weinstein,
> > > > 
> > > > I'd like to listen your comment about this issue.
> > > 
> > > If you don't use native widgets in your context menus, then the CROSS_PLATFORM_CONTEXT_MENUs define probably doesn't make sense, because that is based around the idea that you will want to convert everything to native widget types.
> > > 
> > > It looks like this patch turns off CROSS_PLATFORM_CONTEXT_MENU for EFL, and re-adds the stubs needed for it to build with that define turned off, if I am reading it correctly?
> > 
> > Yes, right. I think EFL port doesn't prepare CROSS_PLATFORM_CONTEXT_MENU completely yet. So, I turn CROSS_PLATFORM_CONTEXT_MENU off, turn stubs needed on. I'd like to enable CROSS_PLATFORM_CONTEXT_MENU when it is prepared completely.
> 
> Great, I will take a look at this patch later tonight, or tomorrow morning, if that's okay.

Ok, I wait for your review. If my patch has any nits, please let me know.

[Lucas De Marchi]

> Yes, right. I think EFL port doesn't prepare CROSS_PLATFORM_CONTEXT_MENU completely yet. So, I turn CROSS_PLATFORM_CONTEXT_MENU off, turn stubs needed on. I'd like to enable CROSS_PLATFORM_CONTEXT_MENU when it is prepared completely.

(In reply to comment #12)
> (In reply to comment #11)
> > >> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?
> > 
> > 
> > Hello Brian Weinstein,
> > 
> > I'd like to listen your comment about this issue.
> 
> If you don't use native widgets in your context menus, then the CROSS_PLATFORM_CONTEXT_MENUs define probably doesn't make sense, because that is based around the idea that you will want to convert everything to native widget types.


Brian, 
I really like the idea of letting all the items in  a std::vector like CROSS_PLATFORM_CONTEXT_MENU does. The only thing that would be needed is a way to forward this to client, like the previous implementation did.

This is what was proposed in bug 50762

[Lucas De Marchi]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > >> We need to forward the requests back to client because we don't use native widgets. How should we handle this issue?
> > > 
> > > 
> > > Hello Brian Weinstein,
> > > 
> > > I'd like to listen your comment about this issue.
> > 
> > If you don't use native widgets in your context menus, then the CROSS_PLATFORM_CONTEXT_MENUs define probably doesn't make sense, because that is based around the idea that you will want to convert everything to native widget types.
> > 
> > It looks like this patch turns off CROSS_PLATFORM_CONTEXT_MENU for EFL, and re-adds the stubs needed for it to build with that define turned off, if I am reading it correctly?
> 
> Yes, right. I think EFL port doesn't prepare CROSS_PLATFORM_CONTEXT_MENU completely yet. So, I turn CROSS_PLATFORM_CONTEXT_MENU off, turn stubs needed on. I'd like to enable CROSS_PLATFORM_CONTEXT_MENU when it is prepared completely.

This is not going to work. The stubs you're adding were already implemented (and you are not putting them back). When you finally implement them, you'll hit the same limitation we had previously (layer violation WebCore / WebKit).

[Lucas De Marchi]

Comment on attachment 85783 [details]
Patch

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

> Source/WebCore/platform/efl/ContextMenuEfl.cpp:86
> +ContextMenu::~ContextMenu()
> +{
> +    notImplemented();
> +}
> +
> +void ContextMenu::insertItem(unsigned position, ContextMenuItem&)
> +{
> +    notImplemented();
> +}
> +
> +void ContextMenu::appendItem(ContextMenuItem&)
> +{
> +    notImplemented();
> +}
> +
> +PlatformMenuDescription ContextMenu::platformDescription() const
> +{
> +    notImplemented();
> +}
> +
> +void ContextMenu::setPlatformDescription(PlatformMenuDescription)
> +{
> +    notImplemented();
> +}
> +
> +PlatformMenuDescription ContextMenu::releasePlatformDescription()
> +{
> +    notImplemented();
> +    return 0;
> +}
> +#endif

E.g.: this was implemented in r7200 (http://trac.webkit.org/browser/trunk/WebCore/platform/efl/ContextMenuEfl.cpp?rev=72000)

Why are you adding this as stub notImplemented()?


Either we go to  CROSS_PLATFORM_CONTEXT_MENU or we add back the complete implementation and figure out the fixes.

[Gyuyoung Kim]

(In reply to comment #18)
> 
> E.g.: this was implemented in r7200 (http://trac.webkit.org/browser/trunk/WebCore/platform/efl/ContextMenuEfl.cpp?rev=72000)
> 
> Why are you adding this as stub notImplemented()?
> 
> 
> Either we go to  CROSS_PLATFORM_CONTEXT_MENU or we add back the complete implementation and figure out the fixes.

Yes, I wanted to back the complete implementation. But, there are some build breaks because ContextMenu.h, ContextMenuItem.h were a little changed. So, I think it is better this implementation is added by new bug. Because, I wasn't sure even if this patch can be landed. Do you think I need to add the complete implementation in this bug ?

[Leandro Pereira]

(In reply to comment #19)
> 
> Yes, I wanted to back the complete implementation. But, there are some build breaks because 
>ContextMenu.h, ContextMenuItem.h were a little changed. So, I think it is better this implementation is 
>added by new bug. Because, I wasn't sure even if this patch can be landed. Do you think I need to add 
>the complete implementation in this bug ?
>

The old implementation won't work because of internal API changes. The old API favoured a layer violation (WebCore side using things from WebKit side) and was refactored so that this isn't possible anymore.

The new API, for cross-platform menus, will create the menu items from inside WebCore using the native widget set for that platform -- and, on EFL this isn't possible because we don't have a widget set (or, better yet, we don't want to force the use of one widget set) *and* WebCore can't access WebKit anymore to forward events so that the browser can create these menus in a way that's more appropriate.

Currently, the context menu implementation on the EFL port is just a stub, as I was running out of time and couldn't think of a better way to write a implementation that uses the new cross-platform way, and the tree was red for too much time. So I just removed everything and went to the cross-platform way as it is cleaner than the old-school one.

I'm not sure, but I bet most platforms offers native widget sets and in a not so distant future they'll end up using this new way -- so bringing the old implementation back won't help (and won't work either, for reasons stated above).

So, to sum it up: If you need to fix the build by adding new required stubs, that's fine. Otherwise, unless you're submitting a fully working implementation of the context menu, I wouldn't bother patching the current code, as you'll be switching from one stub to another.

[Gyuyoung Kim]

(In reply to comment #20)
> (In reply to comment #19)
> > 
> > Yes, I wanted to back the complete implementation. But, there are some build breaks because 
> >ContextMenu.h, ContextMenuItem.h were a little changed. So, I think it is better this implementation is 
> >added by new bug. Because, I wasn't sure even if this patch can be landed. Do you think I need to add 
> >the complete implementation in this bug ?
> >
> 
> The old implementation won't work because of internal API changes. The old API favoured a layer violation (WebCore side using things from WebKit side) and was refactored so that this isn't possible anymore.
> 
> The new API, for cross-platform menus, will create the menu items from inside WebCore using the native widget set for that platform -- and, on EFL this isn't possible because we don't have a widget set (or, better yet, we don't want to force the use of one widget set) *and* WebCore can't access WebKit anymore to forward events so that the browser can create these menus in a way that's more appropriate.
> 
> Currently, the context menu implementation on the EFL port is just a stub, as I was running out of time and couldn't think of a better way to write a implementation that uses the new cross-platform way, and the tree was red for too much time. So I just removed everything and went to the cross-platform way as it is cleaner than the old-school one.
> 
> I'm not sure, but I bet most platforms offers native widget sets and in a not so distant future they'll end up using this new way -- so bringing the old implementation back won't help (and won't work either, for reasons stated above).

Yes, I think so. old implementation will not be help after finishing new implementation. But, we don't know when it is finished in efl port. I need to work context menu now. I'd like remove old implementation when cross-platform menu is completed.

> So, to sum it up: If you need to fix the build by adding new required stubs, that's fine. Otherwise, unless you're submitting a fully working implementation of the context menu, I wouldn't bother patching the current code, as you'll be switching from one stub to another.

I think we need to verify if old implementation can work in this patch. Ok, I will make a fully working implementation of context menu. Then, let's review it again.

[Gyuyoung Kim]

Created attachment 89553 [details]
Proposed Patch

First of all, sorry for my late patch. 

I restore context menu files for efl.  Though I want to avoid to touch context menu control, I'm obliged to modify it.

Demarchi,

I also implement context menu based on CROSS_PLATFORM_CONTEXT_MENUS. But, it seems I need time to do it. So, I need old implementation now because, we're using the old implementation. If I implement context menu based on CROSS_PLATFORM_CONTEXT_MENU or other guys implement it, we can remove old implementation.

[WebKit Review Bot]

Attachment 89553 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/CMakeListsEfl..." exit_code: 1

Source/WebCore/platform/efl/ContextMenuEfl.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/WebCoreSupport/ContextMenuClientEfl.h:59:  The parameter name "menu" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 89553 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8402547

[WebKit Review Bot]

Attachment 89553 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8397633

[Gyuyoung Kim]

Created attachment 89692 [details]
Patch

I fix style error and errors on chrominum port.

[Lucas De Marchi]

(In reply to comment #22)

> Demarchi,
> 
> I also implement context menu based on CROSS_PLATFORM_CONTEXT_MENUS. But, it seems I need time to do it. So, I need old implementation now because, we're using the old implementation. If I implement context menu based on CROSS_PLATFORM_CONTEXT_MENU or other guys implement it, we can remove old implementation.


I talked to Leandro on IRC. We are against doing this. IMO this will just postpone ad eternum the proper fix and we try to follow the principle of not work-arounding our issues. This is also too intrusive in platform-independent files.

Gyuyoung: If you really need this support urgently, I'd suggest you to keep this patch private.

[Gyuyoung Kim]

(In reply to comment #27)
> (In reply to comment #22)
> 
> > Demarchi,
> > 
> > I also implement context menu based on CROSS_PLATFORM_CONTEXT_MENUS. But, it seems I need time to do it. So, I need old implementation now because, we're using the old implementation. If I implement context menu based on CROSS_PLATFORM_CONTEXT_MENU or other guys implement it, we can remove old implementation.
> 
> 
> I talked to Leandro on IRC. We are against doing this. IMO this will just postpone ad eternum the proper fix and we try to follow the principle of not work-arounding our issues. This is also too intrusive in platform-independent files.
> 
> Gyuyoung: If you really need this support urgently, I'd suggest you to keep this patch private.

Hmm, ok. frankly, I also didn't want to touch webcore for this patch. But, I need previous implementation. So, I tried to land this patch. Furthermore, it looks almost ports doesn't support CROSS_PLATFORM_CONTEXT_MENUS yet.

ok, anyway, I have this as private patch. And, if nobody implement the cross platform context menu, I will do it. I think context menu implementation need to be modified considerably for CROSS_PLATFORM_CONTEXT_MENUS.