Bug 208607 – Use the feature flags mechanism to give default feature preference values

• Component: New Bugs
• Status: RESOLVED FIXED

[Peng Liu]

Use the feature flags mechanism to give default feature preference values

[Radar WebKit Bug Importer]

<rdar://problem/60057889>

[Peng Liu]

Created attachment 392489 [details]
WIP patch

[Simon Fraser (smfr)]

Comment on attachment 392489 [details]
WIP patch

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

> Source/WebKit/Feature Flags/WebKit.plist:1
> +<?xml version="1.0" encoding="UTF-8"?>

Remove the space from the "Feature Flags" directory name.

I think we'll also need different plists on different platforms (or preprocess it which would require a new build step).

> Source/WebKit/Feature Flags/WebKit.plist:5
> +	<key>async_scrolling</key>

async_frame_and_overflow_scrolling

> Source/WebKit/Feature Flags/WebKit.plist:8
> +		<key>Enabled</key>
> +		<false/>

I think you have tabs here.

[Peng Liu]

Created attachment 392602 [details]
WIP patch: updated based on comments

[Peng Liu]

Created attachment 392603 [details]
Fix build failures on GTK and WPE

[Simon Fraser (smfr)]

Comment on attachment 392603 [details]
Fix build failures on GTK and WPE

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

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:40
> +#if USE(APPLE_INTERNAL_SDK)
> +#include <os/feature_private.h>
> +#endif

Normally we'd put this into a FooSPI.h header but here the call sites are all wrapped in USE(APPLE_INTERNAL_SDK) so I'm not sure.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:90
> +#if PLATFORM(IOS_FAMILY) || USE(NICOSIA)
> +#if USE(APPLE_INTERNAL_SDK)

Weird for USE(NICOSIA) to wrap USE(APPLE_INTERNAL_SDK)

[youenn fablet]

Comment on attachment 392603 [details]
Fix build failures on GTK and WPE

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

> Source/WebKit/FeatureFlags/WebKit.plist:10
> +    <key>canvas_and_media_in_gpu_process</key>

Can we add another one called webrtc_in_gpu_process?

When enabled, it should activate webrtc codecs and video capture.
It should also activate audio capture on MacOS.
On iOS, audio capture should be controlled by canvas_and_media_in_gpu_process.

Also, on MacOS, this is enabled by default, can we keep it that way?

[Simon Fraser (smfr)]

Comment on attachment 392603 [details]
Fix build failures on GTK and WPE

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

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:40
>> +#endif
> 
> Normally we'd put this into a FooSPI.h header but here the call sites are all wrapped in USE(APPLE_INTERNAL_SDK) so I'm not sure.

Actually we should put this in an SPI header, and add a prototype for non-internal builds. You'll need to wrap it all in something that's pull in via AdditionalFeatureDefines.h like HAVE_OS_FEATURE_ENABLED

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:90
>> +#if USE(APPLE_INTERNAL_SDK)
> 
> Weird for USE(NICOSIA) to wrap USE(APPLE_INTERNAL_SDK)

Also you should not wrap this all in USE(APPLE_INTERNAL_SDK) because opensource builds need to also call the function (otherwise they will have different behavior).

[Peng Liu]

Comment on attachment 392489 [details]
WIP patch

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

>> Source/WebKit/Feature Flags/WebKit.plist:1
>> +<?xml version="1.0" encoding="UTF-8"?>
> 
> Remove the space from the "Feature Flags" directory name.
> 
> I think we'll also need different plists on different platforms (or preprocess it which would require a new build step).

The space in the directory name is removed.

Filed bug #208665 for the task to add different plists for different platforms.

>> Source/WebKit/Feature Flags/WebKit.plist:5
>> +	<key>async_scrolling</key>
> 
> async_frame_and_overflow_scrolling

Changed.

>> Source/WebKit/Feature Flags/WebKit.plist:8
>> +		<false/>
> 
> I think you have tabs here.

Right. This was done by the Xcode editor.
Fixed.

[Peng Liu]

Comment on attachment 392603 [details]
Fix build failures on GTK and WPE

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

