Summary: | [EFL] Remove PlatformTouchEventEfl and PlatformTouchPointEfl | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||
Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dpranke, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 94466 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Ryuan Choi
2012-08-06 07:40:56 PDT
Created attachment 156697 [details]
Patch
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. (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 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. Created attachment 156894 [details]
Patch
(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 on attachment 156894 [details]
Patch
Looks fine. Thanks.
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. Created attachment 156935 [details]
Patch
(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 on attachment 156935 [details]
Patch
rs=me.
Comment on attachment 156935 [details] Patch Clearing flags on attachment: 156935 Committed r124945: <http://trac.webkit.org/changeset/124945> All reviewed patches have been landed. Closing bug. 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. (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. |