Bug 126424 - [EFL][WK2] Use timestamp when event occurs for touch events.
Summary: [EFL][WK2] Use timestamp when event occurs for touch events.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eunmi Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-03 00:52 PST by Eunmi Lee
Modified: 2014-01-15 20:25 PST (History)
8 users (show)

See Also:


Attachments
Patch (14.28 KB, patch)
2014-01-03 00:59 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (14.36 KB, patch)
2014-01-03 01:35 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (15.19 KB, patch)
2014-01-15 18:21 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (15.30 KB, patch)
2014-01-15 20:21 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2014-01-03 00:52:32 PST
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.
Comment 1 Eunmi Lee 2014-01-03 00:59:36 PST
Created attachment 220291 [details]
Patch
Comment 2 Ryuan Choi 2014-01-03 01:10:00 PST
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?
Comment 3 Eunmi Lee 2014-01-03 01:26:21 PST
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.
Comment 4 Eunmi Lee 2014-01-03 01:35:15 PST
Created attachment 220292 [details]
Patch

Rename touch handlers.
Comment 5 Ryuan Choi 2014-01-03 03:32:33 PST
(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.
Comment 6 Gyuyoung Kim 2014-01-13 19:59:50 PST
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 ?
Comment 7 Eunmi Lee 2014-01-13 23:05:48 PST
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.
Comment 8 Eunmi Lee 2014-01-15 18:21:27 PST
Created attachment 221322 [details]
Patch

Fix for Gyuyoung's comments.
Comment 9 Gyuyoung Kim 2014-01-15 20:02:14 PST
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.
Comment 10 Eunmi Lee 2014-01-15 20:21:43 PST
Created attachment 221325 [details]
Patch

Update changelog.
Comment 11 Eunmi Lee 2014-01-15 20:24:58 PST
Comment on attachment 221325 [details]
Patch

Clearing flags on attachment: 221325

Committed r162111: <http://trac.webkit.org/changeset/162111>
Comment 12 Eunmi Lee 2014-01-15 20:25:08 PST
All reviewed patches have been landed.  Closing bug.