Bug 193687 – Long presses on YouTube links incorrectly trigger a load

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Cancel Web Touches Properly

[Megan Gardner]

Created attachment 359780 [details]
Patch

[Tim Horton]

Comment on attachment 359780 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Cancel Web Touches Properly

Can(cel) This Bug & Patch Have A Title That Describes The Symptom?

> Source/WebKit/ChangeLog:5
> +

And Probably A Radar #?

[Megan Gardner]

Created attachment 359786 [details]
Patch

[Tim Horton]

Comment on attachment 359786 [details]
Patch

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

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:761
> +- (void)cancel;

Was this here forever into the past, or do you need some ifdefs?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1354
> +- (void)_cancelWebGestureRecognizer
> +{
> +    [_touchEventGestureRecognizer cancel];
> +}

Why the indirection? Why "Web" gesture recognizer? Aren't they all technically about the web? :D I could see _cancelTouchEventsGestureRecognizer or something, but also think you just don't need this function.

[Megan Gardner]

Created attachment 359791 [details]
Patch

[Tim Horton]

Comment on attachment 359791 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1351
> +- (void)_cancelWebGestureRecognizer

Still not sure why the method. Are you thinking it will have more callers?
And my other complaint about the method name stills stands.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1353
> +#if HAVE(CANCEL_WEB_TOUCH_EVENTS_GESTURE)

I think the name I provided before was closer to the other names in e.g. Platform.h, but I guess this is OK.

[Megan Gardner]

Comment on attachment 359791 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1351
>> +- (void)_cancelWebGestureRecognizer
> 
> Still not sure why the method. Are you thinking it will have more callers?
> And my other complaint about the method name stills stands.

They are this is the Web Gesture in all other instances, because it is the gesture that passes the touches to the web. I think this helps clear up that is this the gesture we are talking about. 
And while there are no additional callers today, there might be in the future, so I'd like to encapsulate the need for feature defines. Also, several other gestures use this exact layout of code to do the exact same thing.

[Tim Horton]

Comment on attachment 359791 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1351
>>> +- (void)_cancelWebGestureRecognizer
>> 
>> Still not sure why the method. Are you thinking it will have more callers?
>> And my other complaint about the method name stills stands.
> 
> They are this is the Web Gesture in all other instances, because it is the gesture that passes the touches to the web. I think this helps clear up that is this the gesture we are talking about. 
> And while there are no additional callers today, there might be in the future, so I'd like to encapsulate the need for feature defines. Also, several other gestures use this exact layout of code to do the exact same thing.

I don't think it's ever called just "Web", that would be crazy. It is "WebTouchEvent" in UIKit, and "TouchEvent" in WebKit. You can decide what to call it.

[Megan Gardner]

Created attachment 359825 [details]
Patch for landing

[Megan Gardner]

<rdar://problem/47056717>

[WebKit Commit Bot]

Comment on attachment 359825 [details]
Patch for landing

Clearing flags on attachment: 359825

Committed r240316: <https://trac.webkit.org/changeset/240316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.