Bug 135945 – [iOS] Disable ENABLE_IOS_{GESTURE, TOUCH}_EVENTS, and temporarily disable ENABLE_TOUCH_EVENTS and ENABLE_XSLT when building with the iOS public SDK

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

We should disable feature defines ENABLE_IOS_{GESTURE, TOUCH}_EVENTS when building the iOS port using the iOS public SDK. For now, we should also temporarily disable feature defines ENABLE_TOUCH_EVENTS and ENABLE_XSLT while we focus to get the rest of the port built using the public SDK. We'll look to enable ENABLE_TOUCH_EVENTS and ENABLE_XSLT once we stabilize the build.

[Daniel Bates]

Created attachment 236606 [details]
Patch

[Andy Estes]

Comment on attachment 236606 [details]
Patch

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

r=me, but it seems like this patch has unnecessary changes around -webkit-tap-highlight-color code.

> Source/WTF/wtf/FeatureDefines.h:97
> +#if !defined(ENABLE_IOS_GESTURE_EVENTS) && defined(__has_include) && __has_include(<WebKitAdditions/GestureEvent.h>)

No need for the defined(__has_include) check since this only applies to iOS.

> Source/WTF/wtf/FeatureDefines.h:105
> +#if !defined(ENABLE_IOS_TOUCH_EVENTS) && defined(__has_include) && __has_include(<WebKitAdditions/TouchIOS.h>)

Ditto.

> Source/WTF/wtf/FeatureDefines.h:145
> +#if !defined(ENABLE_TOUCH_EVENTS) && defined(__has_include) && __has_include(<WebKitAdditions/TouchIOS.h>)

Ditto.

> Source/WTF/wtf/FeatureDefines.h:166
> +#if !defined(__has_include) || !__has_include(<WebKitAdditions/TouchIOS.h>)

No need for the defined() check. Also, is there not an XSLT header you can check for? Doesn't really make sense to gate XSLT support on the presence of WebKitAdditions.

> Source/WebCore/bindings/objc/DOMEvents.h:44
> -#if TARGET_OS_IPHONE
> +#if defined(ENABLE_IOS_GESTURE_EVENTS) && ENABLE_IOS_GESTURE_EVENTS

This is an API header. ENABLE_IOS_GESTURE_EVENTS could be undefined even when the internal SDK is present. Why not just check if the header was generated by the derived sources build phase?

> Source/WebCore/bindings/objc/DOMEvents.h:48
> +#if defined(ENABLE_IOS_TOUCH_EVENTS) && ENABLE_IOS_TOUCH_EVENTS

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:365
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

Why is this now specific to iOS touch events? Weren't other ports with ENABLE(TOUCH_EVENTS) building this?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2739
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/css/CSSParser.cpp:2773
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/css/CSSPropertyNames.in:459
> +#if defined(ENABLE_IOS_TOUCH_EVENTS) && ENABLE_IOS_TOUCH_EVENTS

Ditto.

> Source/WebCore/css/StyleResolver.cpp:2550
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/editing/EditingStyle.cpp:76
> -#if PLATFORM(IOS)
> +#if ENABLE(IOS_TOUCH_EVENTS)
>      CSSPropertyWebkitTapHighlightColor,
> +#endif

I think this answers by question about why you changed ENABLE(TOUCH_EVENTS) to ENABLE(IOS_TOUCH_EVENTS). It looks like you concluded that -webkit-tap-highlight-color code should have always been iOS-only. But that change doesn't seem necessary for this patch.

> Source/WebCore/history/CachedFrame.cpp:53
> -#if ENABLE(TOUCH_EVENTS)
> +#if PLATFORM(IOS) || ENABLE(TOUCH_EVENTS)

Why does iOS need this even with touch events disabled?

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.h:35
> +#if ENABLE(IOS_TOUCH_EVENTS)
>  #include <WebKitAdditions/PlatformTouchEventIOS.h>
> +#endif

Why not just check if the header exists? Although it doesn't matter in this case, you technically shouldn't use ENABLE() in API headers.

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.h:46
> +#if ENABLE(IOS_TOUCH_EVENTS)

Won't this be wrong if iOS enables TOUCH_EVENTS (but not IOS_TOUCH_EVENTS)?

> Source/WebCore/rendering/style/RenderStyle.h:1099
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

See above comments about -webkit-tap-highlight-color.

