Bug 228564 – [macOS Catalina] Some feature preferences have wrong default values

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

[Peng Liu]

When the feature flags SPI is not available, some feature preferences' default values are wrong.

[Peng Liu]

<rdar://81177857>

[Peng Liu]

Created attachment 434451 [details]
Patch

[Tim Horton]

Comment on attachment 434451 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:89
> +#if HAVE(SYSTEM_FEATURE_FLAGS)
>      static const bool runningBoardHandlesPriorities = isFeatureFlagEnabled("RB_full_manage_WK_jetsam"_s);
> +#else
> +    const bool runningBoardHandlesPriorities = true;
> +#endif

I don't think you need this change; RUNNINGBOARD_WEBKIT_PRIORITY_SUPPORT is only on on iOS-family platforms, where we don't backdeploy and thus SYSTEM_FEATURE_FLAGS is always true. (Also evidenced by the fact that it wasn't guarded before)

[Peng Liu]

Comment on attachment 434451 [details]
Patch

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

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:89
>> +#endif
> 
> I don't think you need this change; RUNNINGBOARD_WEBKIT_PRIORITY_SUPPORT is only on on iOS-family platforms, where we don't backdeploy and thus SYSTEM_FEATURE_FLAGS is always true. (Also evidenced by the fact that it wasn't guarded before)

Hmm, I was misled by the folder name (Mac/ProcessLauncherMac.mm).

Maybe we should remove `RB_full_manage_WK_jetsam` from Webkit-macos.plist?

[Tim Horton]

Comment on attachment 434451 [details]
Patch

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

>>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:89
>>> +#endif
>> 
>> I don't think you need this change; RUNNINGBOARD_WEBKIT_PRIORITY_SUPPORT is only on on iOS-family platforms, where we don't backdeploy and thus SYSTEM_FEATURE_FLAGS is always true. (Also evidenced by the fact that it wasn't guarded before)
> 
> Hmm, I was misled by the folder name (Mac/ProcessLauncherMac.mm).
> 
> Maybe we should remove `RB_full_manage_WK_jetsam` from Webkit-macos.plist?

A sad vestige of history (should say Cocoa).

Yeah, we can remove it from the plist later, but in this patch we can just leave it alone.

[Peng Liu]

Created attachment 434465 [details]
Revise the patch based on Tim's comment

[Tim Horton]

Comment on attachment 434465 [details]
Revise the patch based on Tim's comment

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

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:295
> +#if PLATFORM(MAC)
> +    return true;
> +#else
>      return false;
> +#endif

Technically by the same logic you could just change these to return true instead of the PLATFORM(MAC), because we'll always have SYSTEM_FEATURE_FLAGS on iOS-family). But either way will behave the same.

[EWS]

Committed r280424 (240064@main): <https://commits.webkit.org/240064@main>

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

[Peng Liu]

The API test failures are tracked by bug 228589.