Bug 88935 – [EFL[WK2] Add WKViewEfl and WebKit2 API Object to represent Evas_Object.

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

[Ryuan Choi]

In order to support WTR/EFL,
we should provide WebKit2 API Object to represent Evas_Object although we will use ewk API.

[Ryuan Choi]

Created attachment 147194 [details]
Patch

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 147236 [details]
Patch

[Ryuan Choi]

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

[Gyuyoung Kim]

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

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 147264 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 147264 [details]
Patch

LGTM.

[Ryuan Choi]

Created attachment 147277 [details]
Patch

[Ryuan Choi]

Created attachment 147279 [details]
Patch

[Chris Dumez]

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?

[Ryuan Choi]

Created attachment 148266 [details]
Patch

[Ryuan Choi]

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

[Chris Dumez]

Comment on attachment 148266 [details]
Patch

LGTM.

[Chris Dumez]

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

[Ryuan Choi]

Created attachment 148269 [details]
Patch

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 148280 [details]
Patch

[Ryuan Choi]

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

[Gyuyoung Kim]

Comment on attachment 148280 [details]
Patch

Looks better than before. Thanks.

[Chang Shu]

Comment on attachment 148280 [details]
Patch

LGTM

[Ryuan Choi]

Comment on attachment 148280 [details]
Patch

Thank you.

[WebKit Review Bot]

Comment on attachment 148280 [details]
Patch

Clearing flags on attachment: 148280

Committed r120925: <http://trac.webkit.org/changeset/120925>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.