Bug 126424 – [EFL][WK2] Use timestamp when event occurs for touch events.

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

[Eunmi Lee]

We use current time for touch event's timestamp instead of timestamp when event occurs.
Timestamp of touch event is used to implement flick gesture, so it should be real value for accurate gesture.

[Eunmi Lee]

Created attachment 220291 [details]
Patch

[Ryuan Choi]

Comment on attachment 220291 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1337
> +void EwkView::handleTouchMouseDown(void* /* data */, Evas* /* canvas */, Evas_Object* ewkView, void* eventInfo)

How about handleTouchOrMouseDown ?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1339
> +    toEwkView(ewkView)->feedTouchEvents(EWK_TOUCH_START, static_cast<Evas_Event_Mouse_Down*>(eventInfo)->timestamp / 1000.0);

Should we divide timestamp here?

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:355
> +void GestureRecognizer::noGesture(WKEventType type, WKArrayRef touchPoints, double timestamp)

Can we give more correct information? seconds or milliseconds?

[Eunmi Lee]

Comment on attachment 220291 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1337
>> +void EwkView::handleTouchMouseDown(void* /* data */, Evas* /* canvas */, Evas_Object* ewkView, void* eventInfo)
> 
> How about handleTouchOrMouseDown ?

Handlers are for touch, so it is better to rename to handleMouseDownForTouch.

I will change the name.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1339
>> +    toEwkView(ewkView)->feedTouchEvents(EWK_TOUCH_START, static_cast<Evas_Event_Mouse_Down*>(eventInfo)->timestamp / 1000.0);
> 
> Should we divide timestamp here?

Timestamp in the webkit is usually double type and it's unit is second.
So, I think it is better to divide evas event's timestamp (millisecond) with 1000.0 as early as possible.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:355
>> +void GestureRecognizer::noGesture(WKEventType type, WKArrayRef touchPoints, double timestamp)
> 
> Can we give more correct information? seconds or milliseconds?

Unit of most timestamp in the webkit is second, so I think we can use timestamp here.

[Eunmi Lee]

Created attachment 220292 [details]
Patch

Rename touch handlers.

[Ryuan Choi]

(In reply to comment #3)
> (From update of attachment 220291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220291&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1337
> >> +void EwkView::handleTouchMouseDown(void* /* data */, Evas* /* canvas */, Evas_Object* ewkView, void* eventInfo)
> > 
> > How about handleTouchOrMouseDown ?
> 
> Handlers are for touch, so it is better to rename to handleMouseDownForTouch.
> 
> I will change the name.
> 
OK.

> >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1339
> >> +    toEwkView(ewkView)->feedTouchEvents(EWK_TOUCH_START, static_cast<Evas_Event_Mouse_Down*>(eventInfo)->timestamp / 1000.0);
> > 
> > Should we divide timestamp here?
> 
> Timestamp in the webkit is usually double type and it's unit is second.
> So, I think it is better to divide evas event's timestamp (millisecond) with 1000.0 as early as possible.
> 
OK.

I preferred lazy calculation because we can have only one division operator but it's minor issue.

I don't have objection.

> >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:355
> >> +void GestureRecognizer::noGesture(WKEventType type, WKArrayRef touchPoints, double timestamp)
> > 
> > Can we give more correct information? seconds or milliseconds?
> 
> Unit of most timestamp in the webkit is second, so I think we can use timestamp here.

OK.

[Gyuyoung Kim]

Comment on attachment 220292 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1352
> +void EwkView::handleMultiDownForTouch(void* /* data */, Evas* /* canvas */, Evas_Object* ewkView, void* eventInfo)

It looks code is a little messed up

/* data */ -> /*data*/ ,or how about using UNUSED_PARAM() ?

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:412
> +void GestureRecognizer::doubleTapGesture(WKEventType type, WKArrayRef touchPoints, double timestamp)

Don't you need to use UNUSED_PARAM for timestamp ?

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:469
> +void GestureRecognizer::pinchGesture(WKEventType type, WKArrayRef touchPoints, double timestamp)

ditto.

> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:59
> +    void noGesture(WKEventType, WKArrayRef, double timestamp);

Can't we use set|get functions instead of passing a timestamp variable ?

[Eunmi Lee]

Comment on attachment 220292 [details]
Patch

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

Thanks for review :)

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1352
>> +void EwkView::handleMultiDownForTouch(void* /* data */, Evas* /* canvas */, Evas_Object* ewkView, void* eventInfo)
> 
> It looks code is a little messed up
> 
> /* data */ -> /*data*/ ,or how about using UNUSED_PARAM() ?

I think it is better to remove the commented parameter name because it is not used.
I will modify that.

>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.h:59
>> +    void noGesture(WKEventType, WKArrayRef, double timestamp);
> 
> Can't we use set|get functions instead of passing a timestamp variable ?

If we use WKTouchEventRef as a parameter, we don't have to pass a timestamp.
I will modify that.

[Eunmi Lee]

Created attachment 221322 [details]
Patch

Fix for Gyuyoung's comments.

[Gyuyoung Kim]

Comment on attachment 221322 [details]
Patch

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

Looks better than before. r=me together with Ryuan informal review.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:781
> +        evas_object_event_callback_add(m_evasObject, EVAS_CALLBACK_MOUSE_DOWN, handleMouseDownForTouch, sd);

This function name change isn't mentioned in ChangeLog. It would be nicer if you mention this change as well.

[Eunmi Lee]

Created attachment 221325 [details]
Patch

Update changelog.

[Eunmi Lee]

Comment on attachment 221325 [details]
Patch

Clearing flags on attachment: 221325

Committed r162111: <http://trac.webkit.org/changeset/162111>

[Eunmi Lee]

All reviewed patches have been landed.  Closing bug.