Bug 88935 - [EFL[WK2] Add WKViewEfl and WebKit2 API Object to represent Evas_Object.
Summary: [EFL[WK2] Add WKViewEfl and WebKit2 API Object to represent Evas_Object.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks: 87659
  Show dependency treegraph
 
Reported: 2012-06-12 16:42 PDT by Ryuan Choi
Modified: 2012-06-21 08:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.05 KB, patch)
2012-06-12 17:04 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (10.09 KB, patch)
2012-06-12 22:46 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (10.60 KB, patch)
2012-06-13 02:11 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (10.59 KB, patch)
2012-06-13 03:33 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2012-06-13 03:39 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (11.08 KB, patch)
2012-06-18 23:48 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2012-06-19 00:04 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (11.06 KB, patch)
2012-06-19 01:10 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2012-06-12 16:42:59 PDT
In order to support WTR/EFL,
we should provide WebKit2 API Object to represent Evas_Object although we will use ewk API.
Comment 1 Ryuan Choi 2012-06-12 17:04:14 PDT
Created attachment 147194 [details]
Patch
Comment 2 Gyuyoung Kim 2012-06-12 17:51:34 PDT
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.
Comment 3 Ryuan Choi 2012-06-12 22:46:20 PDT
Created attachment 147236 [details]
Patch
Comment 4 Ryuan Choi 2012-06-12 22:48:05 PDT
(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.
Comment 5 Gyuyoung Kim 2012-06-13 00:52:55 PDT
(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?
Comment 6 Ryuan Choi 2012-06-13 01:49:42 PDT
(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?
Comment 7 Ryuan Choi 2012-06-13 02:11:08 PDT
Created attachment 147264 [details]
Patch
Comment 8 Gyuyoung Kim 2012-06-13 02:52:12 PDT
Comment on attachment 147264 [details]
Patch

LGTM.
Comment 9 Ryuan Choi 2012-06-13 03:33:10 PDT
Created attachment 147277 [details]
Patch
Comment 10 Ryuan Choi 2012-06-13 03:39:23 PDT
Created attachment 147279 [details]
Patch
Comment 11 Chris Dumez 2012-06-18 23:20:52 PDT
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?
Comment 12 Ryuan Choi 2012-06-18 23:48:04 PDT
Created attachment 148266 [details]
Patch
Comment 13 Ryuan Choi 2012-06-18 23:51:07 PDT
(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 14 Chris Dumez 2012-06-18 23:52:33 PDT
Comment on attachment 148266 [details]
Patch

LGTM.
Comment 15 Chris Dumez 2012-06-19 00:01:57 PDT
(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.
Comment 16 Ryuan Choi 2012-06-19 00:04:16 PDT
Created attachment 148269 [details]
Patch
Comment 17 Ryuan Choi 2012-06-19 00:05:16 PDT
(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 18 Gyuyoung Kim 2012-06-19 01:07:39 PDT
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.
Comment 19 Ryuan Choi 2012-06-19 01:10:41 PDT
Created attachment 148280 [details]
Patch
Comment 20 Ryuan Choi 2012-06-19 01:18:00 PDT
(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 21 Gyuyoung Kim 2012-06-19 03:51:44 PDT
Comment on attachment 148280 [details]
Patch

Looks better than before. Thanks.
Comment 22 Chang Shu 2012-06-21 06:40:25 PDT
Comment on attachment 148280 [details]
Patch

LGTM
Comment 23 Ryuan Choi 2012-06-21 07:17:53 PDT
Comment on attachment 148280 [details]
Patch

Thank you.
Comment 24 WebKit Review Bot 2012-06-21 08:12:05 PDT
Comment on attachment 148280 [details]
Patch

Clearing flags on attachment: 148280

Committed r120925: <http://trac.webkit.org/changeset/120925>
Comment 25 WebKit Review Bot 2012-06-21 08:12:12 PDT
All reviewed patches have been landed.  Closing bug.