Bug 213038 – WebKit: Make UIGestureRecognizer build for watchOS and tvOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jonathan Bedard]

UIGestureRecognizer SPI is guarded by an iOS macro, but we need the definition for watchOS and tvOS.

[Radar WebKit Bug Importer]

<rdar://problem/64217654>

[Jonathan Bedard]

Created attachment 401561 [details]
Patch

[Jonathan Bedard]

Comment on attachment 401561 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1457
> +    return [recognizer respondsToSelector:@selector(modifierFlags)] ? [recognizer modifierFlags] : 0;

This gets watchOS and tvOS building, but I'm pretty sure it's wrong.

[Darin Adler]

Comment on attachment 401561 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1457
>> +    return [recognizer respondsToSelector:@selector(modifierFlags)] ? [recognizer modifierFlags] : 0;
> 
> This gets watchOS and tvOS building, but I'm pretty sure it's wrong.

Let's not commit something that we are pretty sure is wrong.

[Tim Horton]

Comment on attachment 401561 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1457
>>> +    return [recognizer respondsToSelector:@selector(modifierFlags)] ? [recognizer modifierFlags] : 0;
>> 
>> This gets watchOS and tvOS building, but I'm pretty sure it's wrong.
> 
> Let's not commit something that we are pretty sure is wrong.

Jonathan, don't you just want to make UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS be true on watchOS and tvOS?

[Tim Horton]

(In reply to Tim Horton from comment #5)
> Comment on attachment 401561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401561&action=review
> 
> >>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1457
> >>> +    return [recognizer respondsToSelector:@selector(modifierFlags)] ? [recognizer modifierFlags] : 0;
> >> 
> >> This gets watchOS and tvOS building, but I'm pretty sure it's wrong.
> > 
> > Let's not commit something that we are pretty sure is wrong.
> 
> Jonathan, don't you just want to make UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS
> be true on watchOS and tvOS?

err, HAVE()

[Jonathan Bedard]

Comment on attachment 401561 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1457
>>>>> +    return [recognizer respondsToSelector:@selector(modifierFlags)] ? [recognizer modifierFlags] : 0;
>>>> 
>>>> This gets watchOS and tvOS building, but I'm pretty sure it's wrong.
>>> 
>>> Let's not commit something that we are pretty sure is wrong.
>> 
>> Jonathan, don't you just want to make UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS be true on watchOS and tvOS?
> 
> err, HAVE()

Maybe? Shouldn't the definition of modifierFlags also be guarded by HAVE(UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS), in that case?

I'm a bit confused by the original content of this line in the first place. It seems like it would only compile if the recognizer had a _modifierFlags member variable, which it doesn't seem to have for watchOS. I would expect this code to read:

#if HAVE(UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS)
    return recognizer.modifierFlags;
#else
    return 0;
#endif

[Tim Horton]

(In reply to Jonathan Bedard from comment #7)
> Comment on attachment 401561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401561&action=review
> 
> >>>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1457
> >>>>> +    return [recognizer respondsToSelector:@selector(modifierFlags)] ? [recognizer modifierFlags] : 0;
> >>>> 
> >>>> This gets watchOS and tvOS building, but I'm pretty sure it's wrong.
> >>> 
> >>> Let's not commit something that we are pretty sure is wrong.
> >> 
> >> Jonathan, don't you just want to make UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS be true on watchOS and tvOS?
> > 
> > err, HAVE()
> 
> Maybe? Shouldn't the definition of modifierFlags also be guarded by
> HAVE(UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS), in that case?
> 
> I'm a bit confused by the original content of this line in the first place.
> It seems like it would only compile if the recognizer had a _modifierFlags
> member variable, which it doesn't seem to have for watchOS. I would expect
> this code to read:
> 
> #if HAVE(UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS)
>     return recognizer.modifierFlags;
> #else
>     return 0;
> #endif

1) That's a method, not a member variable.
2) This is all because _modifierFlags used to exist on iOS as SPI and then became API (without the underscore) in iOS 13.4, it's got nothing to do with watchOS/etc., but your substitution is definitely wrong.
3) The header suggests that the watchOS 6 SDK has both the SPI and API versions available. So I think my suggestion is probably the right path forward, but you have alternative paths available if necessary.

[Jonathan Bedard]

Created attachment 401642 [details]
Patch

[Jonathan Bedard]

Created attachment 401643 [details]
Patch

[Tim Horton]

Comment on attachment 401643 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:450
> +#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130400)

As previously mentioned, a IOS_FAMILY + IPHONE_OS_VERSION* check is always wrong.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:422
> -#if PLATFORM(IOS) && !defined(__IPHONE_13_4)
> +#if !HAVE(UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS)

This used defined(__IPHONE_13_4) instead of MIN_REQUIRED for a reason! Blame it and see.

[Tim Horton]

Comment on attachment 401643 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:450
> +#if (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130400)

As previously mentioned, a IOS_FAMILY + IPHONE_OS_VERSION* check is always wrong.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:422
> -#if PLATFORM(IOS) && !defined(__IPHONE_13_4)
> +#if !HAVE(UI_GESTURE_RECOGNIZER_MODIFIER_FLAGS)

This used defined(__IPHONE_13_4) instead of MIN_REQUIRED for a reason! Blame it and see.

[Jonathan Bedard]

Created attachment 401668 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #13)
> Created attachment 401668 [details]
> Patch

I think I was trying way too hard.

Seems that we don't need the SPI header at all. And actually, do we still need the SPI declaration for iOS once we move to 13.5, the old macros only work on 13.4, right?

[Darin Adler]

Comment on attachment 401668 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:450
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130400 || PLATFORM(WATCHOS) || PLATFORM(APPLETV))

Seems like we could either remove the () entirely, or put them just around the && section.

[Jonathan Bedard]

Created attachment 401677 [details]
Patch for landing

[EWS]

Committed r262924: <https://trac.webkit.org/changeset/262924>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401677 [details].