Bug 109698 – [EFL][WK2] Use C API in ewk_context_menu

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

[Michal Pakula vel Rutka]

Currently EFL port in ewk_context_menu uses internal C++ classes directly, it should start using WK2 C API.

[Michal Pakula vel Rutka]

Created attachment 188557 [details]
patch

[EFL EWS Bot]

Comment on attachment 188557 [details]
patch

Attachment 188557 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16598072

[Michal Pakula vel Rutka]

Created attachment 188565 [details]
fixed changelog

this patch won't  build until https://bugs.webkit.org/show_bug.cgi?id=109815 will land

[Michal Pakula vel Rutka]

Created attachment 188859 [details]
rebased patch

rebased patch after WebView class introduction

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[EFL EWS Bot]

Comment on attachment 188859 [details]
rebased patch

Attachment 188859 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16626201

[Mikhail Pozdnyakov]

Comment on attachment 188859 [details]
rebased patch

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

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:802
> +void WKPageContextMenuItemSelected(WKPageRef page, WKContextMenuItemRef item)

WKPageGetSelectedContextMenuItem ?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:44
> +    const size_t size = WKArrayGetSize(items);

const local variables are not up to the coding style.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:145
> +        wkItem = WKContextMenuItemCreateAsAction(static_cast<WKContextMenuItemTag>(item->action()), WKStringCreateWithUTF8CString(item->title()), item->enabled());

WKStringCreateWithUTF8CString needs adoption.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:148
> +        wkItem = WKContextMenuItemCreateAsCheckableAction(static_cast<WKContextMenuItemTag>(item->action()), WKStringCreateWithUTF8CString(item->title()), item->enabled(), item->checked());

same here.

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:151
> +        return false;

is it expected? shouldn't ASSERT_NOT_REACHED be here?

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:29
> +#include "WKContextMenuItemTypes.h"

#include <WebKit2/WKContextMenuItemTypes.h>

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:39
> +    , m_title(WKEinaSharedString(WKContextMenuItemCopyTitle(item)))

needs adoption

> Source/WebKit2/UIProcess/PageClient.h:183
> +#if !PLATFORM(EFL)

could you explain why ?

> Source/WebKit2/UIProcess/efl/ContextMenuClientEfl.cpp:44
> +    contextMenuClient->view()->showContextMenu(menuLocation, menuItems);

nit: could be one line: toContextClientEfl(clientInfo)->view..

> Source/WebKit2/UIProcess/efl/ContextMenuClientEfl.cpp:51
> +    contextMenuClient->view()->hideContextMenu();

ditto

[Michal Pakula vel Rutka]

Comment on attachment 188859 [details]
rebased patch

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

>> Source/WebKit2/UIProcess/API/C/WKPage.cpp:802
>> +void WKPageContextMenuItemSelected(WKPageRef page, WKContextMenuItemRef item)
> 
> WKPageGetSelectedContextMenuItem ?

This function is called to select context menu item, so word Get may be confusing.

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:44
>> +    const size_t size = WKArrayGetSize(items);
> 
> const local variables are not up to the coding style.

OK

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:145
>> +        wkItem = WKContextMenuItemCreateAsAction(static_cast<WKContextMenuItemTag>(item->action()), WKStringCreateWithUTF8CString(item->title()), item->enabled());
> 
> WKStringCreateWithUTF8CString needs adoption.

OK

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:148
>> +        wkItem = WKContextMenuItemCreateAsCheckableAction(static_cast<WKContextMenuItemTag>(item->action()), WKStringCreateWithUTF8CString(item->title()), item->enabled(), item->checked());
> 
> same here.

OK

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu.cpp:151
>> +        return false;
> 
> is it expected? shouldn't ASSERT_NOT_REACHED be here?

OK I will add it here before return

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:29
>> +#include "WKContextMenuItemTypes.h"
> 
> #include <WebKit2/WKContextMenuItemTypes.h>

OK

>> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.cpp:39
>> +    , m_title(WKEinaSharedString(WKContextMenuItemCopyTitle(item)))
> 
> needs adoption

