Bug 90662 – [EFL][WK2] Add NativeWebTouchEvent and handle the Touch event.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Eunmi Lee]

I will make Touch event patch for WK2 EFL port.

[Eunmi Lee]

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]

Created attachment 164362 [details]
Patch

[Gyuyoung Kim]

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]

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

Created attachment 164365 [details]
Patch

[Ryuan Choi]

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 on attachment 164365 [details]
Patch

Unit tests?

[Gyuyoung Kim]

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 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]

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

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

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

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

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

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

Created attachment 164491 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164491 [details]
Patch

LGTM now.

[Chris Dumez]

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 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 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]

(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 on attachment 164491 [details]
Patch

Clearing flags on attachment: 164491

Committed r128853: <http://trac.webkit.org/changeset/128853>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.