>> Source/WebKit/FeatureFlags/WebKit.plist:10
>> +    <key>canvas_and_media_in_gpu_process</key>
> 
> Can we add another one called webrtc_in_gpu_process?
> 
> When enabled, it should activate webrtc codecs and video capture.
> It should also activate audio capture on MacOS.
> On iOS, audio capture should be controlled by canvas_and_media_in_gpu_process.
> 
> Also, on MacOS, this is enabled by default, can we keep it that way?

Sure. Added a feature flag called webrtc_in_gpu_process and updated the related functions.
CaptureVideoInUIProcessEnabled is always false, so it is unnecessary to add a function for it.

We will need to update the plist file to enable it by default (for cocoa platforms).

>>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:40
>>> +#endif
>> 
>> Normally we'd put this into a FooSPI.h header but here the call sites are all wrapped in USE(APPLE_INTERNAL_SDK) so I'm not sure.
> 
> Actually we should put this in an SPI header, and add a prototype for non-internal builds. You'll need to wrap it all in something that's pull in via AdditionalFeatureDefines.h like HAVE_OS_FEATURE_ENABLED

Added pal/spi/cocoa/FeatureFlagsSPI.h.

>>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:90
>>> +#if USE(APPLE_INTERNAL_SDK)
>> 
>> Weird for USE(NICOSIA) to wrap USE(APPLE_INTERNAL_SDK)
> 
> Also you should not wrap this all in USE(APPLE_INTERNAL_SDK) because opensource builds need to also call the function (otherwise they will have different behavior).

Fixed.

[Peng Liu]

Created attachment 392640 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392640 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/FeatureFlagsSPI.h:35
> +bool _os_feature_enabled_impl(const char *domain, const char *feature);

Do we need an extern "C" around this?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:85
> +bool defaultAsyncFrameScrollingEnabled()

Maybe make this into a static helper function called defaultAsyncFrameAndOverflowScrollingEnabled() and call it from the two places.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:88
> +#if PLATFORM(IOS_FAMILY) || USE(NICOSIA)

Give USE(NICOSIA) its own #if block and return true.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:104
> +{
> +#if PLATFORM(COCOA)
> +    return defaultAsyncFrameScrollingEnabled();
> +#else
> +    return false;
> +#endif

Need a USE(NICOSIA) here

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:137
> +    return os_feature_enabled(WebKit, webrtc_in_gpu_process);
> +#else
> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);

This is confusing, and seems like it would prevent us from toggling audio capture independently if we need to.

[Peng Liu]

Created attachment 392655 [details]
Update the patch based on Simon's comments

[Simon Fraser (smfr)]

Comment on attachment 392655 [details]
Update the patch based on Simon's comments

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

> Source/WTF/wtf/PlatformHave.h:610
> +#define HAVE_SYSTEM_FEATURE_FLAGS_SPI 1

Call this HAVE_SYSTEM_FEATURE_FLAGS

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:98
> +#if USE(NICOSIA)
> +    return true;
> +#elif HAVE(HAVE_SYSTEM_FEATURE_FLAGS_SPI)
> +
> +#if PLATFORM(IOS_FAMILY)
> +    return os_feature_enabled(WebKit, async_frame_and_overflow_scrolling);
> +#else
> +    return false;
> +#endif
> +
> +#else
> +    return false;
> +#endif

#if PLATFORM(IOS_FAMILY) || USE(NICOSIA)
    return true;
#endif

#if PLATFORM(MAC) && HAVE(HAVE_SYSTEM_FEATURE_FLAGS_SPI)
    return os_feature_enabled(WebKit, async_frame_and_overflow_scrolling);
#endif

    return false;

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:116
> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);

This will give the same answer for macOS and iOS. Is that OK?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:125
> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);

This will give the same answer for macOS and iOS. Is that OK?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:137
> +#if HAVE(HAVE_SYSTEM_FEATURE_FLAGS_SPI)

I think it's better if this is nested inside the platform checks.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:155
> +    return !defaultCaptureAudioInGPUProcessEnabled();

This will give the same answer for macOS and iOS. Is that OK?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:168
> +    return os_feature_enabled(WebKit, webrtc_in_gpu_process);

This will give the same answer for macOS and iOS. Is that OK?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:190
> +    return os_feature_enabled(WebKit, WebGL2);

This will give the same answer for macOS and iOS. Is that OK?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:203
> +    return os_feature_enabled(WebKit, WebGPU);

This will give the same answer for macOS and iOS. Is that OK?

[Peng Liu]

