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
Summary: [iOS] Disable ENABLE_IOS_{GESTURE, TOUCH}_EVENTS, and temporarily disable ENA...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 135993
Blocks: 179168
  Show dependency tree / graph
 
Reported: 2014-08-14 11:11 PDT by Daniel Bates
Modified: 2017-11-02 05:08 PDT (History)
5 users (show)

See Also:

Attachments
Patch (73.41 KB, patch)
2014-08-14 11:47 PDT, Daniel Bates 		aestes: review+
Details | Formatted Diff | Diff
Patch (68.80 KB, patch)
2014-08-20 15:17 PDT, Daniel Bates 		aestes: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-08-14 11:11:36 PDT 
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.
Comment 1 Daniel Bates 2014-08-14 11:47:41 PDT 
Created attachment 236606 [details]
Patch
Comment 2 Andy Estes 2014-08-14 13:19:32 PDT 
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.
Comment 3 Daniel Bates 2014-08-14 16:05:18 PDT 
(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.
Comment 4 Andy Estes 2014-08-14 18:42:13 PDT 
Probably goes without saying, but you might want to post another patch to verify that the EWS bots are happy before you land.
Comment 5 Daniel Bates 2014-08-20 15:17:22 PDT 
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.
Comment 6 Andy Estes 2014-08-20 15:57:17 PDT 
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?
Comment 7 Daniel Bates 2014-08-22 11:39:04 PDT 
(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.
Comment 8 Daniel Bates 2014-08-22 12:09:28 PDT 
Committed r172865: <http://trac.webkit.org/changeset/172865>