Bug 217582 – [Preferences] Introduce string based SPI for WKPreferences to allow tests to change internal behavior without always adding additional SPI

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

[Sam Weinig]

[Preferences] Introduce string based SPI for WKPreferences to allow tests to change internal behavior without always adding additional SPI

[Sam Weinig]

Created attachment 411059 [details]
Patch

[Sam Weinig]

Created attachment 411061 [details]
Patch

[Sam Weinig]

Created attachment 411068 [details]
Patch

[Sam Weinig]

Created attachment 411077 [details]
Patch

[Sam Weinig]

Created attachment 411078 [details]
Patch

[Sam Weinig]

Created attachment 411079 [details]
Patch

[Sam Weinig]

Created attachment 411085 [details]
Patch

[Sam Weinig]

Created attachment 411092 [details]
Patch

[Darin Adler]

Comment on attachment 411092 [details]
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?

[EWS]

Committed r268342: <https://trac.webkit.org/changeset/268342>

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

[Radar WebKit Bug Importer]

<rdar://problem/70194793>

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> Comment on attachment 411092 [details]
> 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?

Yeah, I guess so. I kind of hate that, but it's better than a comment for sure.

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

I don't think we need that at the moment, but it may come up. We may also want functions for thinks like checking if "feature string" / "key" is supported or maybe getting the default value, but I am going to hold off on adding them until I really need them. 

> 
> > 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?

Yeah, we probably should.

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

Those are hardcoded to only work for experimental or internal feature keys. The new one works for any key. I will be getting rid of the experimental and internal ones that take string keys soon.

> 
> > 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?

The test runner override stuff is some convoluted WebProcess/InjectedBundle only functionality that gets reset anytime any preference changes, that I will replace with use of window.internals/test headers soon. All WKPreferencesResetTestRunnerOverrides() does is force a a fake update of the preferences to reset all the overrides in web process.

> 
> > 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?

Maybe. An alternate option is to use something like https://bugs.webkit.org/show_bug.cgi?id=217297, with a set of defaults defined in ./LayoutTests. 

> 
> > 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?

It will be in future changes.


Thanks for the review!