Comment on attachment 392640 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/cocoa/FeatureFlagsSPI.h:35
>> +bool _os_feature_enabled_impl(const char *domain, const char *feature);
> 
> Do we need an extern "C" around this?

Right. Fixed.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:85
>> +bool defaultAsyncFrameScrollingEnabled()
> 
> Maybe make this into a static helper function called defaultAsyncFrameAndOverflowScrollingEnabled() and call it from the two places.

Good idea! Done.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:88
>> +#if PLATFORM(IOS_FAMILY) || USE(NICOSIA)
> 
> Give USE(NICOSIA) its own #if block and return true.

Fixed.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:104
>> +#endif
> 
> Need a USE(NICOSIA) here

Fixed.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:137
>> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);
> 
> This is confusing, and seems like it would prevent us from toggling audio capture independently if we need to.

This is the requirement from Youenn.

[Peng Liu]

Comment on attachment 392655 [details]
Update the patch based on Simon's comments

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

>> Source/WTF/wtf/PlatformHave.h:610
>> +#define HAVE_SYSTEM_FEATURE_FLAGS_SPI 1
> 
> Call this HAVE_SYSTEM_FEATURE_FLAGS

OK, fixed.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:98
>> +#endif
> 
> #if PLATFORM(IOS_FAMILY) || USE(NICOSIA)
>     return true;
> #endif
> 
> #if PLATFORM(MAC) && HAVE(HAVE_SYSTEM_FEATURE_FLAGS_SPI)
>     return os_feature_enabled(WebKit, async_frame_and_overflow_scrolling);
> #endif
> 
>     return false;

Fixed.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:116
>> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);
> 
> This will give the same answer for macOS and iOS. Is that OK?

This won't be an issue after we use different plists for different platforms.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:125
>> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);
> 
> This will give the same answer for macOS and iOS. Is that OK?

This won't be an issue after we use different plists for different platforms.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:137
>> +#if HAVE(HAVE_SYSTEM_FEATURE_FLAGS_SPI)
> 
> I think it's better if this is nested inside the platform checks.

OK. I will update this function and let Youenn to review it.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:155
>> +    return !defaultCaptureAudioInGPUProcessEnabled();
> 
> This will give the same answer for macOS and iOS. Is that OK?

This won't be an issue after we use different plists for different platforms.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:168
>> +    return os_feature_enabled(WebKit, webrtc_in_gpu_process);
> 
> This will give the same answer for macOS and iOS. Is that OK?

This won't be an issue after we use different plists for different platforms.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:190
>> +    return os_feature_enabled(WebKit, WebGL2);
> 
> This will give the same answer for macOS and iOS. Is that OK?

This won't be an issue after we use different plists for different platforms.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:203
>> +    return os_feature_enabled(WebKit, WebGPU);
> 
> This will give the same answer for macOS and iOS. Is that OK?

This won't be an issue after we use different plists for different platforms.

[Peng Liu]

Comment on attachment 392603 [details]
Fix build failures on GTK and WPE

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

>>> Source/WebKit/FeatureFlags/WebKit.plist:10
>>> +    <key>canvas_and_media_in_gpu_process</key>
>> 
>> Can we add another one called webrtc_in_gpu_process?
>> 
>> When enabled, it should activate webrtc codecs and video capture.
>> It should also activate audio capture on MacOS.
>> On iOS, audio capture should be controlled by canvas_and_media_in_gpu_process.
>> 
>> Also, on MacOS, this is enabled by default, can we keep it that way?
> 
> Sure. Added a feature flag called webrtc_in_gpu_process and updated the related functions.
> CaptureVideoInUIProcessEnabled is always false, so it is unnecessary to add a function for it.
> 
> We will need to update the plist file to enable it by default (for cocoa platforms).

> Also, on MacOS, this is enabled by default, can we keep it that way?
You mean webrtc_in_gpu_process is enabled on Mac by default?

[Peng Liu]

Created attachment 392665 [details]
Update the patch based on Simon's comments

[youenn fablet]

> >> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:137
> >> +    return os_feature_enabled(WebKit, canvas_and_media_in_gpu_process);
> > 
> > This is confusing, and seems like it would prevent us from toggling audio capture independently if we need to.
> 
> This is the requirement from Youenn.

On MacOS, we can toggle audio capture independently.
On iOS, we need to have audio capture where audio playback happens.

