Bug 186286

Summary: Revise DEFAULT_EXPERIMENTAL_FEATURES_ENABLED to work properly on Apple builds
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ews-watchlist, keith_miller, mark.lam, mitz, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mitz: review+

Description Brent Fulgham 2018-06-04 15:14:01 PDT
We currently have a very fragile system where we have a flag enabling experimental features for our development builds and Safari Technology Preview, but do not want to ship this not-ready-for-prime-time features in releases. This generally involves landing a branch-specific hard-coded flag to turn these features off.

This is silly! We know what we are building. Let's let our build system do this work for us.
Comment 1 Radar WebKit Bug Importer 2018-06-04 15:14:20 PDT
<rdar://problem/40782992>
Comment 2 Brent Fulgham 2018-06-04 19:01:08 PDT
Created attachment 341946 [details]
Patch
Comment 3 Brent Fulgham 2018-06-04 19:39:05 PDT
Created attachment 341949 [details]
Patch
Comment 4 mitz 2018-06-05 09:43:46 PDT
Comment on attachment 341949 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        Use the WK_RELOCATABLE_FRAMEWORKS flag (which is always defined for non-production builds)
> +        to define ENABLE(EXPERIMENTAL_FEATURES) so that we do not need to manually
> +        change this flag when preparing for a production release.

This seems like a reasonable plan, but then what you did below is strangely different and unnecessarily complicated.

> Source/WebKit/Configurations/BaseTarget.xcconfig:39
> +GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) $(FEATURE_DEFINES) $(WK_MANUAL_SANDBOXING_DEFINES) $(WK_CORE_PREDICTION_DEFINES) U_DISABLE_RENAMING=1 U_SHOW_CPLUSPLUS_API=0 WK_RELOCATABLE_FRAMEWORKS=$(WK_RELOCATABLE_FRAMEWORKS) FRAMEWORK_NAME=WebKit;

Instead of doing this, just make sure that ENABLE_EXPERIMENTAL_FEATURES is defined correctly. You can do this by adding something like this to (every copy of) FeatureDefines.xcconfig:

ENABLE_EXPERIMENTAL_FEATURES = $(ENABLE_EXPERIMENTAL_FEATURES_$(WK_RELOCATABLE_FRAMEWORKS));
ENABLE_EXPERIMENTAL_FEATURES_YES = ENABLE_EXPERIMENTAL_FEATURES;

And adding $(ENABLE_EXPERIMENTAL_FEATURES) to FEATURE_DEFINES in that file.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:69
> +bool enableExperimentalFeatures()
> +{
> +#if PLATFORM(COCOA) && defined(WK_RELOCATABLE_FRAMEWORKS)
> +    return STRINGIZE_VALUE_OF(WK_RELOCATABLE_FRAMEWORKS) == "YES";
> +#elif ENABLE(EXPERIMENTAL_FEATURES)
> +    return true;
> +#else
> +    return false;
> +#endif
> +}

Then you won’t need this.

> Source/WebKit/Shared/WebPreferencesDefaultValues.h:191
> -// Cocoa ports must disable experimental features on release branches for now.
> -#if ENABLE(EXPERIMENTAL_FEATURES) || PLATFORM(COCOA)
> -#define DEFAULT_EXPERIMENTAL_FEATURES_ENABLED true
> -#else
> -#define DEFAULT_EXPERIMENTAL_FEATURES_ENABLED false
> -#endif
> +#define DEFAULT_EXPERIMENTAL_FEATURES_ENABLED enableExperimentalFeatures()

And here you’ll just need to remove the “|| PLATFORM(COCOA)” from the first #if.
Comment 5 Brent Fulgham 2018-06-05 10:07:56 PDT
Comment on attachment 341949 [details]
Patch

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

>> Source/WebKit/Configurations/BaseTarget.xcconfig:39
>> +GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES) $(FEATURE_DEFINES) $(WK_MANUAL_SANDBOXING_DEFINES) $(WK_CORE_PREDICTION_DEFINES) U_DISABLE_RENAMING=1 U_SHOW_CPLUSPLUS_API=0 WK_RELOCATABLE_FRAMEWORKS=$(WK_RELOCATABLE_FRAMEWORKS) FRAMEWORK_NAME=WebKit;
> 
> Instead of doing this, just make sure that ENABLE_EXPERIMENTAL_FEATURES is defined correctly. You can do this by adding something like this to (every copy of) FeatureDefines.xcconfig:
> 
> ENABLE_EXPERIMENTAL_FEATURES = $(ENABLE_EXPERIMENTAL_FEATURES_$(WK_RELOCATABLE_FRAMEWORKS));
> ENABLE_EXPERIMENTAL_FEATURES_YES = ENABLE_EXPERIMENTAL_FEATURES;
> 
> And adding $(ENABLE_EXPERIMENTAL_FEATURES) to FEATURE_DEFINES in that file.

Makes sense. I'll take that approach.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:69
>> +}
> 
> Then you won’t need this.

Good!

>> Source/WebKit/Shared/WebPreferencesDefaultValues.h:191
>> +#define DEFAULT_EXPERIMENTAL_FEATURES_ENABLED enableExperimentalFeatures()
> 
> And here you’ll just need to remove the “|| PLATFORM(COCOA)” from the first #if.

Great!
Comment 6 Brent Fulgham 2018-06-05 10:13:11 PDT
Created attachment 341973 [details]
Patch
Comment 7 Brent Fulgham 2018-06-05 11:14:06 PDT
Committed r232515: <https://trac.webkit.org/changeset/232515>