Bug 74179 – [EFL] Context menu restore for WebKit-EFL

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

[Michal Pakula vel Rutka]

Adds context menu support for EFL port using CROSS_PLATFORM_CONTEXT_MENUS approach. As there are no native widgets in EFL I used Eina_List for NativeMenu (wrapped in a structure) and Ewk_Context_Menu_Item as NativeItem.

[Michal Pakula vel Rutka]

Created attachment 118569 [details]
proposed patch

[Michal Pakula vel Rutka]

Created attachment 118578 [details]
ChangeLog fixes

[Michal Pakula vel Rutka]

Created attachment 118579 [details]
ChangeLog fixes

[Michal Pakula vel Rutka]

Created attachment 118580 [details]
new files added to previous patch

[Gyuyoung Kim]

Comment on attachment 118580 [details]
new files added to previous patch

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

Personally, I like this patch. EFL port needs context menu so far. :-)

> Source/WebKit/efl/ewk/ewk_private.h:170
> +void ewk_context_menu_item_append(Ewk_Context_Menu_Core* o, const WebCore::ContextMenuItem& core);

We don't use efl variable name style in internal functions. Change o with menu or coreMenu.

> Source/WebKit/efl/ewk/ewk_private.h:173
> +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* o, Vector<Ewk_Context_Menu_Item*>& itemsVector);

ditto.

[Michal Pakula vel Rutka]

Created attachment 120004 [details]
variable style fixes

Fixed internal functions variables names.

[Gyuyoung Kim]

Comment on attachment 120004 [details]
variable style fixes

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

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:318
> +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* coreMenu, Vector<Ewk_Context_Menu_Item*>& itemsVector)

I'm not sure if *vector* name is good. Is it better to use ewk_context_menu_cores_get?

[Michal Pakula vel Rutka]

Comment on attachment 120004 [details]
variable style fixes

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

>> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:318
>> +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* coreMenu, Vector<Ewk_Context_Menu_Item*>& itemsVector)
> 
> I'm not sure if *vector* name is good. Is it better to use ewk_context_menu_cores_get?

how about list ewk_context_menu_core_items_get?

[Gyuyoung Kim]

