Bug 217582 - [Preferences] Introduce string based SPI for WKPreferences to allow tests to change internal behavior without always adding additional SPI
Summary: [Preferences] Introduce string based SPI for WKPreferences to allow tests to ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-11 12:45 PDT by Sam Weinig
Modified: 2020-10-12 10:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (70.84 KB, patch)
2020-10-11 12:48 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (71.27 KB, patch)
2020-10-11 13:15 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (71.50 KB, patch)
2020-10-11 15:08 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (74.18 KB, patch)
2020-10-11 16:24 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (73.90 KB, patch)
2020-10-11 16:29 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (69.99 KB, patch)
2020-10-11 16:31 PDT, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (69.39 KB, patch)
2020-10-11 18:19 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (69.50 KB, patch)
2020-10-11 19:35 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-10-11 12:45:30 PDT
[Preferences] Introduce string based SPI for WKPreferences to allow tests to change internal behavior without always adding additional SPI
Comment 1 Sam Weinig 2020-10-11 12:48:35 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-10-11 13:15:16 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2020-10-11 15:08:51 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2020-10-11 16:24:50 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2020-10-11 16:29:19 PDT Comment hidden (obsolete)
Comment 6 Sam Weinig 2020-10-11 16:31:06 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2020-10-11 18:19:21 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2020-10-11 19:35:38 PDT
Created attachment 411092 [details]
Patch
Comment 9 Darin Adler 2020-10-11 21:03:29 PDT
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?
Comment 10 EWS 2020-10-11 23:29:58 PDT
Committed r268342: <https://trac.webkit.org/changeset/268342>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411092 [details].
Comment 11 Radar WebKit Bug Importer 2020-10-11 23:30:17 PDT
<rdar://problem/70194793>
Comment 12 Sam Weinig 2020-10-12 10:43:16 PDT
(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!