> Source/WebCore/rendering/style/RenderStyle.h:1630
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:133
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:217
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:259
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:156
> -#if ENABLE(TOUCH_EVENTS)
> +#if ENABLE(IOS_TOUCH_EVENTS)

Ditto.

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 236606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236606&action=review
> 
> r=me, but it seems like this patch has unnecessary changes around -webkit-tap-highlight-color code.
> 

Will revert the changes I made in the patch with regards to the -webkit-tap-highlight-color code.

> > Source/WTF/wtf/FeatureDefines.h:97
> > +#if !defined(ENABLE_IOS_GESTURE_EVENTS) && defined(__has_include) && __has_include(<WebKitAdditions/GestureEvent.h>)
> 
> No need for the defined(__has_include) check since this only applies to iOS.
>  

Will remove defined(__has_include) conjunct.

> > Source/WTF/wtf/FeatureDefines.h:105
> > +#if !defined(ENABLE_IOS_TOUCH_EVENTS) && defined(__has_include) && __has_include(<WebKitAdditions/TouchIOS.h>)
> 
> Ditto.
> 

Will remove defined(__has_include) conjunct.

> > Source/WTF/wtf/FeatureDefines.h:145
> > +#if !defined(ENABLE_TOUCH_EVENTS) && defined(__has_include) && __has_include(<WebKitAdditions/TouchIOS.h>)
> 
> Ditto.
> 

Will remove defined(__has_include) conjunct.

> > Source/WTF/wtf/FeatureDefines.h:166
> > +#if !defined(__has_include) || !__has_include(<WebKitAdditions/TouchIOS.h>)
> 
> No need for the defined() check.

Will remove defined(__has_include) conjunct.

> Also, is there not an XSLT header you can check for? Doesn't really make sense to gate XSLT support on the presence of WebKitAdditions.
> 

Will change to check for header <libxslt/xslt.h>.

> > Source/WebCore/bindings/objc/DOMEvents.h:44
> > -#if TARGET_OS_IPHONE
> > +#if defined(ENABLE_IOS_GESTURE_EVENTS) && ENABLE_IOS_GESTURE_EVENTS
> 
> This is an API header. ENABLE_IOS_GESTURE_EVENTS could be undefined even when the internal SDK is present. Why not just check if the header was generated by the derived sources build phase?
>

Will change to check for header <WebCore/DOMGestureEvent.h>.

> > Source/WebCore/bindings/objc/DOMEvents.h:48
> > +#if defined(ENABLE_IOS_TOUCH_EVENTS) && ENABLE_IOS_TOUCH_EVENTS
> 
> Ditto.
> 

Will change to check for header <WebCore/DOMTouchEvent.h>. 

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:365
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Why is this now specific to iOS touch events? Weren't other ports with ENABLE(TOUCH_EVENTS) building this?
>

Will revert.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2739
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert.

> > Source/WebCore/css/CSSParser.cpp:2773
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert.

> > Source/WebCore/css/CSSPropertyNames.in:459
> > +#if defined(ENABLE_IOS_TOUCH_EVENTS) && ENABLE_IOS_TOUCH_EVENTS
> 
> Ditto.
> 

Will revert.

> > Source/WebCore/css/StyleResolver.cpp:2550
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert.


> > Source/WebCore/editing/EditingStyle.cpp:76
> > -#if PLATFORM(IOS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> >      CSSPropertyWebkitTapHighlightColor,
> > +#endif
> 
> I think this answers by question about why you changed ENABLE(TOUCH_EVENTS) to ENABLE(IOS_TOUCH_EVENTS). It looks like you concluded that -webkit-tap-highlight-color code should have always been iOS-only. But that change doesn't seem necessary for this patch.
> 

Will revert.

> > Source/WebCore/history/CachedFrame.cpp:53
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if PLATFORM(IOS) || ENABLE(TOUCH_EVENTS)
> 
> Why does iOS need this even with touch events disabled?
> 

We have some PLATFORM(iOS)-guard logic for requesting scroll notifications from the client among other things.

> > Source/WebCore/platform/ios/PlatformEventFactoryIOS.h:35
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> >  #include <WebKitAdditions/PlatformTouchEventIOS.h>
> > +#endif
> 
> Why not just check if the header exists? Although it doesn't matter in this case, you technically shouldn't use ENABLE() in API headers.
> 

Will change to check for header <WebKitAdditions/PlatformTouchEventIOS.h>.

> > Source/WebCore/platform/ios/PlatformEventFactoryIOS.h:46
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Won't this be wrong if iOS enables TOUCH_EVENTS (but not IOS_TOUCH_EVENTS)?
> 

