Bug 96906

Created attachment 165039 [details] Patch

Created attachment 165328 [details] Patch

Comment on attachment 165328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165328&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:366 > +static Eina_List* _ewk_view_touch_point_list_get_from_evas(Evas_Object* ewkView) static inline ? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:375 > + point = static_cast<Ewk_Touch_Point*>(calloc(1, sizeof(Ewk_Touch_Point))); use 'new' please > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394 > + free(point); Coding style forbids using 'free', pls. use 'delete' instead > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:406 > + free(point); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:410 > +{ All 3 functions _ewk_view_on_touch_(up, down, move) seem to do the same except passing of different Ewk_Touch_Event_Type argument for ewk_view_feed_touch_event, so think it's worth creating one common aux function to be invoked from each of them.

Comment on attachment 165328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165328&action=review > Source/WebKit2/ChangeLog:8 > + Provide default behaivor for processing touch event in the ewk_view if "behavior", "touch events" > Source/WebKit2/ChangeLog:10 > + We can to process touch event using mouse and multi event because the "touch events" >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:366 >> +static Eina_List* _ewk_view_touch_point_list_get_from_evas(Evas_Object* ewkView) > > static inline ? const Evas_Object* ? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:372 > + int count = evas_touch_point_list_count(smartData->base.evas); unsigned count = ...; // evas_touch_point_list_count() returns an unsigned integer according to its doc. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:373 > + Ewk_Touch_Point* point; In wrong scope, please define point variable inside the for loop. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:410 >> +{ > > All 3 functions _ewk_view_on_touch_(up, down, move) seem to do the same except passing of different Ewk_Touch_Event_Type argument for ewk_view_feed_touch_event, so think it's worth creating one common aux function to be invoked from each of them. Completely agree with Mikhail here. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:714 > + * The ewk_view will support the touch events if EINA_TRUE or not support the @c ? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:715 > + * touch events otherwise. The default value is EINA_TRUE. @c ? You say here that the default value is true but you actually set if to false in the constructor. Am I missing something? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:752 > +TEST_F(EWK2UnitTestBase, ewk_view_touch_events_enabled) Is there any way we can test that enabling touch events actually works? e.g. check the view actually gets touch events if enabled? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:754 > + ASSERT_TRUE(ewk_view_touch_events_enabled_set(webView(), EINA_TRUE)); Before setting the value, you should test the default value returned by ewk_view_touch_events_enabled_get().

Comment on attachment 165328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165328&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:374 > + for (int i = 0; i < count; ++i) { Use unsigned instead of int. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1857 > + // FIXME: We have to connect touch callbacks with Mouse and Multi event s/Mouse/mouse/g, s/Multi/multi/g >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:754 >> + ASSERT_TRUE(ewk_view_touch_events_enabled_set(webView(), EINA_TRUE)); > > Before setting the value, you should test the default value returned by ewk_view_touch_events_enabled_get(). Use standard boolean, in webkit-efl mailing list, we almost decided to use standard boolean for unit test.

Comment on attachment 165328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165328&action=review >> Source/WebKit2/ChangeLog:8 >> + Provide default behaivor for processing touch event in the ewk_view if > > "behavior", "touch events" oops, I will fix them. >> Source/WebKit2/ChangeLog:10 >> + We can to process touch event using mouse and multi event because the > > "touch events" ditto >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:366 >>> +static Eina_List* _ewk_view_touch_point_list_get_from_evas(Evas_Object* ewkView) >> >> static inline ? > > const Evas_Object* ? OK, I will fix for them. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:376 > + int count = evas_touch_point_list_count(smartData->base.evas); > + Ewk_Touch_Point* point; > + for (int i = 0; i < count; ++i) { > + point = static_cast<Ewk_Touch_Point*>(calloc(1, sizeof(Ewk_Touch_Point))); > + point->id = evas_touch_point_list_nth_id_get(smartData->base.evas, i); Thanks for your comments. I will fix for them. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394 >> + free(point); > > Coding style forbids using 'free', pls. use 'delete' instead ok >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:406 >> + free(point); > > ditto. ok >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:410 >>> +{ >> >> All 3 functions _ewk_view_on_touch_(up, down, move) seem to do the same except passing of different Ewk_Touch_Event_Type argument for ewk_view_feed_touch_event, so think it's worth creating one common aux function to be invoked from each of them. > > Completely agree with Mikhail here. ok, I will change to reduce duplicated codes. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1857 >> + // FIXME: We have to connect touch callbacks with Mouse and Multi event > > s/Mouse/mouse/g, s/Multi/multi/g ok >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:715 >> + * touch events otherwise. The default value is EINA_TRUE. > > @c ? > You say here that the default value is true but you actually set if to false in the constructor. Am I missing something? oops, it is my mistake. the default value is EINA_FALSE. I will fix it. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:752 >> +TEST_F(EWK2UnitTestBase, ewk_view_touch_events_enabled) > > Is there any way we can test that enabling touch events actually works? e.g. check the view actually gets touch events if enabled? ok, I will prepare test codes to check that touch events really work. >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:754 >>> + ASSERT_TRUE(ewk_view_touch_events_enabled_set(webView(), EINA_TRUE)); >> >> Before setting the value, you should test the default value returned by ewk_view_touch_events_enabled_get(). > > Use standard boolean, in webkit-efl mailing list, we almost decided to use standard boolean for unit test. ok, I will fix.

Created attachment 165536 [details] Patch

Comment on attachment 165536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1829 > +Eina_Bool ewk_view_touch_events_enabled_set(Evas_Object* ewkView, Eina_Bool enabled) any reason why this cannot always be one? like why would you ever turn touch support off? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 > + // FIXME: We have to connect touch callbacks with mouse and multi events > + // because the Evas creates mouse events for first touch and multi events > + // for second and third touches. Below codes should be fixed when the Evas > + // supports the touch events. Oh really? EFL seems so broken. So noone can use mouse and touch together... just great

Comment on attachment 165536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1829 >> +Eina_Bool ewk_view_touch_events_enabled_set(Evas_Object* ewkView, Eina_Bool enabled) > > any reason why this cannot always be one? like why would you ever turn touch support off? If I turn on the touch events, touch events will occur when we use mouse in the desktop because we generates touch event using Evas's mouse events. That means both touch and mouse events occur for one event from mouse device. I don't think that is right operation. so, I hope that enabling mouse events and disabling touch events for desktop environment (with mouse device), and disabling mouse events and enabling touch events for touch device. >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 >> + // supports the touch events. > > Oh really? EFL seems so broken. So noone can use mouse and touch together... just great Yes, I hope that EFL supports touch events later.

(In reply to comment #9) > > Oh really? EFL seems so broken. So noone can use mouse and touch together... just great > > Yes, I hope that EFL supports touch events later. Is there a bugID/issueID for EFL where this could be tracked ?

Comment on attachment 165536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 >>> + // supports the touch events. >> >> Oh really? EFL seems so broken. So noone can use mouse and touch together... just great > > Yes, I hope that EFL supports touch events later. As Gombos said, I think it is better to write bug url you create for this as well.

(In reply to comment #11) > (From update of attachment 165536 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 > >>> + // supports the touch events. > >> > >> Oh really? EFL seems so broken. So noone can use mouse and touch together... just great > > > > Yes, I hope that EFL supports touch events later. > > As Gombos said, I think it is better to write bug url you create for this as well. unfortunately, we don't have any bug or issue id for that in the EFL.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 165536 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review > > > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 > > >>> + // supports the touch events. > > >> > > >> Oh really? EFL seems so broken. So noone can use mouse and touch together... just great > > > > > > Yes, I hope that EFL supports touch events later. > > > > As Gombos said, I think it is better to write bug url you create for this as well. > > unfortunately, we don't have any bug or issue id for that in the EFL. Even though there is no bug id to track in the EFL, I will create new bug in the WebKit to record this issue.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 165536 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review > > > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 > > >>> + // supports the touch events. > > >> > > >> Oh really? EFL seems so broken. So noone can use mouse and touch together... just great > > > > > > Yes, I hope that EFL supports touch events later. > > > > As Gombos said, I think it is better to write bug url you create for this as well. > > unfortunately, we don't have any bug or issue id for that in the EFL. FWIW, anyone is free to create a ticket in <http://trac.enlightenment.org/e/newticket> :-)

(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 165536 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=165536&action=review > > > > > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1844 > > > >>> + // supports the touch events. > > > >> > > > >> Oh really? EFL seems so broken. So noone can use mouse and touch together... just great > > > > > > > > Yes, I hope that EFL supports touch events later. > > > > > > As Gombos said, I think it is better to write bug url you create for this as well. > > > > unfortunately, we don't have any bug or issue id for that in the EFL. > > FWIW, anyone is free to create a ticket in <http://trac.enlightenment.org/e/newticket> :-) Thanks, I will discuss in the efl mailinglist and then create new ticket. :)

Created attachment 165997 [details] Patch

Comment on attachment 165997 [details] Patch Clearing flags on attachment: 165997 Committed r129765: <http://trac.webkit.org/changeset/129765>

All reviewed patches have been landed. Closing bug.