Bug 93270 - [EFL] Remove PlatformTouchEventEfl and PlatformTouchPointEfl
Summary: [EFL] Remove PlatformTouchEventEfl and PlatformTouchPointEfl
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: 94466
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-06 07:40 PDT by Ryuan Choi
Modified: 2012-08-20 04:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.97 KB, patch)
2012-08-06 08:07 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (17.71 KB, patch)
2012-08-07 01:49 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (18.36 KB, patch)
2012-08-07 07:18 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-08-06 07:40:56 PDT
In order to remove WebKit dependency from WebCore, 
codes of PlatformTouchEventEfl and PlatformTouchPointEfl should be moved to WebKit/Efl.
Comment 1 Ryuan Choi 2012-08-06 08:07:14 PDT
Created attachment 156697 [details]
Patch
Comment 2 Gyuyoung Kim 2012-08-06 22:22:33 PDT
I wonder if we should move touch event implementation from WebCore to WebKit. Can't we share this with WK2 ?  As you know, blackberry port also implemented this in WebCore.
Comment 3 Ryuan Choi 2012-08-06 22:56:28 PDT
(In reply to comment #2)
> I wonder if we should move touch event implementation from WebCore to WebKit. Can't we share this with WK2 ?  As you know, blackberry port also implemented this in WebCore.

WebKit2 also has own conversion logic for Touch event and it is more complicated because of IPC.
So, WebKit2 indirectly converts port specific structure to PlatformTouchEvent.

But, PlatformTouchEventEfl and PlatformTouchPointEfl initialize PlatformTouchEvent directly from Ewk_TouchEvent which is WebKit1/Efl API.

As a result, we can not share this logic and I think that WebCore should not depends on WebKit.

For touch event for WebKit2/Efl, you can see the Bug 90662.

Thank you.
Comment 4 Gyuyoung Kim 2012-08-06 23:26:20 PDT
Comment on attachment 156697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156697&action=review

> Source/WebCore/ChangeLog:12
> +        Ewk_Touch_XXX to PlatformTouchXXX.

I think it is good to mention that PlatformTouchEventEfl can't be shared with WK2.

> Source/WebKit/efl/ewk/ewk_frame.cpp:981
> +class WebKitPlatformTouchPoint : public WebCore::PlatformTouchPoint {

It looks this class can be moved to new file. For example, ewk_touch_event_private.h and so on.

> Source/WebKit/efl/ewk/ewk_frame.cpp:992
> +class WebKitPlatformTouchEvent : public WebCore::PlatformTouchEvent {

Ditto.
Comment 5 Ryuan Choi 2012-08-07 01:49:29 PDT
Created attachment 156894 [details]
Patch
Comment 6 Ryuan Choi 2012-08-07 02:21:03 PDT
(In reply to comment #4)
> (From update of attachment 156697 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156697&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Ewk_Touch_XXX to PlatformTouchXXX.
> 
> I think it is good to mention that PlatformTouchEventEfl can't be shared with WK2.
> 
I add comment little bit more.

> > Source/WebKit/efl/ewk/ewk_frame.cpp:981
> > +class WebKitPlatformTouchPoint : public WebCore::PlatformTouchPoint {
> 
> It looks this class can be moved to new file. For example, ewk_touch_event_private.h and so on.

I added ewk_touch_event.cpp which includes WebKitPlatformTouchPoint and WebKitPlatformTouchEvent, because WebKitPlatformTouchPoint is simple and only used in WebKitPlatformTouchEvent.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:992
> > +class WebKitPlatformTouchEvent : public WebCore::PlatformTouchEvent {
> 
> Ditto.
Comment 7 Gyuyoung Kim 2012-08-07 02:22:30 PDT
Comment on attachment 156894 [details]
Patch

Looks fine. Thanks.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-08-07 06:30:01 PDT
Comment on attachment 156894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156894&action=review

> Source/WebKit/efl/ewk/ewk_touch_event.cpp:27
> +#include "ewk_touch_event.h"

Isn't this a private header that should end in "_private.h"?

> Source/WebKit/efl/ewk/ewk_touch_event.cpp:41
> +    {
> +        m_id = id;
> +        m_state = state;
> +        m_screenPos = windowPos;
> +        m_pos = windowPos;
> +    }

You can move these to the constructor's initialization list.

> Source/WebKit/efl/ewk/ewk_touch_event.h:35
> +class WebKitPlatformTouchEvent : public WebCore::PlatformTouchEvent {
> +public:
> +    WebKitPlatformTouchEvent(const Eina_List* points, const WebCore::IntPoint& pos, Ewk_Touch_Event_Type action, unsigned modifiers);
> +};

Even for private headers and constructions, we normally hide the classes in the .cpp files and provide EFL-style functions in the headers which create them.
Comment 9 Ryuan Choi 2012-08-07 07:18:03 PDT
Created attachment 156935 [details]
Patch
Comment 10 Ryuan Choi 2012-08-07 07:20:18 PDT
(In reply to comment #8)
> (From update of attachment 156894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156894&action=review
> 
> > Source/WebKit/efl/ewk/ewk_touch_event.cpp:27
> > +#include "ewk_touch_event.h"
> 
> Isn't this a private header that should end in "_private.h"?
> 
OK, I did.

> > Source/WebKit/efl/ewk/ewk_touch_event.cpp:41
> > +    {
> > +        m_id = id;
> > +        m_state = state;
> > +        m_screenPos = windowPos;
> > +        m_pos = windowPos;
> > +    }
> 
> You can move these to the constructor's initialization list.
Because these are members of parent class, I can't move them to initialization list.

> 
> > Source/WebKit/efl/ewk/ewk_touch_event.h:35
> > +class WebKitPlatformTouchEvent : public WebCore::PlatformTouchEvent {
> > +public:
> > +    WebKitPlatformTouchEvent(const Eina_List* points, const WebCore::IntPoint& pos, Ewk_Touch_Event_Type action, unsigned modifiers);
> > +};
> 
> Even for private headers and constructions, we normally hide the classes in the .cpp files and provide EFL-style functions in the headers which create them.

I tried like you mentioned.

Thank you.
Comment 11 Eric Seidel (no email) 2012-08-07 15:15:57 PDT
Comment on attachment 156935 [details]
Patch

rs=me.
Comment 12 WebKit Review Bot 2012-08-07 16:52:34 PDT
Comment on attachment 156935 [details]
Patch

Clearing flags on attachment: 156935

Committed r124945: <http://trac.webkit.org/changeset/124945>
Comment 13 WebKit Review Bot 2012-08-07 16:52:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Dirk Pranke 2012-08-07 18:25:27 PDT
fwiw, we forgot to remove the files from Chromium's WebCore.gypi, so this broke the Chromium win build. Fixed in http://trac.webkit.org/changeset/124965, hopefully.
Comment 15 Ryuan Choi 2012-08-07 18:35:44 PDT
(In reply to comment #14)
> fwiw, we forgot to remove the files from Chromium's WebCore.gypi, so this broke the Chromium win build. Fixed in http://trac.webkit.org/changeset/124965, hopefully.

Oops, sorry I didn't know it.
I will remember.
Thank you for your information and fixing.