You're right! This function will most likely be useful when building with either TOUCH_EVENTS or IOS_TOUCH_EVENTS enabled as a way to convert a platform touch event to a platform-independent touch event. I'll change the condition of the if-statement to "ENABLE(TOUCH_EVENTS) || ENABLE(IOS_TOUCH_EVENTS)". We'll eventually need to provide a ENABLE(TOUCH_EVENTS) && !ENABLE(IOS_TOUCH_EVENTS)-guarded implementation of this function when we support such a build configuration.

> > Source/WebCore/rendering/style/RenderStyle.h:1099
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> See above comments about -webkit-tap-highlight-color.
> 

Will revert this change.

> > Source/WebCore/rendering/style/RenderStyle.h:1630
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert this change.

> > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:133
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert this change.

> > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:217
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert this change.

> > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:259
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.
> 

Will revert this change.

> > Source/WebCore/rendering/style/StyleRareInheritedData.h:156
> > -#if ENABLE(TOUCH_EVENTS)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> 
> Ditto.

Will revert this change.

[Andy Estes]

Probably goes without saying, but you might want to post another patch to verify that the EWS bots are happy before you land.

[Daniel Bates]

Created attachment 236899 [details]
Patch

Updated patch. I chose to use USE(APPLE_INTERNAL_SDK) instead of checking for the existence of a specific header file when defining ENABLE_IOS_GESTURE_EVENTS, ENABLE_IOS_TOUCH_EVENTS, and ENABLE_XSLT in file Source/WTF/wtf/FeatureDefines.h so as to be materially consistent with the xcconfig logic in this patch for the same feature defines. Let me know if it's preferred to check for a header file when defining these feature defines.

[Andy Estes]

Comment on attachment 236899 [details]
Patch

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

Yeah, I agree with your use of USE(APPLE_INTERNAL_SDK) over has_include checks. r=me.

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:1258
> -#if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE
> +/* FIXME: We should explicitly include this header when generating bindings for iOS with the internal SDK
> +instead of including it if the file exists so that we error out if the file doesn't exist when building
> +for iOS with the internal iOS SDK. */
> +#if __has_include(<WebKitAdditions/PublicDOMInterfacesIOS.h>)

You could also have CodeGeneratorObjC.pm add something like -DUSE_APPLE_INTERNAL_SDK to the compiler invocation that preprocesses this file if ${USE_INTERNAL_SDK} is YES, then check here if USE_APPLE_INTERNAL_SDK is defined.

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.h:35
> +#if __has_include(<WebKitAdditions/PlatformTouchEventIOS.h>)
>  #include <WebKitAdditions/PlatformTouchEventIOS.h>
> +#endif

Can this be a USE(APPLE_INTERNAL_SDK) check instead?

[Daniel Bates]

(In reply to comment #6)
> (From update of attachment 236899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236899&action=review
> 
> Yeah, I agree with your use of USE(APPLE_INTERNAL_SDK) over has_include checks. r=me.
> 
> > Source/WebCore/bindings/objc/PublicDOMInterfaces.h:1258
> > -#if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE
> > +/* FIXME: We should explicitly include this header when generating bindings for iOS with the internal SDK
> > +instead of including it if the file exists so that we error out if the file doesn't exist when building
> > +for iOS with the internal iOS SDK. */
> > +#if __has_include(<WebKitAdditions/PublicDOMInterfacesIOS.h>)
> 
> You could also have CodeGeneratorObjC.pm add something like -DUSE_APPLE_INTERNAL_SDK to the compiler invocation that preprocesses this file if ${USE_INTERNAL_SDK} is YES, then check here if USE_APPLE_INTERNAL_SDK is defined.
> 

Will teach Source/WebCore/DerivedSources.make to pass WTF_USE_APPLE_INTERNAL_SDK as a define to the bindings generators when USE_APPLE_INTERNAL_SDK is defined.

> > Source/WebCore/platform/ios/PlatformEventFactoryIOS.h:35
> > +#if __has_include(<WebKitAdditions/PlatformTouchEventIOS.h>)
> >  #include <WebKitAdditions/PlatformTouchEventIOS.h>
> > +#endif
> 
> Can this be a USE(APPLE_INTERNAL_SDK) check instead?

Will change to USE(APPLE_INTERNAL_SDK) before landing.

[Daniel Bates]

Committed r172865: <http://trac.webkit.org/changeset/172865>