In order to support WTR/EFL, we should provide WebKit2 API Object to represent Evas_Object although we will use ewk API.
Created attachment 147194 [details] Patch
Comment on attachment 147194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147194&action=review > Source/WebKit2/Shared/API/c/WKBase.h:44 > +#if defined(WTF_PLATFORM_EFL) Why do you use defined(WTF_PLATFORM_EFL) instead of PLATFORM(EFL) ? PLATFORM(EFL) is more general. > Source/WebKit2/UIProcess/API/C/WKAPICast.h:367 > +#if defined(WTF_PLATFORM_EFL) ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:546 > +WebPageProxy* ewk_view_page_get(Evas_Object* ewkView) Use const keyword for _get() > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:38 > +WebKit::WebPageProxy* ewk_view_page_get(Evas_Object* ewkView); ditto.
Created attachment 147236 [details] Patch
(In reply to comment #2) > (From update of attachment 147194 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147194&action=review > > > Source/WebKit2/Shared/API/c/WKBase.h:44 > > +#if defined(WTF_PLATFORM_EFL) > > Why do you use defined(WTF_PLATFORM_EFL) instead of PLATFORM(EFL) ? PLATFORM(EFL) is more general. > > > Source/WebKit2/UIProcess/API/C/WKAPICast.h:367 > > +#if defined(WTF_PLATFORM_EFL) > > ditto. > It should be as-is because they don't know Platform.h > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:546 > > +WebPageProxy* ewk_view_page_get(Evas_Object* ewkView) > > Use const keyword for _get() > > > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:38 > > +WebKit::WebPageProxy* ewk_view_page_get(Evas_Object* ewkView); > > ditto. I fixed.
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 147194 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147194&action=review > > > > > Source/WebKit2/Shared/API/c/WKBase.h:44 > > > +#if defined(WTF_PLATFORM_EFL) > > > > Why do you use defined(WTF_PLATFORM_EFL) instead of PLATFORM(EFL) ? PLATFORM(EFL) is more general. > > > > > Source/WebKit2/UIProcess/API/C/WKAPICast.h:367 > > > +#if defined(WTF_PLATFORM_EFL) > > > > ditto. > > > It should be as-is because they don't know Platform.h I can't find WTF_PLATFORM_XXX in WebKit2 directory. PLATFORM(XXXX) has used so far in WebKit2. For example, - http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L443 If this patch is landed, I think this can be a reference to use WTF_PLATFORM_XXX. If WKBase.h can't use Platform.h, why don't you let it know?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > (From update of attachment 147194 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=147194&action=review > > > > > > > Source/WebKit2/Shared/API/c/WKBase.h:44 > > > > +#if defined(WTF_PLATFORM_EFL) > > > > > > Why do you use defined(WTF_PLATFORM_EFL) instead of PLATFORM(EFL) ? PLATFORM(EFL) is more general. > > > > > > > Source/WebKit2/UIProcess/API/C/WKAPICast.h:367 > > > > +#if defined(WTF_PLATFORM_EFL) > > > > > > ditto. > > > > > It should be as-is because they don't know Platform.h > > I can't find WTF_PLATFORM_XXX in WebKit2 directory. PLATFORM(XXXX) has used so far in WebKit2. > > For example, > - http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L443 > > If this patch is landed, I think this can be a reference to use WTF_PLATFORM_XXX. If WKBase.h can't use Platform.h, why don't you let it know? Because WKBase.h is API, I believe that it should not add it. Instead, other ports like Gtk, Win checks their own macro like WIN32, _WIN32, BUILDING_GTK__ But, We don't have proper macro without BUILDING_WITH_CMAKE in OptionsCommon.cmake and WTF_PLATFORM_EFL in OptionsEfl.cmak. If you concern that it makes confusion about using WTF_PLATFORM_XXX, how about adding BUILDING_EFL__ in OptionsEfl.cmake?
Created attachment 147264 [details] Patch
Comment on attachment 147264 [details] Patch LGTM.
Created attachment 147277 [details] Patch
Created attachment 147279 [details] Patch
Comment on attachment 147279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147279&action=review > Source/WebKit2/PlatformEfl.cmake:31 > + UIProcess/API/C/efl/WKView.cpp ordering is incorrect here. > Source/WebKit2/PlatformEfl.cmake:89 > + "${WEBKIT2_DIR}/UIProcess/API/C/efl/" Other lines don't use a trailing slash. > Source/WebKit2/Shared/API/c/efl/WKBaseEfl.h:27 > +#include <stdbool.h> Why this include? > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:27 > +using namespace WebCore; Is this really needed here?
Created attachment 148266 [details] Patch
(In reply to comment #11) > (From update of attachment 147279 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147279&action=review > > > Source/WebKit2/PlatformEfl.cmake:31 > > + UIProcess/API/C/efl/WKView.cpp > > ordering is incorrect here. Right, I fixed. > > > Source/WebKit2/PlatformEfl.cmake:89 > > + "${WEBKIT2_DIR}/UIProcess/API/C/efl/" > > Other lines don't use a trailing slash. Fixed. > > > Source/WebKit2/Shared/API/c/efl/WKBaseEfl.h:27 > > +#include <stdbool.h> > > Why this include? Mistake while cleaning. fixed. > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:27 > > +using namespace WebCore; > > Is this really needed here? Yes, It is for toAPI and toImpl. I fixed without this. Thank you.
Comment on attachment 148266 [details] Patch LGTM.
(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 147279 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147279&action=review > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:27 > > > +using namespace WebCore; > > > > Is this really needed here? > > Yes, It is for toAPI and toImpl. > I fixed without this. I still don't believe this is needed. toAPI and toImpl are in WebKit namespace, not WebCore, AFAIK.
Created attachment 148269 [details] Patch
(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #11) > > > (From update of attachment 147279 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=147279&action=review > > > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:27 > > > > +using namespace WebCore; > > > > > > Is this really needed here? > > > > Yes, It is for toAPI and toImpl. > > I fixed without this. > > I still don't believe this is needed. toAPI and toImpl are in WebKit namespace, not WebCore, AFAIK. Ah Sorry, I misread your comment. you are right. Removed WebCore namespace.
Comment on attachment 148269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148269&action=review > Source/WebKit2/UIProcess/API/C/WKAPICast.h:371 > +#if PLATFORM(EFL) Though I'm not sure whether WKAPICast.h is public, I think it is more safe to use #if defined(BUILDING_EFL__) like GTK port. If this header file can be opened, application may not recognize PLATFORM(EFL) macro.
Created attachment 148280 [details] Patch
(In reply to comment #18) > (From update of attachment 148269 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148269&action=review > > > Source/WebKit2/UIProcess/API/C/WKAPICast.h:371 > > +#if PLATFORM(EFL) > > Though I'm not sure whether WKAPICast.h is public, I think it is more safe to use #if defined(BUILDING_EFL__) like GTK port. If this header file can be opened, application may not recognize PLATFORM(EFL) macro. I modified like you mentioned although I believe that WKAPICast.h is not public.
Comment on attachment 148280 [details] Patch Looks better than before. Thanks.
Comment on attachment 148280 [details] Patch LGTM
Comment on attachment 148280 [details] Patch Thank you.
Comment on attachment 148280 [details] Patch Clearing flags on attachment: 148280 Committed r120925: <http://trac.webkit.org/changeset/120925>
All reviewed patches have been landed. Closing bug.