Bug 109698

Created attachment 188557 [details] patch

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

Created attachment 188565 [details] fixed changelog this patch won't build until https://bugs.webkit.org/show_bug.cgi?id=109815 will land

Created attachment 188859 [details] rebased patch rebased patch after WebView class introduction

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

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

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

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

Created attachment 189496 [details] fixes after review

Created attachment 189716 [details] rebased

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.

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.

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.

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

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?

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.

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.

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

Created attachment 191723 [details] patch patch splitted into WK2 C API and EFL part

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

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

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 ?

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

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

Created attachment 194852 [details] rebased after 111552 changes

Created attachment 198046 [details] rebased

Comment on attachment 198046 [details] rebased LGTM based on previous informal reviews.

Comment on attachment 198046 [details] rebased Clearing flags on attachment: 198046 Committed r148434: <http://trac.webkit.org/changeset/148434>

All reviewed patches have been landed. Closing bug.