Bug 228803 – [macOS] Clean up Feature Flags related code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peng Liu]

[macOS] Clean up Feature Flags related code

[Peng Liu]

<rdar://81142982>

[Peng Liu]

Created attachment 434944 [details]
Patch

[Peng Liu]

Comment on attachment 434944 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        For system WebKit and staged framework, we use the Feature Flags SPI to

The comment is not accurate. For staged framework, webkit parses the plist file instead of using the SPI.

[Simon Fraser (smfr)]

Comment on attachment 434944 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:50
> +    const static NSBundle *bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")];
> +
> +    static bool isSystemWebKit = [] {
> +        return [bundle.bundlePath hasPrefix:@"/System/"];
> +    }();

Use AuxiliaryProcess::isSystemWebKit() which also had some logic for readonly system volumes?

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:60
>      if (!isWebKitBundleFromStagedFramework)
> -        return _os_feature_enabled_impl("WebKit", (const char*)featureName.utf8().data());
> +        return defaultValue;

I would prefer you write this as:

if (isWebKitBundleFromStagedFramework) {
   ... read from /Library/Apple/System/Library/FeatureFlags/Domain/WebKit.plist
    return;
}

> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:65
>          return _os_feature_enabled_impl("WebKit", (const char*)featureName.characters8());

I think it would make more sense here to return defaultValue?

> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:48
> +bool isFeatureFlagEnabled(const String& featureName, bool defaultValue)

I wish we could share this code. But make it identical.

[Peng Liu]

Comment on attachment 434944 [details]
Patch

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

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:50
>> +    }();
> 
> Use AuxiliaryProcess::isSystemWebKit() which also had some logic for readonly system volumes?

Yes. But it will return true for staged framework.

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:60
>> +        return defaultValue;
> 
> I would prefer you write this as:
> 
> if (isWebKitBundleFromStagedFramework) {
>    ... read from /Library/Apple/System/Library/FeatureFlags/Domain/WebKit.plist
>     return;
> }

Sounds a good idea considering the next comment.

>> Source/WebKit/Shared/Cocoa/WebPreferencesDefaultValuesCocoa.mm:65
>>          return _os_feature_enabled_impl("WebKit", (const char*)featureName.characters8());
> 
> I think it would make more sense here to return defaultValue?

Yeah, that seems better.

>> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:48
>> +bool isFeatureFlagEnabled(const String& featureName, bool defaultValue)
> 
> I wish we could share this code. But make it identical.

Agree.

[Peng Liu]

Created attachment 434948 [details]
Revise the patch based on Simon's comments

[Peng Liu]

Created attachment 434951 [details]
Update change logs

[Tim Horton]

Comment on attachment 434951 [details]
Update change logs

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

> Source/WebKit/ChangeLog:10
> +        For other cases, let's use hardcoded values.

This could use more words, we have a /strong/ motivation.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:258
> +    // TODO: double check

This looks fine to me (since it's inside HAVE(INCREMENTAL_PDF_APIS); if you've got them, we want to use them.)

> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:81
> +    // TODO: double check

Yeah, we don't have support in WebKitLegacy so this is right.

> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:92
> +    // TODO: double check
> +    return isFeatureFlagEnabled("WebXR", false);

This is right

[Peng Liu]

Created attachment 434964 [details]
Update change logs

[Peng Liu]

Created attachment 434999 [details]
Fix a layout test failure

[Kate Cheney]

Comment on attachment 434999 [details]
Fix a layout test failure

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

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:245
>  bool defaultInAppBrowserPrivacy()

It doesn't seem like this function is used anywhere. I think we can remove it.

[Peng Liu]

Created attachment 435010 [details]
Revise the patch based on Kate's comment

[Peng Liu]

Comment on attachment 434999 [details]
Fix a layout test failure

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

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:245
>>  bool defaultInAppBrowserPrivacy()
> 
> It doesn't seem like this function is used anywhere. I think we can remove it.

Yes! Removed.
I will remove the corresponding item in WebKit.plist in a follow-up patch.

[Peng Liu]

Created attachment 435023 [details]
Update a layout test expectation

[Tim Horton]

Comment on attachment 435023 [details]
Update a layout test expectation

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

I really hope we can get rid of this ASAP

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:122
>  #if PLATFORM(MAC)
> -    return true;
> +    bool defaultValue = true;
> +#else
> +    bool defaultValue = false;
>  #endif

This is wrong, async_frame_and_overflow_scrolling should be on everywhere (confirm with smfr, but the plists agree)

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:313
> +    return isFeatureFlagEnabled("webm_webaudio", false);

Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong.

> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:54
> +        NSBundle *bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")];

It seems exceedingly bizarre that WebKitLegacy looks up WKWebView; maybe you should use WebView here? (what does other code do?)

> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:290
> +    return isFeatureFlagEnabled("webm_webaudio", false);

Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong.

> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:299
> -#if HAVE(SYSTEM_FEATURE_FLAGS)
> -    return isFeatureFlagEnabled("vp8_decoder");
> -#endif
> -
> -    return false;
> +    return isFeatureFlagEnabled("vp8_decoder", false);

This key does not exist in any of the feature flags plists, so this should not use isFeatureFlagEnabled, should just return false (at which point you should get rid of the function and just put the `false` in WebPreferences.yaml directly).

[Tim Horton]

Comment on attachment 435023 [details]
Update a layout test expectation

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

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:122
>>  #endif
> 
> This is wrong, async_frame_and_overflow_scrolling should be on everywhere (confirm with smfr, but the plists agree)

Ah, simon points out the #ifdef above covers iOS

[Peng Liu]

Comment on attachment 435023 [details]
Update a layout test expectation

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

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:313
>> +    return isFeatureFlagEnabled("webm_webaudio", false);
> 
> Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong.

It was a mistake to enable `webm_webaudio` through the plist file. I will remove it from the WebKit.plist for appletvOS, watchOS, and tvOS, and change its value to false in WebKit-macOS.plist.

>> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:54
>> +        NSBundle *bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")];
> 
> It seems exceedingly bizarre that WebKitLegacy looks up WKWebView; maybe you should use WebView here? (what does other code do?)

Good catch. Will fix it. Thanks!

>> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:290
>> +    return isFeatureFlagEnabled("webm_webaudio", false);
> 
> Why is this `false` here if it's `true` in the plist? That means that a local build will have it off on Monterey even though it's on in the system WebKit, and seems wrong.

Will fix. Actually this feature should be disabled.

>> Source/WebKitLegacy/mac/WebView/WebPreferencesDefaultValues.mm:299
>> +    return isFeatureFlagEnabled("vp8_decoder", false);
> 
> This key does not exist in any of the feature flags plists, so this should not use isFeatureFlagEnabled, should just return false (at which point you should get rid of the function and just put the `false` in WebPreferences.yaml directly).

Will add an item to the plist files to enable it.

[Peng Liu]

Created attachment 435039 [details]
Revise the patch based on Tim's comments

[Peng Liu]

Created attachment 435040 [details]
Update changelogs

[Peng Liu]

Created attachment 435055 [details]
Rebase a test expectation for BigSur

[Peng Liu]

The test failure (fast/speechrecognition/start-recognition-twice-exception.html) is not relevant to this patch.

[EWS]

Committed r280726 (240316@main): <https://commits.webkit.org/240316@main>

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

[Truitt Savell]

Looks like the changes here broke 25 imported/w3c/web-platform-tests/fetch/ tests, tracking in https://bugs.webkit.org/show_bug.cgi?id=228926