Bug 88935

Summary:

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

Product:

WebKit

Reporter:

Ryuan Choi <ryuan.choi>

Component:

WebKit EFL

Assignee:

Ryuan Choi <ryuan.choi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cshu, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

87659

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Ryuan Choi

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

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Ryuan Choi

Comment 1 2012-06-12 17:04:14 PDT

Created attachment 147194 [details] Patch

Gyuyoung Kim

Comment 2 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.

Ryuan Choi

Comment 3 2012-06-12 22:46:20 PDT

Created attachment 147236 [details] Patch

Ryuan Choi

Comment 4 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.

Gyuyoung Kim

Comment 5 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?

Ryuan Choi

Comment 6 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?

Ryuan Choi

Comment 7 2012-06-13 02:11:08 PDT

Created attachment 147264 [details] Patch

Gyuyoung Kim

Comment 8 2012-06-13 02:52:12 PDT

Comment on attachment 147264 [details] Patch LGTM.

Ryuan Choi

Comment 9 2012-06-13 03:33:10 PDT

Created attachment 147277 [details] Patch

Ryuan Choi

Comment 10 2012-06-13 03:39:23 PDT

Created attachment 147279 [details] Patch

Chris Dumez

Comment 11 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?

Ryuan Choi

Comment 12 2012-06-18 23:48:04 PDT

Created attachment 148266 [details] Patch

Ryuan Choi

Comment 13 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.

Chris Dumez

Comment 14 2012-06-18 23:52:33 PDT

Comment on attachment 148266 [details] Patch LGTM.

Chris Dumez

Comment 15 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.

Ryuan Choi

Comment 16 2012-06-19 00:04:16 PDT

Created attachment 148269 [details] Patch

Ryuan Choi

Comment 17 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.

Gyuyoung Kim

Comment 18 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.

Ryuan Choi

Comment 19 2012-06-19 01:10:41 PDT

Created attachment 148280 [details] Patch

Ryuan Choi

Comment 20 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.

Gyuyoung Kim

Comment 21 2012-06-19 03:51:44 PDT

Comment on attachment 148280 [details] Patch Looks better than before. Thanks.

Chang Shu

Comment 22 2012-06-21 06:40:25 PDT

Comment on attachment 148280 [details] Patch LGTM

Ryuan Choi

Comment 23 2012-06-21 07:17:53 PDT

Comment on attachment 148280 [details] Patch Thank you.

WebKit Review Bot

Comment 24 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>

WebKit Review Bot

Comment 25 2012-06-21 08:12:12 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.