Bug 93270 – [EFL] Remove PlatformTouchEventEfl and PlatformTouchPointEfl

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

[Ryuan Choi]

In order to remove WebKit dependency from WebCore, 
codes of PlatformTouchEventEfl and PlatformTouchPointEfl should be moved to WebKit/Efl.

[Ryuan Choi]

Created attachment 156697 [details]
Patch

[Gyuyoung Kim]

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.

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 156894 [details]
Patch

[Ryuan Choi]

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

[Gyuyoung Kim]

Comment on attachment 156894 [details]
Patch

Looks fine. Thanks.

[Raphael Kubo da Costa (:rakuco)]

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.

[Ryuan Choi]

Created attachment 156935 [details]
Patch

[Ryuan Choi]

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

[Eric Seidel (no email)]

Comment on attachment 156935 [details]
Patch

rs=me.

[WebKit Review Bot]

Comment on attachment 156935 [details]
Patch

Clearing flags on attachment: 156935

Committed r124945: <http://trac.webkit.org/changeset/124945>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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.

[Ryuan Choi]

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