If UIGestureRecognizer::Gesture called by touchStart event, it is always processed as single Tap Gesture. even though there are two or more touch points. So we should add a logic for checking multi touch in GestureRecognizer::noGesture
Created attachment 222308 [details] Patch
Created attachment 222310 [details] Patch
Comment on attachment 222310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222310&action=review Good point, but please consider my comments. > Source/WebKit2/ChangeLog:8 > + If UIGestureRecognizer::Gesture called by touchStart event, it is always UIGestureRecognizer? Is it mistake? > Source/WebKit2/ChangeLog:9 > + processed as single Tap Gesture. even though there are two or more touch single *t*ap *g*esture > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:359 > + { Interesting, style bot does not complain this. But, I think that below is correct. case kWKEventTypeTouchStart: { ... } > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:369 > + WKArrayRef touchPoints = WKTouchEventGetTouchPoints(eventRef); > + size_t numberOfTouchPoints = WKArrayGetSize(touchPoints); > + > + switch (numberOfTouchPoints) { > + case 1: > + break; > + case 2: > + m_recognizerFunction = &GestureRecognizer::pinchGesture; > + m_gestureHandler->handlePinchStarted(createVectorWithWKArray(touchPoints, 2)); > + return; How about 3 points? Is it fine? And, can you just check numberOfTouchPoints? if (WKArrayGetSize(WKTouchEventGetTouchPoints(eventRef)) > 1) ? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:375 > + } > + } > m_gestureHandler->reset(); > Please include whole kWKEventTypeTouchStart case into the block.
Created attachment 222315 [details] Patch
Comment on attachment 222315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222315&action=review > Source/WebKit2/ChangeLog:3 > + [EFL] Add a logic for checking multi touch in GestureRecognizer::noGesture Add [WK2] keyword in the title. > Source/WebKit2/ChangeLog:8 > + If GestureRecognizer::noGesture called by touchStart event, it is always It seems that you can improve the description more clearly. When processing TouchStart event in GestureRecognizer::noGesture(), we should check the number of touch points to distinguish if the gesture is single tap or pinch zoom. Current logic only considers the event as single tap.
Created attachment 222316 [details] Patch
Comment on attachment 222310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222310&action=review >> Source/WebKit2/ChangeLog:8 >> + If UIGestureRecognizer::Gesture called by touchStart event, it is always > > UIGestureRecognizer? Is it mistake? Yes. it is my mistake. I fixed. >> Source/WebKit2/ChangeLog:9 >> + processed as single Tap Gesture. even though there are two or more touch > > single *t*ap *g*esture ditto >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:369 >> + return; > > How about 3 points? Is it fine? > > And, can you just check numberOfTouchPoints? > if (WKArrayGetSize(WKTouchEventGetTouchPoints(eventRef)) > 1) ? I think we have to check three case. one point, two points, more than two points. In three or more points case, There are no proper gesture. So nothing to do.
Comment on attachment 222315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222315&action=review >> Source/WebKit2/ChangeLog:3 >> + [EFL] Add a logic for checking multi touch in GestureRecognizer::noGesture > > Add [WK2] keyword in the title. Thanks, I fixed. >> Source/WebKit2/ChangeLog:8 >> + If GestureRecognizer::noGesture called by touchStart event, it is always > > It seems that you can improve the description more clearly. > When processing TouchStart event in GestureRecognizer::noGesture(), we should check the number of touch points > to distinguish if the gesture is single tap or pinch zoom. Current logic only considers the event as single tap. ditto
Comment on attachment 222316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222316&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:359 > + switch (WKArrayGetSize(WKTouchEventGetTouchPoints(eventRef))) { You can make WKTouchEventGetTouchPoints() as a variable because it is used multiple times. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:361 > + break; I think it is better to move codes for case 1 here, instead of "break" for readability.
Created attachment 222383 [details] Patch
Comment on attachment 222316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222316&action=review >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:359 >> + switch (WKArrayGetSize(WKTouchEventGetTouchPoints(eventRef))) { > > You can make WKTouchEventGetTouchPoints() as a variable because it is used multiple times. Ok, I fixed. thanks >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:361 >> + break; > > I think it is better to move codes for case 1 here, instead of "break" for readability. ditto
Comment on attachment 222383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222383&action=review > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:360 > + Looks unneeded line. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:364 > + ditto. > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:374 > + return; Why do you use "return" instead of using "break" ? > Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:375 > + default: Please add ASSERT_NOT_REACHED() to here.
Comment on attachment 222383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222383&action=review >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:360 >> + > > Looks unneeded line. Ok. I'll remove this line. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:374 >> + return; > > Why do you use "return" instead of using "break" ? I'll change "return" to "break". thanks.
Created attachment 222391 [details] Patch
Comment on attachment 222391 [details] Patch LGTM, however, it would be good to get a final review from Eunmi.
Comment on attachment 222383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222383&action=review >>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:360 >>> + >> >> Looks unneeded line. > > Ok. I'll remove this line. Ok. I'll remove this line. >>> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:374 >>> + return; >> >> Why do you use "return" instead of using "break" ? > > I'll change "return" to "break". thanks. I'll change "return" to "break". thanks. >> Source/WebKit2/UIProcess/API/efl/GestureRecognizer.cpp:375 >> + default: > > Please add ASSERT_NOT_REACHED() to here. I think we have to remove ASSERT_NOT_REACHED(). Because it is possible that when noGesture() called, there are 3 touch points.
(In reply to comment #16) > I think we have to remove ASSERT_NOT_REACHED(). > Because it is possible that when noGesture() called, there are 3 touch points. If we need to handle 3 touch points as well, I prefer to add "case 3:". Then, you may add a notImplemented() or similar thing.
(In reply to comment #17) > (In reply to comment #16) > > I think we have to remove ASSERT_NOT_REACHED(). > > Because it is possible that when noGesture() called, there are 3 touch points. > > If we need to handle 3 touch points as well, I prefer to add "case 3:". Then, you may add a notImplemented() or similar thing. But four or more touch point also possible. So how about add a notImplemented() in "default:" case? I think "default:" can cover 3 or more touch point case.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > I think we have to remove ASSERT_NOT_REACHED(). > > > Because it is possible that when noGesture() called, there are 3 touch points. > > > > If we need to handle 3 touch points as well, I prefer to add "case 3:". Then, you may add a notImplemented() or similar thing. > > But four or more touch point also possible. > So how about add a notImplemented() in "default:" case? > I think "default:" can cover 3 or more touch point case. I see. If so, it would be good to add a comment as well for it.
Created attachment 222404 [details] Patch for landing
Created attachment 222951 [details] Patch
Comment on attachment 222951 [details] Patch Clearing flags on attachment: 222951 Committed r163284: <http://trac.webkit.org/changeset/163284>
All reviewed patches have been landed. Closing bug.