Bug 90662

Created attachment 151031 [details] [WIP] Patch for the Touch event. This patch is work in progress. I will update patch when Bug 88631 is finished.

Created attachment 164362 [details] Patch

Comment on attachment 164362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164362&action=review Don't you need to add ewk_touch.h file to installation file list ? > Source/WebKit2/PlatformEfl.cmake:113 > +IF (ENABLE_TOUCH_EVENTS) Why don't you move this macro to file side? Then, move this file to existing file list. > Source/WebKit2/Shared/efl/WebEventFactory.cpp:245 > + touchPoints.append(WebPlatformTouchPoint(point->id, state, IntPoint(point->x, point->y), IntPoint(point->x - position->x, point->y - position->y))); Could you explain how to make touch point item ? I mean what is point, what is position.

(In reply to comment #3) > (From update of attachment 164362 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164362&action=review > > Don't you need to add ewk_touch.h file to installation file list ? > I don't need to do that here because it is not used in the outside yet :) > > Source/WebKit2/PlatformEfl.cmake:113 > > +IF (ENABLE_TOUCH_EVENTS) > > Why don't you move this macro to file side? Then, move this file to existing file list. > OK, I will move that. > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:245 > > + touchPoints.append(WebPlatformTouchPoint(point->id, state, IntPoint(point->x, point->y), IntPoint(point->x - position->x, point->y - position->y))); > > Could you explain how to make touch point item ? I mean what is point, what is position. We can make touch point item by creating new Ewk_Touch_Point and make list of touch points by adding pointer of Ewk_Touch_Point to the Eina_List. The point is Ewk_Touch_Point and the position is ewk_view's position in the screen. I will add API to feed touch event in the Bug 96903 and codes in this patch will be used there.

Created attachment 164365 [details] Patch

Comment on attachment 164365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review > Source/WebKit2/Shared/efl/WebEventFactory.cpp:218 > + WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0); I think that 0 may be TouchReleased. If you want to set default value for error case(default), I think that TouchCancelled looks better. And I think that it should be moved into EINA_LIST_FOREACH. Now this is only for first item. The state of second item will be the state of previous item when point->state is error case.

Comment on attachment 164365 [details] Patch Unit tests?

Comment on attachment 164365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review Set r? again. It looks take a look further a little bit. > Source/WebKit2/Shared/efl/WebEventFactory.cpp:199 > + switch (type) { It looks it is better to add new function to convert from EFL event type to WebKit event type as QT port. I think this function can be used by other functions. http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp#L66 >> Source/WebKit2/Shared/efl/WebEventFactory.cpp:218 >> + WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0); > > I think that 0 may be TouchReleased. > If you want to set default value for error case(default), I think that TouchCancelled looks better. > > And I think that it should be moved into EINA_LIST_FOREACH. > Now this is only for first item. > The state of second item will be the state of previous item when point->state is error case. Should we move *state* initialization into EINA_LIST_FOREACH loop? Evas only supports 5 states as below, 504 /** 505 * State of Evas_Coord_Touch_Point 506 */ 507 typedef enum _Evas_Touch_Point_State 508 { 509 EVAS_TOUCH_POINT_DOWN, /**< Touch point is pressed down */ 510 EVAS_TOUCH_POINT_UP, /**< Touch point is released */ 511 EVAS_TOUCH_POINT_MOVE, /**< Touch point is moved */ 512 EVAS_TOUCH_POINT_STILL, /**< Touch point is not moved after pressed */ 513 EVAS_TOUCH_POINT_CANCEL /**< Touch point is cancelled */ 514 } Evas_Touch_Point_State; It looks QT port just sets this out of loop. But, I don't mind to move it into loop inside.

Comment on attachment 164365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review >>> Source/WebKit2/Shared/efl/WebEventFactory.cpp:218 >>> + WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0); >> >> I think that 0 may be TouchReleased. >> If you want to set default value for error case(default), I think that TouchCancelled looks better. >> >> And I think that it should be moved into EINA_LIST_FOREACH. >> Now this is only for first item. >> The state of second item will be the state of previous item when point->state is error case. > > Should we move *state* initialization into EINA_LIST_FOREACH loop? Evas only supports 5 states as below, > > 504 /** > 505 * State of Evas_Coord_Touch_Point > 506 */ > 507 typedef enum _Evas_Touch_Point_State > 508 { > 509 EVAS_TOUCH_POINT_DOWN, /**< Touch point is pressed down */ > 510 EVAS_TOUCH_POINT_UP, /**< Touch point is released */ > 511 EVAS_TOUCH_POINT_MOVE, /**< Touch point is moved */ > 512 EVAS_TOUCH_POINT_STILL, /**< Touch point is not moved after pressed */ > 513 EVAS_TOUCH_POINT_CANCEL /**< Touch point is cancelled */ > 514 } Evas_Touch_Point_State; > > It looks QT port just sets this out of loop. But, I don't mind to move it into loop inside. If we consider error case (case default:), yes it should be moved because the state of previous item will be remained and used. Anyway, this is not pretty common case. So, I think that it looks better to declare state simply and assign it to reasonable error value in case of `default:`

