Bug 127675 – [EFL][WK2] Add a logic for checking multi touch in GestureRecognizer::noGesture

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Sanghyup Lee]

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

[Sanghyup Lee]

Created attachment 222308 [details]
Patch

[Sanghyup Lee]

Created attachment 222310 [details]
Patch

[Ryuan Choi]

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.

[Sanghyup Lee]

Created attachment 222315 [details]
Patch

[Jinwoo Song]

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.

[Sanghyup Lee]

Created attachment 222316 [details]
Patch

[Sanghyup Lee]

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.

[Sanghyup Lee]

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

[Eunmi Lee]

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.

[Sanghyup Lee]

Created attachment 222383 [details]
Patch

[Sanghyup Lee]

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

[Gyuyoung Kim]

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.

[Sanghyup Lee]

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.

[Sanghyup Lee]

Created attachment 222391 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 222391 [details]
Patch

LGTM, however, it would be good to get a final review from Eunmi.

[Sanghyup Lee]

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.

[Gyuyoung Kim]

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

[Sanghyup Lee]

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

[Gyuyoung Kim]

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

[Sanghyup Lee]

Created attachment 222404 [details]
Patch for landing

[Sanghyup Lee]

Created attachment 222951 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 222951 [details]
Patch

Clearing flags on attachment: 222951

Committed r163284: <http://trac.webkit.org/changeset/163284>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.