Bug 96903 – [EFL][WK2] Add API to feed touch event.

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

[Eunmi Lee]

Add API to feed touch event to the view.

[Eunmi Lee]

Created attachment 164364 [details]
Patch

[Ryuan Choi]

Comment on attachment 164364 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164364&action=review

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:365
> +TEST_F(EWK2UnitTestBase, ewk_view_feed_touch_event)

I hope that we have more useful tests for this?
Is it possible?

[Eunmi Lee]

Comment on attachment 164364 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164364&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:365
>> +TEST_F(EWK2UnitTestBase, ewk_view_feed_touch_event)
> 
> I hope that we have more useful tests for this?
> Is it possible?

Do you want to see that touch event is working?
The touch event behavior will be tested in the WTR, so I think we don't have to do duplicated tests here.
How do you think?

[Eunmi Lee]

Created attachment 164517 [details]
Patch

[Chris Dumez]

*** Bug 96792 has been marked as a duplicate of this bug. ***

[Gyuyoung Kim]

Comment on attachment 164517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164517&action=review

Is there any unskip test case by this patch ?

> Source/WebKit2/PlatformEfl.cmake:226
> +    "${CMAKE_CURRENT_SOURCE_DIR}/UIProcess/API/efl/ewk_touch.h"

Eunmi, please add public file to EWebKit2.h together when the public file is added to mainline from next time.

[Eunmi Lee]