(In reply to comment #6) > (From update of attachment 164365 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review > > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:218 > > + WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0); > > I think that 0 may be TouchReleased. > If you want to set default value for error case(default), I think that TouchCancelled looks better. > > And I think that it should be moved into EINA_LIST_FOREACH. > Now this is only for first item. > The state of second item will be the state of previous item when point->state is error case. OK, it looks better to use TouchCancelled. and, we don't have to move that into the EINA_LIST_FOREACH because we don't add point to the points list in error case. (214 line)

(In reply to comment #7) > (From update of attachment 164365 [details]) > Unit tests? I didn't add new API, so I don't have to make EFL API unit tests. Do you mean different unit tests?

(In reply to comment #10) > (In reply to comment #6) > > (From update of attachment 164365 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review > > > > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:218 > > > + WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0); > > > > I think that 0 may be TouchReleased. > > If you want to set default value for error case(default), I think that TouchCancelled looks better. > > > > And I think that it should be moved into EINA_LIST_FOREACH. > > Now this is only for first item. > > The state of second item will be the state of previous item when point->state is error case. > > OK, it looks better to use TouchCancelled. > and, > we don't have to move that into the EINA_LIST_FOREACH because we don't add point to the points list in error case. (214 line) ah, I've understand what you mean. Yes, we need to set state for error case. I will modify the codes.

(In reply to comment #8) > (From update of attachment 164365 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review > > Set r? again. It looks take a look further a little bit. > > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:199 > > + switch (type) { > > It looks it is better to add new function to convert from EFL event type to WebKit event type as QT port. I think this function can be used by other functions. > > http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp#L66 > I can make type converting function but I don't think it will be used by other functions. In the Qt port, they have common type of events (QEvent), so it is useful to make type converting function. but in the EFL port, we don't have common type of events and we have types for each event. (Evas_Event_Mouse_Move, Evas_Event_Mouse_Wheel and so on) So, we need type converting function only for Touch event types which is defined for WebKit2 EFL port and it will be used only in the createWebTouchEvent(). Do I have to make converting function in this case?

Comment on attachment 164365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review >>> Source/WebKit2/Shared/efl/WebEventFactory.cpp:199 >>> + switch (type) { >> >> It looks it is better to add new function to convert from EFL event type to WebKit event type as QT port. I think this function can be used by other functions. >> >> http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp#L66 > > I can make type converting function but I don't think it will be used by other functions. > > In the Qt port, they have common type of events (QEvent), so it is useful to make type converting function. > but in the EFL port, we don't have common type of events and we have types for each event. (Evas_Event_Mouse_Move, Evas_Event_Mouse_Wheel and so on) > So, we need type converting function only for Touch event types which is defined for WebKit2 EFL port and it will be used only in the createWebTouchEvent(). > Do I have to make converting function in this case? If so, I don't mind to keep this here. Nevertheless, I prefer to move this to new function for code readability as below, static inline WebMouseEvent::Button buttonForEvent(int button) { if (button == LeftButton) return WebMouseEvent::LeftButton; if (button == MiddleButton) return WebMouseEvent::MiddleButton; if (button == RightButton) return WebMouseEvent::RightButton; return WebMouseEvent::NoButton; }

(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #6) > > > (From update of attachment 164365 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review > > > > > > > Source/WebKit2/Shared/efl/WebEventFactory.cpp:218 > > > > + WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0); > > > > > > I think that 0 may be TouchReleased. > > > If you want to set default value for error case(default), I think that TouchCancelled looks better. > > > > > > And I think that it should be moved into EINA_LIST_FOREACH. > > > Now this is only for first item. > > > The state of second item will be the state of previous item when point->state is error case. > > > > OK, it looks better to use TouchCancelled. > > and, > > we don't have to move that into the EINA_LIST_FOREACH because we don't add point to the points list in error case. (214 line) > > ah, I've understand what you mean. > Yes, we need to set state for error case. I will modify the codes. As we talked about this in the IRC, the default case should not be reached and I've added ASSERT_NOT_REACHED. So, I think we don't have to add defense code for that again. That means, we also don't have to initialize state variable because it will be always set (except ASSERT_NOT_REACHED case). So, I will modify codes to remove initializing code simply.

(In reply to comment #14) > (From update of attachment 164365 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164365&action=review > > >>> Source/WebKit2/Shared/efl/WebEventFactory.cpp:199 > >>> + switch (type) { > >> > >> It looks it is better to add new function to convert from EFL event type to WebKit event type as QT port. I think this function can be used by other functions. > >> > >> http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp#L66 > > > > I can make type converting function but I don't think it will be used by other functions. > > > > In the Qt port, they have common type of events (QEvent), so it is useful to make type converting function. > > but in the EFL port, we don't have common type of events and we have types for each event. (Evas_Event_Mouse_Move, Evas_Event_Mouse_Wheel and so on) > > So, we need type converting function only for Touch event types which is defined for WebKit2 EFL port and it will be used only in the createWebTouchEvent(). > > Do I have to make converting function in this case? > > If so, I don't mind to keep this here. Nevertheless, I prefer to move this to new function for code readability as below, > > > static inline WebMouseEvent::Button buttonForEvent(int button) > { > if (button == LeftButton) > return WebMouseEvent::LeftButton; > if (button == MiddleButton) > return WebMouseEvent::MiddleButton; > if (button == RightButton) > return WebMouseEvent::RightButton; > > return WebMouseEvent::NoButton; > } The buttonForEvent() function is used 3 times in the WebEventFactory.cpp file. It seems that you like to make new function for touch type converting, so I will make function for that.

Created attachment 164491 [details] Patch

Comment on attachment 164491 [details] Patch LGTM now.

Comment on attachment 164491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164491&action=review > Source/WebKit2/Shared/efl/WebEventFactory.cpp:243 > + return WebTouchEvent(typeForTouchEvent(type), touchPoints, modifiersForEvent(modifiers), timestamp); Why can't we simply cast type here? We already have COMPILE_ASSERT_MATCHING_ENUM() to make sure the enums match at compile time. This would be more efficient.

Comment on attachment 164491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164491&action=review > Source/WebKit2/UIProcess/API/efl/ewk_touch.h:26 > +#ifndef ewk_touch_h Is this supposed to be a public header? If not, then the naming is not correct (ewk_touch_private.h) ? If it is supposed to be public, then it should be added to EWebKit2.h and Cmake Headers install.

Comment on attachment 164491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164491&action=review >> Source/WebKit2/Shared/efl/WebEventFactory.cpp:243 >> + return WebTouchEvent(typeForTouchEvent(type), touchPoints, modifiersForEvent(modifiers), timestamp); > > Why can't we simply cast type here? We already have COMPILE_ASSERT_MATCHING_ENUM() to make sure the enums match at compile time. This would be more efficient. I am pan of COMPILE_ASSERT_MATCHING_ENUM. but WebEvent::Type contains all event types including TouchXXX. Unfortunately, we can not cast.

(In reply to comment #21) > (From update of attachment 164491 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164491&action=review > > >> Source/WebKit2/Shared/efl/WebEventFactory.cpp:243 > >> + return WebTouchEvent(typeForTouchEvent(type), touchPoints, modifiersForEvent(modifiers), timestamp); > > > > Why can't we simply cast type here? We already have COMPILE_ASSERT_MATCHING_ENUM() to make sure the enums match at compile time. This would be more efficient. > > I am pan of COMPILE_ASSERT_MATCHING_ENUM. > but WebEvent::Type contains all event types including TouchXXX. > > Unfortunately, we can not cast. After discussing this on IRC, I no longer have any objection to this landing. Please go ahead and sorry I joined the discussion a bit late (many meetings...).

Comment on attachment 164491 [details] Patch Clearing flags on attachment: 164491 Committed r128853: <http://trac.webkit.org/changeset/128853>

All reviewed patches have been landed. Closing bug.