Bug 96903

Created attachment 164364 [details] Patch

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

Created attachment 164517 [details] Patch

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

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.

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

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

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

(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 :)

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

(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 :)

Created attachment 164695 [details] Patch

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

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

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

Created attachment 164878 [details] Patch

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.

(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 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 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 on attachment 164878 [details] Patch Yes thanks please add such a test case!

Created attachment 164895 [details] Patch

Created attachment 164905 [details] Patch - rebased

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?

Created attachment 165028 [details] Patch

(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 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 on attachment 165028 [details] Patch Clearing flags on attachment: 165028 Committed r129196: <http://trac.webkit.org/changeset/129196>

All reviewed patches have been landed. Closing bug.