Bug 96903

Summary: [EFL][WK2] Add API to feed touch event.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit EFLAssignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, kenneth, lucas.de.marchi, naginenis, rakuco, ryuan.choi, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90662    
Bug Blocks: 61838, 96465    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch - rebased
none
Patch none

Description Eunmi Lee 2012-09-17 03:18:25 PDT
Add API to feed touch event to the view.
Comment 1 Eunmi Lee 2012-09-17 03:50:07 PDT
Created attachment 164364 [details]
Patch
Comment 2 Ryuan Choi 2012-09-17 06:58:29 PDT
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?
Comment 3 Eunmi Lee 2012-09-18 01:43:49 PDT
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?
Comment 4 Eunmi Lee 2012-09-18 01:51:27 PDT
Created attachment 164517 [details]
Patch
Comment 5 Chris Dumez 2012-09-18 05:15:45 PDT
*** Bug 96792 has been marked as a duplicate of this bug. ***
Comment 6 Gyuyoung Kim 2012-09-18 19:33:54 PDT
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.
Comment 7 Eunmi Lee 2012-09-18 19:38:11 PDT
(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.
Comment 8 Gyuyoung Kim 2012-09-18 19:54:06 PDT
(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.
Comment 9 Gyuyoung Kim 2012-09-18 19:54:27 PDT
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
Comment 10 Ryuan Choi 2012-09-18 20:21:17 PDT
(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.
Comment 11 Eunmi Lee 2012-09-18 20:24:57 PDT
(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 :)
Comment 12 Eunmi Lee 2012-09-18 20:25:44 PDT
(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.
Comment 13 Eunmi Lee 2012-09-18 20:26:41 PDT
(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 :)
Comment 14 Eunmi Lee 2012-09-19 02:33:03 PDT
Created attachment 164695 [details]
Patch
Comment 15 Gyuyoung Kim 2012-09-19 18:55:36 PDT
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.
Comment 16 Jinwoo Song 2012-09-19 20:11:39 PDT
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?
Comment 17 Eunmi Lee 2012-09-19 21:45:21 PDT
(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.
Comment 18 Eunmi Lee 2012-09-19 21:49:07 PDT
(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.
Comment 19 Eunmi Lee 2012-09-20 03:21:06 PDT
Created attachment 164878 [details]
Patch
Comment 20 Kenneth Rohde Christiansen 2012-09-20 03:23:58 PDT
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.
Comment 21 Kenneth Rohde Christiansen 2012-09-20 03:40:23 PDT
(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.
Comment 22 Kenneth Rohde Christiansen 2012-09-20 03:42:12 PDT
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)
Comment 23 Eunmi Lee 2012-09-20 04:14:51 PDT
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?
Comment 24 Kenneth Rohde Christiansen 2012-09-20 04:17:18 PDT
Comment on attachment 164878 [details]
Patch

Yes thanks please add such a test case!
Comment 25 Eunmi Lee 2012-09-20 05:07:42 PDT
Created attachment 164895 [details]
Patch
Comment 26 Eunmi Lee 2012-09-20 06:07:12 PDT
Created attachment 164905 [details]
Patch - rebased
Comment 27 Kenneth Rohde Christiansen 2012-09-20 06:22:48 PDT
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?
Comment 28 Eunmi Lee 2012-09-20 18:12:44 PDT
Created attachment 165028 [details]
Patch
Comment 29 Eunmi Lee 2012-09-20 18:13:36 PDT
(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 :)
Comment 30 Kenneth Rohde Christiansen 2012-09-20 23:08:56 PDT
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)
Comment 31 WebKit Review Bot 2012-09-20 23:23:13 PDT
Comment on attachment 165028 [details]
Patch

Clearing flags on attachment: 165028

Committed r129196: <http://trac.webkit.org/changeset/129196>
Comment 32 WebKit Review Bot 2012-09-20 23:23:18 PDT
All reviewed patches have been landed.  Closing bug.