[webkit-reviews] review granted: [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 : [Attachment 236606] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
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
https://bugs.webkit.org/show_bug.cgi?id=135945

Attachment 236606: Patch
https://bugs.webkit.org/attachment.cgi?id=236606&action=review

------- Additional Comments from Andy Estes <aestes at apple.com>
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.