OK

>> Source/WebKit2/UIProcess/PageClient.h:183
>> +#if !PLATFORM(EFL)
> 
> could you explain why ?

Context menu WK client is located inside WKPage.h, so we do not need separate ContextMenuProxy the same as i.e. for WKPageFindClient.

>> Source/WebKit2/UIProcess/efl/ContextMenuClientEfl.cpp:44
>> +    contextMenuClient->view()->showContextMenu(menuLocation, menuItems);
> 
> nit: could be one line: toContextClientEfl(clientInfo)->view..

OK

>> Source/WebKit2/UIProcess/efl/ContextMenuClientEfl.cpp:51
>> +    contextMenuClient->view()->hideContextMenu();
> 
> ditto

OK

[Michal Pakula vel Rutka]

Created attachment 189496 [details]
fixes after review

[Michal Pakula vel Rutka]

Created attachment 189716 [details]
rebased

[Jesus Sanchez-Palencia]

Comment on attachment 189716 [details]
rebased

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3071
> +#if PLATFORM(EFL)

Why adding more platform-specific code to this common file if this could go through WebContextMenuProxy (m_activeContextMenu, in this context).

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3087
> +#if PLATFORM(EFL)

Ditto.
I have checked your ContextMenuClientEfl and neither its showContextMenu or hideContextMenu end up using the WebPageProxy ptr, so again this could be going through m_activeContextMenu.

[Michal Pakula vel Rutka]

Comment on attachment 189716 [details]
rebased

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:3087
>> +#if PLATFORM(EFL)
> 
> Ditto.
> I have checked your ContextMenuClientEfl and neither its showContextMenu or hideContextMenu end up using the WebPageProxy ptr, so again this could be going through m_activeContextMenu.

If I want to use WK API I have to call the methods on WebContextMenuClient (m_contextMenuClient) which has a member m_client of WKPageContextMenuClient type. WebContextMenuProxy currently does not have access to WebContextMenuClient, and adding it will need to introduce a change in context menu implementation, as WebContextMenuProxy will have to be changed to APIObject. My intention was to make a small patch, and change other ports' implementation as little as it is possible.

[Michal Pakula vel Rutka]

Comment on attachment 189716 [details]
rebased

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

>>> Source/WebKit2/UIProcess/WebPageProxy.cpp:3087
>>> +#if PLATFORM(EFL)
>> 
>> Ditto.
>> I have checked your ContextMenuClientEfl and neither its showContextMenu or hideContextMenu end up using the WebPageProxy ptr, so again this could be going through m_activeContextMenu.
> 
> If I want to use WK API I have to call the methods on WebContextMenuClient (m_contextMenuClient) which has a member m_client of WKPageContextMenuClient type. WebContextMenuProxy currently does not have access to WebContextMenuClient, and adding it will need to introduce a change in context menu implementation, as WebContextMenuProxy will have to be changed to APIObject. My intention was to make a small patch, and change other ports' implementation as little as it is possible.

I checked and this can be done in easier way by adding WebPageContexMenuClient (m_contextMenuClient), as one of arguments in showContextMenu and hideContextMenu, but this solution has two disadvantages: this argument will be unused in all other ports, WebContextMenuProxy in EFL port will be created only to share WebPageProxy implementation with others. In this case I think it is better to add EFL-only code rather than adding unneeded code.

[Jesus Sanchez-Palencia]