(In reply to comment #6)
> (From update of attachment 164517 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164517&action=review
> 
> Is there any unskip test case by this patch ?
> 

Yes, this patch is needed to test touch event test cases in Bug 96465.

> > Source/WebKit2/PlatformEfl.cmake:226
> > +    "${CMAKE_CURRENT_SOURCE_DIR}/UIProcess/API/efl/ewk_touch.h"
> 
> Eunmi, please add public file to EWebKit2.h together when the public file is added to mainline from next time.

You can see the ewk_touch.h was already added in the patch.

[Gyuyoung Kim]

(In reply to comment #7)
 
> You can see the ewk_touch.h was already added in the patch.

I mean ewk_touch.h needed to be added to EWebKit2.h in the bug 90662 together. Because, the file has public API file style. We needed to deal it as public API in the bug.

[Gyuyoung Kim]

Comment on attachment 164517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164517&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:672
> + *        from the Evas using evas_key_modifier_get() and enable and disable

It looks *and enable and disable* is ambiguous statement.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:596
> +    Ewk_Touch_Point point = {0, 0, 0, EVAS_TOUCH_POINT_DOWN};

I think we need to test other touch events as well.

EVAS_TOUCH_POINT_UP,EVAS_TOUCH_POINT_MOVE, EVAS_TOUCH_POINT_STILL, EVAS_TOUCH_POINT_CANCEL

[Ryuan Choi]

(In reply to comment #3)
> (From update of attachment 164364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164364&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:365
> >> +TEST_F(EWK2UnitTestBase, ewk_view_feed_touch_event)
> > 
> > I hope that we have more useful tests for this?
> > Is it possible?
> 
> Do you want to see that touch event is working?
> The touch event behavior will be tested in the WTR, so I think we don't have to do duplicated tests here.
> How do you think?

OK.
Although I still want better API tests regardless of layout tests, the reason looks weaker than the cost.
So I don't have objection about this.

[Eunmi Lee]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > You can see the ewk_touch.h was already added in the patch.
> 
> I mean ewk_touch.h needed to be added to EWebKit2.h in the bug 90662 together. Because, the file has public API file style. We needed to deal it as public API in the bug.

OK, I understand. I will do like that next time :)

[Eunmi Lee]

(In reply to comment #9)
> (From update of attachment 164517 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164517&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:672
> > + *        from the Evas using evas_key_modifier_get() and enable and disable
> 
> It looks *and enable and disable* is ambiguous statement.
> 

OK, I will change the statement.

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:596
> > +    Ewk_Touch_Point point = {0, 0, 0, EVAS_TOUCH_POINT_DOWN};
> 
> I think we need to test other touch events as well.
> 
> EVAS_TOUCH_POINT_UP,EVAS_TOUCH_POINT_MOVE, EVAS_TOUCH_POINT_STILL, EVAS_TOUCH_POINT_CANCEL

Yes, I will add them.

[Eunmi Lee]

(In reply to comment #10)
> (In reply to comment #3)
> > (From update of attachment 164364 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164364&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:365
> > >> +TEST_F(EWK2UnitTestBase, ewk_view_feed_touch_event)
> > > 
> > > I hope that we have more useful tests for this?
> > > Is it possible?
> > 
> > Do you want to see that touch event is working?
> > The touch event behavior will be tested in the WTR, so I think we don't have to do duplicated tests here.
> > How do you think?
> 
> OK.
> Although I still want better API tests regardless of layout tests, the reason looks weaker than the cost.
> So I don't have objection about this.

OK, thanks for your comment :)

[Eunmi Lee]

Created attachment 164695 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164695&action=review

Any other opinions for this patch ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1705
> +    Evas_Point position = {smartData->view.x, smartData->view.y};

{smartData->view.x, smartData->view.y} -> { smartData->view.x, smartData->view.y }; ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:620
> +    Ewk_Touch_Point point1 = {0, 0, 0, EVAS_TOUCH_POINT_DOWN};

ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:621
> +    Ewk_Touch_Point point2 = {1, 0, 0, EVAS_TOUCH_POINT_DOWN};

ditto.

[Jinwoo Song]

Comment on attachment 164695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164695&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:671
> + * @param modifiers modifiers state of touch event. Users can get the Evas_Modifier

'the list of modifier keys for touch event' seems to be correct rather than 'modifiers state of touch event'. Is that right?

[Eunmi Lee]

(In reply to comment #15)
> (From update of attachment 164695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164695&action=review
> 
> Any other opinions for this patch ?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1705
> > +    Evas_Point position = {smartData->view.x, smartData->view.y};
> 
> {smartData->view.x, smartData->view.y} -> { smartData->view.x, smartData->view.y }; ?
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:620
> > +    Ewk_Touch_Point point1 = {0, 0, 0, EVAS_TOUCH_POINT_DOWN};
> 
> ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:621
> > +    Ewk_Touch_Point point2 = {1, 0, 0, EVAS_TOUCH_POINT_DOWN};
> 
> ditto.

Thanks, I will modify as your comment.

[Eunmi Lee]

(In reply to comment #16)
> (From update of attachment 164695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164695&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:671
> > + * @param modifiers modifiers state of touch event. Users can get the Evas_Modifier
> 
> 'the list of modifier keys for touch event' seems to be correct rather than 'modifiers state of touch event'. Is that right?

I agree that 'modifiers state' is ambiguous, but I don't think 'the list of modifier keys' is good sentence.
so, I will change sentence to 'a handle to the list of modifier keys registered' which is referred from the Evas document.

[Eunmi Lee]

Created attachment 164878 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164878&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1699
> +Eina_Bool ewk_view_feed_touch_event(Evas_Object* ewkView, Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)
> +{

This seems way too simple to me. How can you determine which finger was pressed before and now released/moved etc?

I hope the touch support in EFL is not that broken. Qt does this quite nicely.

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> (From update of attachment 164878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164878&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1699
> > +Eina_Bool ewk_view_feed_touch_event(Evas_Object* ewkView, Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)
> > +{
> 
> This seems way too simple to me. How can you determine which finger was pressed before and now released/moved etc?
> 
> I hope the touch support in EFL is not that broken. Qt does this quite nicely.

Apparently evas supports this just fine there are even methods for feeding events

evas_feed_multi* etc. I guess you just have to send all the touches using the same timestamp.

[Kenneth Rohde Christiansen]

Comment on attachment 164878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164878&action=review

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:631
> +    ASSERT_TRUE(ewk_view_feed_touch_event(webView(), EWK_TOUCH_START, points, evas_key_modifier_get(evas_object_evas_get(webView()))));
> +
> +    point1.state = EVAS_TOUCH_POINT_STILL;
> +    point2.x = 100;
> +    point2.y = 100;
> +    point2.state = EVAS_TOUCH_POINT_MOVE;
> +    ASSERT_TRUE(ewk_view_feed_touch_event(webView(), EWK_TOUCH_MOVE, points, evas_key_modifier_get(evas_object_evas_get(webView()))));
> +

I dont get why it is not possible to place two fingers and then remove just one. Then the resulting event should contain two points (one released and one stationary)

[Eunmi Lee]

Comment on attachment 164878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164878&action=review

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1699
>>> +{
>> 
>> This seems way too simple to me. How can you determine which finger was pressed before and now released/moved etc?
>> 
>> I hope the touch support in EFL is not that broken. Qt does this quite nicely.
> 
> Apparently evas supports this just fine there are even methods for feeding events
> 
> evas_feed_multi* etc. I guess you just have to send all the touches using the same timestamp.

We can determine each finger's state by "const Eina_List* points" parameter,
because the type of each point is Ewk_Touch_Point which has id, x, y and state (Evas_Touch_Point_Sate).

I know there are APIs in the Evas for feeding events but I could not use that, because I can not send multiple points using that API.
We need API to feed touch event with multiple points at a time,
especially in the WTR, we can not pass the test cases without this API.

In the Qt port, we can feed touch events with multiple points because QTouchEvent can have multiple points,
but there is no API and type like that in the Evas.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:631
>> +
> 
> I dont get why it is not possible to place two fingers and then remove just one. Then the resulting event should contain two points (one released and one stationary)

Why not? it is possible. Do you want to add such test case?

[Kenneth Rohde Christiansen]

Comment on attachment 164878 [details]
Patch

Yes thanks please add such a test case!

[Eunmi Lee]

Created attachment 164895 [details]
Patch

[Eunmi Lee]

Created attachment 164905 [details]
Patch - rebased

[Kenneth Rohde Christiansen]

Comment on attachment 164905 [details]
Patch - rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=164905&action=review

> Source/WebKit2/ChangeLog:8
> +        This API will be used by applications to feed touch event to the view

Could you explain the use-case?

[Eunmi Lee]

Created attachment 165028 [details]
Patch

[Eunmi Lee]

(In reply to comment #27)
> (From update of attachment 164905 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164905&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        This API will be used by applications to feed touch event to the view
> 
> Could you explain the use-case?

I've modified ChangeLog, please review again :)

[Kenneth Rohde Christiansen]

Comment on attachment 165028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165028&action=review

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:740
> +    point2.state = EVAS_TOUCH_POINT_MOVE;
> +    ASSERT_TRUE(ewk_view_feed_touch_event(webView(), EWK_TOUCH_MOVE, points, evas_key_modifier_get(evas_object_evas_get(webView()))));
> +

Wouldnt it be better to actually test that it gets the points? like load a page with a touch event handler and query the state with evaluatejavascript or similar (Qt has the option to do so)

[WebKit Review Bot]

Comment on attachment 165028 [details]
Patch

Clearing flags on attachment: 165028

Committed r129196: <http://trac.webkit.org/changeset/129196>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.