[youenn fablet]

Comment on attachment 392665 [details]
Update the patch based on Simon's comments

r=me.
Please make sure GTK/WPE bots are compiling.
It might be worth testing that this works as expected on Mac/iOS.

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

> Source/WebKit/Shared/WebPreferences.yaml:1755
> +  condition: ENABLE(GPU_PROCESS)

If you add ENABLE(GPU_PROCESS) to RenderCanvasInGPUProcessEnabled, you should probably add it for other GPU stuff like CaptureAudioInGPUProcessEnabled, CaptureVideoInGPUProcessEnabled and WebRTCPlatformCodecsInGPUProcessEnabled.
And keep that in sync with the ifdef of defaultRenderCanvasInGPUProcessEnabled definition.
Not sure this is worth it though.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:90
> +#if PLATFORM(MAC) && HAVE(HAVE_SYSTEM_FEATURE_FLAGS)

This should probably be
#if HAVE(SYSTEM_FEATURE_FLAGS)
Ditto elsewhere

We could use #elif here and below.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:150
> +    return !defaultCaptureAudioInGPUProcessEnabled();

Let's change this to:
#if PLATFORM(MAC)
    return !defaultCaptureAudioInGPUProcessEnabled();
#else
   return false;
#endif

[Peng Liu]

Comment on attachment 392665 [details]
Update the patch based on Simon's comments

I have verified that this patch works on an iPhone. We have to copy the WebKit.plist manually to test for now.

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

>> Source/WebKit/Shared/WebPreferences.yaml:1755
>> +  condition: ENABLE(GPU_PROCESS)
> 
> If you add ENABLE(GPU_PROCESS) to RenderCanvasInGPUProcessEnabled, you should probably add it for other GPU stuff like CaptureAudioInGPUProcessEnabled, CaptureVideoInGPUProcessEnabled and WebRTCPlatformCodecsInGPUProcessEnabled.
> And keep that in sync with the ifdef of defaultRenderCanvasInGPUProcessEnabled definition.
> Not sure this is worth it though.

I see.
Alright, let me revert this change and the other similar ones. You folks decide whether we need to add it back later.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:90
>> +#if PLATFORM(MAC) && HAVE(HAVE_SYSTEM_FEATURE_FLAGS)
> 
> This should probably be
> #if HAVE(SYSTEM_FEATURE_FLAGS)
> Ditto elsewhere
> 
> We could use #elif here and below.

Right. It will do the same thing concisely here. But in other places we do need to check the platform. So I will only update this one.

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:150
>> +    return !defaultCaptureAudioInGPUProcessEnabled();
> 
> Let's change this to:
> #if PLATFORM(MAC)
>     return !defaultCaptureAudioInGPUProcessEnabled();
> #else
>    return false;
> #endif

OK, will update it.
So CaptureAudioInUIProcessEnabled and CaptureAudioInGPUProcessEnabled can be both false for iOS. That's interesting.

[Peng Liu]

Created attachment 392741 [details]
Patch for landing

[Peng Liu]

Created attachment 392745 [details]
Patch for landing (fix GTK build failures)

[WebKit Commit Bot]

Comment on attachment 392745 [details]
Patch for landing (fix GTK build failures)

Rejecting attachment 392745 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 392745, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13334911

[Brent Fulgham]

Created attachment 392759 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 392759 [details]
Patch for landing

Clearing flags on attachment: 392759

Committed r258026: <https://trac.webkit.org/changeset/258026>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Peng Liu]

Reopen this bug to fix a build error on an old SDK.

[Peng Liu]

Created attachment 392816 [details]
A follow up patch to fix a build error

[WebKit Commit Bot]

Comment on attachment 392816 [details]
A follow up patch to fix a build error

Clearing flags on attachment: 392816

Committed r258046: <https://trac.webkit.org/changeset/258046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Peng Liu]

Have to reopen it to fix a Catalyst build failure.

[Peng Liu]

Created attachment 392880 [details]
Fix a Catalyst build failure

[Peng Liu]

Created attachment 392900 [details]
Fix a Catalyst build failure - v2

[WebKit Commit Bot]

Comment on attachment 392900 [details]
Fix a Catalyst build failure - v2

Clearing flags on attachment: 392900

Committed r258090: <https://trac.webkit.org/changeset/258090>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.