[webkit-reviews] review granted: [Bug 217582] [Preferences] Introduce string based SPI for WKPreferences to allow tests to change internal behavior without always adding additional SPI : [Attachment 411092] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 217582: [Preferences] Introduce string based SPI for WKPreferences to allow
tests to change internal behavior without always adding additional SPI
https://bugs.webkit.org/show_bug.cgi?id=217582

Attachment 411092: Patch

https://bugs.webkit.org/attachment.cgi?id=411092&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 411092
  --> https://bugs.webkit.org/attachment.cgi?id=411092
Patch

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

> Source/WebKit/ChangeLog:10
> +	   Add SPI for setting any WebKit preference using the key as defined
in the WebPreferences*.yaml
> +	   files. This will allow adding testing of non-default behavior that
we don't necessarily want to
> +	   expose via its own API or SPI.

If it’s for testing specifically, should we put "for testing" in the function
names?

Also, do we need something that resets everything to defaults, or things that
set all features of a certain category ("all experimental features on")?

> Source/WebKit/UIProcess/WebPreferences.h:80
>      // Exposed for WebKitTestRunner use only.

Could we have put that in the function names?

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:58
> +// The following generic setter functions are only intended for use by
testing infrastructure.

Same question: Could we encode that in the name?

How is the bool one below different from
WKPreferencesSetExperimentalFeatureForKey and
WKPreferencesSetInternalDebugFeatureForKey?

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:65
>  WK_EXPORT void WKPreferencesResetTestRunnerOverrides(WKPreferencesRef
preferencesRef);

What is a "test runner override" and how is that different from other settings?

> Tools/WebKitTestRunner/TestController.cpp:845
>      // Set internal features that have different default values for testing.

Should this be configured by the .yaml file instead of explicit code here?

> Tools/WebKitTestRunner/TestOptions.cpp:101
>  static const std::unordered_map<std::string, bool>& boolDefaultsMap()

Should this be configured by the .yaml file instead of explicit code here?