WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
90662
[EFL][WK2] Add NativeWebTouchEvent and handle the Touch event.
https://bugs.webkit.org/show_bug.cgi?id=90662
Summary
[EFL][WK2] Add NativeWebTouchEvent and handle the Touch event.
Eunmi Lee
Reported
2012-07-06 00:29:20 PDT
I will make Touch event patch for WK2 EFL port.
Attachments
[WIP] Patch for the Touch event.
(9.77 KB, patch)
2012-07-06 01:13 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(10.42 KB, patch)
2012-09-17 03:05 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(10.38 KB, patch)
2012-09-17 04:13 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(10.29 KB, patch)
2012-09-17 21:49 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Eunmi Lee
Comment 1
2012-07-06 01:13:45 PDT
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.
Eunmi Lee
Comment 2
2012-09-17 03:05:55 PDT
Created
attachment 164362
[details]
Patch
Gyuyoung Kim
Comment 3
2012-09-17 03:27:31 PDT
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.
Eunmi Lee
Comment 4
2012-09-17 03:41:15 PDT
(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.
Eunmi Lee
Comment 5
2012-09-17 04:13:39 PDT
Created
attachment 164365
[details]
Patch
Ryuan Choi
Comment 6
2012-09-17 05:43:22 PDT
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.
Raphael Kubo da Costa (:rakuco)
Comment 7
2012-09-17 06:21:24 PDT
Comment on
attachment 164365
[details]
Patch Unit tests?
Gyuyoung Kim
Comment 8
2012-09-17 06:22:00 PDT
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.
Ryuan Choi
Comment 9
2012-09-17 06:57:25 PDT
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:`
Eunmi Lee
Comment 10
2012-09-17 18:59:59 PDT
(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)
Eunmi Lee
Comment 11
2012-09-17 19:01:55 PDT
(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?
Eunmi Lee
Comment 12
2012-09-17 19:10:50 PDT
(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.
Eunmi Lee
Comment 13
2012-09-17 19:19:40 PDT
(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?
Gyuyoung Kim
Comment 14
2012-09-17 19:30:26 PDT
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; }
Eunmi Lee
Comment 15
2012-09-17 20:11:59 PDT
(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.
Eunmi Lee
Comment 16
2012-09-17 20:17:40 PDT
(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.
Eunmi Lee
Comment 17
2012-09-17 21:49:39 PDT
Created
attachment 164491
[details]
Patch
Gyuyoung Kim
Comment 18
2012-09-17 22:21:42 PDT
Comment on
attachment 164491
[details]
Patch LGTM now.
Chris Dumez
Comment 19
2012-09-17 22:55:35 PDT
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.
Chris Dumez
Comment 20
2012-09-17 22:58:54 PDT
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.
Ryuan Choi
Comment 21
2012-09-17 23:03:34 PDT
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.
Chris Dumez
Comment 22
2012-09-17 23:36:29 PDT
(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...).
WebKit Review Bot
Comment 23
2012-09-17 23:44:46 PDT
Comment on
attachment 164491
[details]
Patch Clearing flags on attachment: 164491 Committed
r128853
: <
http://trac.webkit.org/changeset/128853
>
WebKit Review Bot
Comment 24
2012-09-17 23:44:51 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.
Top of Page
Format For Printing
XML
Clone This Bug