(In reply to comment #8)
> (From update of attachment 120004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120004&action=review
> 
> >> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:318
> >> +void ewk_context_menu_core_vector_get(Ewk_Context_Menu_Core* coreMenu, Vector<Ewk_Context_Menu_Item*>& itemsVector)
> > 
> > I'm not sure if *vector* name is good. Is it better to use ewk_context_menu_cores_get?
> 
> how about list ewk_context_menu_core_items_get?

Looks better.

[Michal Pakula vel Rutka]

Created attachment 120444 [details]
change function name

[Gyuyoung Kim]

Comment on attachment 120444 [details]
change function name

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

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:323
> +    Eina_List* l;

Why don't we use more clear name instead of *l* ?

[Michal Pakula vel Rutka]

OK, I will change it to listIterator

[Michal Pakula vel Rutka]

Created attachment 120448 [details]
change list variable name

[Ryuan Choi]

Comment on attachment 120448 [details]
change list variable name

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

> Source/JavaScriptCore/wtf/Platform.h:1078
> +#if PLATFORM(WIN) || PLATFORM(EFL)

Almost good for me.

could you remove -DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1 from Source/WebCore/PlatformEfl.cmake ?

[Gyuyoung Kim]

(In reply to comment #14)
> (From update of attachment 120448 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120448&action=review
> 
> > Source/JavaScriptCore/wtf/Platform.h:1078
> > +#if PLATFORM(WIN) || PLATFORM(EFL)
> 
> Almost good for me.
> 
> could you remove -DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1 from Source/WebCore/PlatformEfl.cmake ?

EFL WK1 is enabling CROSS_PLATFORM_CONTEXT_MENU. But, EFL WK2 is not using CROSS_PLATFORM_MENU. As discussed in private channel, Michal will support CROSS_PLATFORM_CONTEXT_MENU on EFL WebKit2.

Why don't we disable CROSS_PLATFORM_CONTEXT_MENU when we build EFL WebKit2 until Michal fix it ?  

If CORSS_PLATFORM_CONTEXT_MENUS is turned off, I remember there were build errors during the WebKit EFL port building.

[Michal Pakula vel Rutka]

(In reply to comment #14)
> (From update of attachment 120448 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120448&action=review
> 
> > Source/JavaScriptCore/wtf/Platform.h:1078
> > +#if PLATFORM(WIN) || PLATFORM(EFL)
> 
> Almost good for me.
> 
> could you remove -DWTF_USE_CROSS_PLATFORM_CONTEXT_MENUS=1 from Source/WebCore/PlatformEfl.cmake ?

OK, done, Platform.h definition is enough.

(In reply to comment #15)
> EFL WK1 is enabling CROSS_PLATFORM_CONTEXT_MENU. But, EFL WK2 is not using CROSS_PLATFORM_MENU. As discussed in private channel, Michal will support CROSS_PLATFORM_CONTEXT_MENU on EFL WebKit2.
> 
> Why don't we disable CROSS_PLATFORM_CONTEXT_MENU when we build EFL WebKit2 until Michal fix it ?  
> 
> If CORSS_PLATFORM_CONTEXT_MENUS is turned off, I remember there were build errors during the WebKit EFL port building.

Actually from WebKit2 in EFL point there is no or very little difference in WebKit2 implementation between CROSS and non-CROSS_PLATFORM_CONTEXT_MENUS, so switching to CROSS_PLATFORM_CONTEXT_MENUS=on will require only a minor patch.

[Michal Pakula vel Rutka]

Created attachment 120677 [details]
removed definition from PlatformEfl.cmake

[Gyuyoung Kim]

Comment on attachment 120677 [details]
removed definition from PlatformEfl.cmake

Ok, LGTM.

[Ryuan Choi]

Comment on attachment 120677 [details]
removed definition from PlatformEfl.cmake

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

> Source/WebCore/platform/ContextMenuItem.h:52
> +#elif PLATFORM(EFL)
> +#include "ewk_contextmenu.h"

WebKit2 does not use ewk_contextmenu.h

How do you think finding better way to use in both webkit and webkit2?

[Michal Pakula vel Rutka]

yes, you are right, I need to rebase this patch and I will think of this problem bearing in mind WK2...

[Ryuan Choi]

Comment on attachment 120677 [details]
removed definition from PlatformEfl.cmake

Clear flags because of above comment.

[Michal Pakula vel Rutka]

In that case when EFL WebKit2 is using non-cross platform context menus I changed the patch so it will use "standard" context menus, with Ewk_Context_Menu in Ewk_View_Private_Data. EWebLauncher patch will be updated soon.

[Michal Pakula vel Rutka]

Created attachment 135564 [details]
new approach not using cross platform context menus

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135564 [details]
new approach not using cross platform context menus

Does it make sense to enable context menus even if the client implementation is empty?

[Michal Pakula vel Rutka]

(In reply to comment #24)
> (From update of attachment 135564 [details])
> Does it make sense to enable context menus even if the client implementation is empty?

Unfortunately ContextMenuClientEfl have to be registered to pageClients in Page.h if CONTEXT_MENUS flag is enabled. Also it is used by ContextMenuController - there are virtual methods called on ContextMenuClient m_client, and they have to be implemented.

[Michal Pakula vel Rutka]

Created attachment 138317 [details]
rebased patch

the patch needed to be rebased, plus some minor fixes added

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebKit/efl/WebCoreSupport/ContextMenuClientEfl.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/efl/WebCoreSupport/ContextMenuClientEfl.h:46:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 12 files


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

[Michal Pakula vel Rutka]

Created attachment 138319 [details]
rebased patch

style fixes

[Michal Pakula vel Rutka]

Created attachment 141178 [details]
rebased patch

rebased again to match new PlatformMenuDescription after https://bugs.webkit.org/show_bug.cgi?id=84136

[Michal Pakula vel Rutka]

Created attachment 142259 [details]
rebased after ewk_private changes

Rebase after splitting ewk_private into separate header files.

[Gyuyoung Kim]

Comment on attachment 142259 [details]
rebased after ewk_private changes

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

Almost make sense except for my comments.

> ChangeLog:11
> +        No new tests.

You have to mention why this change doesn't need to have test cases, Or, if this modification doesn't need to have test case, please remove this line.

> Source/WebCore/ChangeLog:11
> +        No new tests.

ditto.

> Source/WebKit/ChangeLog:11
> +        No new tests.

ditto.

> Source/WebKit/efl/ChangeLog:11
> +        No new tests.

ditto.

[Michal Pakula vel Rutka]

Comment on attachment 142259 [details]
rebased after ewk_private changes

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

>> ChangeLog:11
>> +        No new tests.
> 
> You have to mention why this change doesn't need to have test cases, Or, if this modification doesn't need to have test case, please remove this line.

It's not a new feature, just a new way of implementing, so no new test will be needed. I will remove this lines.

[Michal Pakula vel Rutka]

Created attachment 143508 [details]
change log fixes

new patch with fixed changelogs according to Gyuyoung comment

[Gyuyoung Kim]

Comment on attachment 143508 [details]
change log fixes

Looks fine now. By the way, are there any test cases covered by this patch?

[Michal Pakula vel Rutka]

There are some test cases which requires isContextClick to be implemented, which requires this one to land.

[Gyuyoung Kim]

Comment on attachment 143508 [details]
change log fixes

Please rebase this patch for landing.

[Michal Pakula vel Rutka]

Created attachment 161997 [details]
rebased patch

[Grzegorz Czajkowski]

Comment on attachment 161997 [details]
rebased patch

LGTM. Thanks

[WebKit Review Bot]

Comment on attachment 161997 [details]
rebased patch

Clearing flags on attachment: 161997

Committed r127462: <http://trac.webkit.org/changeset/127462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.