Use the feature flags mechanism to give default feature preference values
<rdar://problem/60057889>
Created attachment 392489 [details] WIP patch
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.
Created attachment 392602 [details] WIP patch: updated based on comments
Created attachment 392603 [details] Fix build failures on GTK and WPE
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)
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?
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).
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.
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.
Created attachment 392640 [details] Patch
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.
Created attachment 392655 [details] Update the patch based on Simon's comments
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?
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.
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.
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?
Created attachment 392665 [details] Update the patch based on Simon's comments
> >> 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.
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
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.
Created attachment 392741 [details] Patch for landing
Created attachment 392745 [details] Patch for landing (fix GTK build failures)
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
Created attachment 392759 [details] Patch for landing
Comment on attachment 392759 [details] Patch for landing Clearing flags on attachment: 392759 Committed r258026: <https://trac.webkit.org/changeset/258026>
All reviewed patches have been landed. Closing bug.
Reopen this bug to fix a build error on an old SDK.
Created attachment 392816 [details] A follow up patch to fix a build error
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>
Have to reopen it to fix a Catalyst build failure.
Created attachment 392880 [details] Fix a Catalyst build failure
Created attachment 392900 [details] Fix a Catalyst build failure - v2
Comment on attachment 392900 [details] Fix a Catalyst build failure - v2 Clearing flags on attachment: 392900 Committed r258090: <https://trac.webkit.org/changeset/258090>