(In reply to comment #13)
> I checked and this can be done in easier way by adding WebPageContexMenuClient (m_contextMenuClient), as one of arguments in showContextMenu and hideContextMenu, but this solution has two disadvantages: this argument will be unused in all other ports, WebContextMenuProxy in EFL port will be created only to share WebPageProxy implementation with others. In this case I think it is better to add EFL-only code rather than adding unneeded code.

Hi! I've applied your patch locally and I was having a closer look at it. It would be great if we could reach a common code for us all (EFL, Nix, Qt, and perhaps GTK).

If we all decide to go through the C client like this, then keeping m_activeContextMenu in WebPageProxy will only make sense for Mac. All I wished is that we could route all ContextMenu code path through a single entity instead of having both a WebContextMenuProxy and a WebPageContextMenuClient in WebPageProxy.

Anyway, I guess we should get some feedback from a reviewer now.

[Kenneth Rohde Christiansen]

Comment on attachment 189716 [details]
rebased

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

> Source/WebKit2/ChangeLog:13
> +        to avoid violating layering.
> +        This patch changes EFL context menu API to use only WK2 C API, also adds two functions to WKPageContextMenuClient.
> +

Maybe the patch adding those should be separate and come with a unit test?

[Jesus Sanchez-Palencia]

Comment on attachment 189716 [details]
rebased

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

> Source/WebKit2/UIProcess/PageClient.h:183
> +#if !PLATFORM(EFL)

There is no need for this.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3088
> +    m_contextMenuClient.showContextMenu(this, menuLocation, proposedItems);

I believe here you should call m_contextMenuClient.getContextMenuFromProposedMenu() as it is done on the lines below (inside the #else block). Otherwise ports going through this code path will lose the option of customizing the menu through their context menu client.

[Michal Pakula vel Rutka]

Comment on attachment 189716 [details]
rebased

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

>> Source/WebKit2/ChangeLog:13
>> +
> 
> Maybe the patch adding those should be separate and come with a unit test?

OK I can split it into two parts: common and EFL-only. Maybe I wrote it wrong, but this patch does not change any Webkit-EFL API functions, only the way it communicates with WebKit, so no changes in unit tests are needed.

>> Source/WebKit2/UIProcess/PageClient.h:183
>> +#if !PLATFORM(EFL)
> 
> There is no need for this.

If I will leave this declaration here, I will have to leave the createContextMenuProxy implementation also in WebView class.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:3088
>> +    m_contextMenuClient.showContextMenu(this, menuLocation, proposedItems);
> 
> I believe here you should call m_contextMenuClient.getContextMenuFromProposedMenu() as it is done on the lines below (inside the #else block). Otherwise ports going through this code path will lose the option of customizing the menu through their context menu client.

OK, I will add this.

[Michal Pakula vel Rutka]

Created attachment 190708 [details]
fixes

added possibility to customize context menu items, replace COMPILE_ASSERT_MATCHING_ENUM macro with functions converting enums between WK API and EWK

[Michal Pakula vel Rutka]

Created attachment 191723 [details]
patch

patch splitted into WK2 C API and EFL part

[Gyuyoung Kim]

(In reply to comment #18)
> Created an attachment (id=190708) [details]
> fixes
> 
> added possibility to customize context menu items, replace COMPILE_ASSERT_MATCHING_ENUM macro with functions converting enums between WK API and EWK

I'm wondering if the functions can guarantee that EWK_CONTEXT_MENU_ITEM_TAG_XXX is sync with WebCore::ContextMenuItemTagXXX. Can the functions check if WebCore::ContextMenuItemTagXXX is removed or changed ?

[Michal Pakula vel Rutka]

(In reply to comment #20)
> (In reply to comment #18)
> > Created an attachment (id=190708) [details] [details]
> > fixes
> > 
> > added possibility to customize context menu items, replace COMPILE_ASSERT_MATCHING_ENUM macro with functions converting enums between WK API and EWK
> 
> I'm wondering if the functions can guarantee that EWK_CONTEXT_MENU_ITEM_TAG_XXX is sync with WebCore::ContextMenuItemTagXXX. Can the functions check if WebCore::ContextMenuItemTagXXX is removed or changed ?

There functions I have added (WK2 C API <-> EWK) are similar to ones from here: https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L321, converting between WebCore and WK2 C API. 
If new context menu tags/actions will be added to WebCore, they won't be just visible for application using ewk_context_menu API.
If I will keep using COMPILE_ASSERT_MATCHING_ENUM, Ewk_Context_Menu_Item_Action will have to be in sync with WKContextMenuItemTag, which contains items not used by EFL port.

[Gyuyoung Kim]

https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L321

I think this case is reasonable because this map WebCore's context menu enum on WK2's enum. However, getWKTagFromEwkAction() and getEwkActionFromWKTag() just deal with WK2 enum defined by us. I still think COMPILE_ASSERT_MATCHING_ENUM will help that we realize this removal when one of WebCore::ContextMenuAction enums is removed.

>> If I will keep using COMPILE_ASSERT_MATCHING_ENUM, Ewk_Context_Menu_Item_Action will have to be in sync with WKContextMenuItemTag, which contains items not used by EFL port.

Basically, when I added Ewk_Context_Menu_Item, I considered it is in sync with WebCore's ContextMenuItem.h. Look at http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.h#L56

Isn't WKContextMenuItemTag in sync with ContextMenuItem.h as well ? If EFL port doesn't want not to support some items, I think we disable it in ContextMenuItem.h.

If you wanna remove COMPILE_ASSERT_MATCHING_ENUM, how about using ContextMenuItem.h directly as GTK port ?

[Michal Pakula vel Rutka]

(In reply to comment #22)
> https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L321
> 
> I think this case is reasonable because this map WebCore's context menu enum on WK2's enum. However, getWKTagFromEwkAction() and getEwkActionFromWKTag() just deal with WK2 enum defined by us. I still think COMPILE_ASSERT_MATCHING_ENUM will help that we realize this removal when one of WebCore::ContextMenuAction enums is removed.
> 

If some of WebCore's items will be removed also WKContextMenuItemTag should be removed or at least it has to be removed from from toAPI, toImpl functions. 
In first case Ewk_Context_Menu_Item_Tag should be also removed or it will cause build break, in second case using non-existent tag will generate error message.

> >> If I will keep using COMPILE_ASSERT_MATCHING_ENUM, Ewk_Context_Menu_Item_Action will have to be in sync with WKContextMenuItemTag, which contains items not used by EFL port.
> 
> Basically, when I added Ewk_Context_Menu_Item, I considered it is in sync with WebCore's ContextMenuItem.h. Look at http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.h#L56

I guess I forgot to remove this comment :)

> 
> Isn't WKContextMenuItemTag in sync with ContextMenuItem.h as well ? If EFL port doesn't want not to support some items, I think we disable it in ContextMenuItem.h.

I have checked and WKContextMenuItemTag is not in sync with ContextMenuAction from WebCore. Also some of ContextMenuAction enum are surrounded by ifdefs, while WK2 API are not.

> 
> If you wanna remove COMPILE_ASSERT_MATCHING_ENUM, how about using ContextMenuItem.h directly as GTK port ?

Main purpose of this patch was stop relaying on WebKit2 internal C++ classes and start using WK2 API directly, and I thought it also concerns using WK2 C API enum instead of WebCore.

[Gyuyoung Kim]

(In reply to comment #23)
 
> > If you wanna remove COMPILE_ASSERT_MATCHING_ENUM, how about using ContextMenuItem.h directly as GTK port ?
> 
> Main purpose of this patch was stop relaying on WebKit2 internal C++ classes and start using WK2 API directly, and I thought it also concerns using WK2 C API enum instead of WebCore.

Ok, I'm fine from the point of using WK2 API. And, as you said, it looks that toAPI, toImpl functions will guarantee my concern implicitly. Thank you for your explanation.

>> I guess I forgot to remove this comment :)

Yes, please modify this comment as well. :)

[Michal Pakula vel Rutka]

Created attachment 194852 [details]
rebased after 111552 changes

[Michal Pakula vel Rutka]

Created attachment 198046 [details]
rebased

[Gyuyoung Kim]

Comment on attachment 198046 [details]
rebased

LGTM based on previous informal reviews.

[WebKit Commit Bot]

Comment on attachment 198046 [details]
rebased

Clearing flags on attachment: 198046

Committed r148434: <http://trac.webkit.org/changeset/148434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.