Summary: | [EFL][WK2] Use timestamp when event occurs for touch events. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eunmi Lee <enmi.lee> | ||||||||||
Component: | WebKit EFL | Assignee: | Eunmi Lee <enmi.lee> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Eunmi Lee
2014-01-03 00:52:32 PST
Created attachment 220291 [details]
Patch
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 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. Created attachment 220292 [details]
Patch
Rename touch handlers.
(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 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 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. Created attachment 221322 [details]
Patch
Fix for Gyuyoung's comments.
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. Created attachment 221325 [details]
Patch
Update changelog.
Comment on attachment 221325 [details] Patch Clearing flags on attachment: 221325 Committed r162111: <http://trac.webkit.org/changeset/162111> All reviewed patches have been landed. Closing bug. |