[EFL][WK2] Process touch events using mouse and multi events of Evas.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Eunmi Lee]

The Evas creates mouse and multi event on the touch device when touching the screen.
The mouse event occurs for first finger and multi event occurs for second and third finger,
so we can process touch event using them.

[Eunmi Lee]

Created attachment 165039 [details]
Patch

[Eunmi Lee]

Created attachment 165328 [details]
Patch

[Mikhail Pozdnyakov]

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.

[Chris Dumez]

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

[Gyuyoung Kim]

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.

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 165536 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Eunmi Lee]

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.

[Laszlo Gombos]

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

[Gyuyoung Kim]

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.

[Eunmi Lee]

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

[Eunmi Lee]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 165997 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165997 [details]
Patch

Clearing flags on attachment: 165997

Committed r129765: <http://trac.webkit.org/changeset/129765>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.