Bug 220521 – Add experimental feature to use network loader

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Add experimental feature to use network loader

[Alex Christensen]

Created attachment 417397 [details]
Patch

[Alex Christensen]

<rdar://problem/69394713>

[Sam Weinig]

Comment on attachment 417397 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        * Scripts/Preferences/WebPreferencesExperimental.yaml:

I know we have done this in the past, but thinking about it, I'm not sure it makes sense to use the preferences infrastructure for network process functionality, unless we are passing that preference on every request from a web process.

With our current setup, it seems like it would make more sense as API for the data store.

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:594
> +NetworkLoaderEnabled:

This name seems way to generic. How about something like ExperimentalCFNetworkLibNetworkLoaderEnabled (same idea for the HAVE and the human readable names/descriptions).

[Alex Christensen]

Comment on attachment 417397 [details]
Patch

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

>> Source/WTF/ChangeLog:9
>> +        * Scripts/Preferences/WebPreferencesExperimental.yaml:
> 
> I know we have done this in the past, but thinking about it, I'm not sure it makes sense to use the preferences infrastructure for network process functionality, unless we are passing that preference on every request from a web process.
> 
> With our current setup, it seems like it would make more sense as API for the data store.

I disagree.  We want it to be in the experimental features list in the UI, and this is how to do it.
I manually pass the value into the NetworkProcess through NetworkSessionCreationParameters.  Are you saying that it is also sent to the web process through generated code?  If so, and if that becomes a problem, I think we should add something to the yaml to tell it not to do that, kind of like webcoreBinding: none

>> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:594
>> +NetworkLoaderEnabled:
> 
> This name seems way to generic. How about something like ExperimentalCFNetworkLibNetworkLoaderEnabled (same idea for the HAVE and the human readable names/descriptions).

Ok.  I wasn't sure we wanted to be that explicit, but now there's no reason not to be.

[Alex Christensen]

Created attachment 418009 [details]
Patch

[Geoffrey Garen]

Comment on attachment 418009 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.h:101
> +    bool useNetworkLoader { false };

I think I would call this "useLibNetworkLoader" (to match the experimental feature name) or "useNWLoader" (to match the CFNetwork name). "Network" is a bit generic.

[Geoffrey Garen]

> > I know we have done this in the past, but thinking about it, I'm not sure it makes sense to use the preferences infrastructure for network process functionality, unless we are passing that preference on every request from a web process.
> > 
> > With our current setup, it seems like it would make more sense as API for the data store.
> 
> I disagree.  We want it to be in the experimental features list in the UI,
> and this is how to do it.
> I manually pass the value into the NetworkProcess through
> NetworkSessionCreationParameters.

I think I see two points being made here:

(1) It's nice to use the WebKit prefs infrastructure because that's how experimental features work;

(2) It's odd to program this pref in a way that can change within the lifetime of a give website data store (for example, between not launching and launching the first network process, and after a network process crash).

Maybe we can synthesize these points by having the website data store read and store the experimental feature pref in its constructor?

[Geoffrey Garen]

Thinking a little more, does the "API for the data store" suggestion mean:

(A) Public API on WKWebsiteDataStore? -- (seems super problematic, so probably not?)

(B) Private SPI on WKWebsiteDataStore? -- (less problematic, but still not obvious how to make this work with experimental features infrastructure)

(C) Part of the interface to WebKit::WebsiteDataStore::WebsiteDataStore(), passed in through WebKit::WebsiteDataStoreConfiguration? -- (seems reasonable to me)

[Geoffrey Garen]

Comment on attachment 418009 [details]
Patch

I'll say r+ since this patch follows the existing convention, but let's keep talking about whether we can improve on that convention.

[Alex Christensen]

Created attachment 418069 [details]
Patch

[EWS]

Committed r271714: <https://trac.webkit.org/changeset/271714>

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

[Sam Weinig]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 417397 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417397&action=review
> 
> >> Source/WTF/ChangeLog:9
> >> +        * Scripts/Preferences/WebPreferencesExperimental.yaml:
> > 
> > I know we have done this in the past, but thinking about it, I'm not sure it makes sense to use the preferences infrastructure for network process functionality, unless we are passing that preference on every request from a web process.
> > 
> > With our current setup, it seems like it would make more sense as API for the data store.
> 
> I disagree.  We want it to be in the experimental features list in the UI,
> and this is how to do it.

I think you misunderstand what the experimental preferences infrastructure is. I understand you want something in the UI in Safari, and that seems fine, but doing it through the existing experimental features infrastructure is incorrect as they are part of the WebPreferences class which is not global. The expectation of the SPI is that for any WebPreferences object you have you can get and set the experimental features state, and this change breaks that invariant. 

Specially, it's the code in WebsiteDataStoreCocoa.mm that is bad here. It is possible to use preferences (and therefore the experimental features infrastructure) if ensure that the preferences state being set is used, probably by passing it for each request, but it could also be done in other ways.

I have put quite a bit of effort in trying to clean up the preferences lately, so I would appreciate if you would fix this.

[Geoffrey Garen]

Sam, can you clarify your suggestion by answering the question in comment 8?

But also: Why is it a goal to make all experimental features gettable and settable at any time on a per-WebView basis? Does this even work? Like, if you turn on WebGL 2.0 or WebGPU and then turn it off in a live webpage.... what is the expectation?

[Alex Christensen]

We need to have a way for experimental features to affect things in the network process.  I'm open to suggestions and willing to improve things.

[Sam Weinig]

(In reply to Geoffrey Garen from comment #13)
> Sam, can you clarify your suggestion by answering the question in comment 8?

(In reply to Geoffrey Garen from comment #8)
> Thinking a little more, does the "API for the data store" suggestion mean:
> 
> (A) Public API on WKWebsiteDataStore? -- (seems super problematic, so
> probably not?)
> 
> (B) Private SPI on WKWebsiteDataStore? -- (less problematic, but still not
> obvious how to make this work with experimental features infrastructure)
> 
> (C) Part of the interface to WebKit::WebsiteDataStore::WebsiteDataStore(),
> passed in through WebKit::WebsiteDataStoreConfiguration? -- (seems
> reasonable to me)

My suggestion was B. You would make this work by changing the Safari code that creates the experimental features menu to also create a menu item for this.

> 
> But also: Why is it a goal to make all experimental features gettable and
> settable at any time on a per-WebView basis? 

It's not actually per-WKWebView, and I am not sure it is a goal per-say, it is just how the API was designed. Experimental Features are gotten and set on a WKPreferences, which is not a global object. "Experimental Features" are just fancy web preferences. If someone wants to create a new thing that is global, that could probably be done, that's just not what the "experimental features" feature is. 

Does this even work? Like, if
> you turn on WebGL 2.0 or WebGPU and then turn it off in a live webpage....
> what is the expectation?

This is a slightly different question. The answer depends on the preference. Most cause future attempts to use the feature not work (if you disable it after a page load), some, do other things like force a style recalc so they can be applied right away.

[Sam Weinig]

(In reply to Alex Christensen from comment #14)
> We need to have a way for experimental features to affect things in the
> network process.  I'm open to suggestions and willing to improve things.

If you would like the existing "experimental features" mechanism to affect functionality in the Network Process, the way to do that would have the preferences flow to the network process on a per-page basis, meaning the network process may have to have to operate in both modes at the same time, depending on which page is requesting action from the network process.

[Alex Christensen]

Right now reading experimental feature status seems to be implemented using NSUserDefaults which is UIProcess-global.  Are there plans to change that?

Since that is the case, I have no problem adding something to the yaml like exposed: [ NetworkProcess ] then having it only exposed in the network process.  Would you oppose something like that?

[Sam Weinig]

(In reply to Alex Christensen from comment #17)
> Right now reading experimental feature status seems to be implemented using
> NSUserDefaults which is UIProcess-global.  Are there plans to change that?

That is not how experimental features work, that is just a hack someone added to make it seemed like it worked for Safari's use case and happens to work due to the fact that safari syncs uses preferences synced to NSUserDefaults.

To see the actual implementation of experimental features, you can look in WebPreferences, tracing the calls from the SPI:

+ (NSArray<_WKExperimentalFeature *> *)_experimentalFeatures WK_API_AVAILABLE(macos(10.12), ios(10.0));
- (BOOL)_isEnabledForFeature:(_WKExperimentalFeature *)feature WK_API_AVAILABLE(macos(10.12), ios(10.0));
- (void)_setEnabled:(BOOL)value forFeature:(_WKExperimentalFeature *)feature WK_API_AVAILABLE(macos(10.12), ios(10.0));
- (BOOL)_isEnabledForExperimentalFeature:(_WKExperimentalFeature *)feature WK_API_AVAILABLE(macos(10.12), ios(10.0));
- (void)_setEnabled:(BOOL)value forExperimentalFeature:(_WKExperimentalFeature *)feature WK_API_AVAILABLE(macos(10.12), ios(10.0));


> Since that is the case, I have no problem adding something to the yaml like
> exposed: [ NetworkProcess ] then having it only exposed in the network
> process.  Would you oppose something like that?

I would oppose that for the reason stated above.

[Alex Christensen]

What about "exposed: [ WebsiteDataStore ]" in the yaml?

[Alex Christensen]

I guess instead of putting it in WebPreferencesExperimental.yaml it could go in WebsiteDataStoreExperimental.yaml

The reason I would prefer not to make SPI on WKWebsiteDataStore is that there are already two experimental features in the network process and I am aware of more on the way.  I think it would be cleaner to have a set of experimental features for the network process using similarly generated code.  I get that it should be separate from WebPreferencesExperimental.  Do you still dislike this?

[Sam Weinig]

(In reply to Alex Christensen from comment #20)
> I guess instead of putting it in WebPreferencesExperimental.yaml it could go
> in WebsiteDataStoreExperimental.yaml
> 
> The reason I would prefer not to make SPI on WKWebsiteDataStore is that
> there are already two experimental features in the network process and I am
> aware of more on the way.  I think it would be cleaner to have a set of
> experimental features for the network process using similarly generated
> code.  I get that it should be separate from WebPreferencesExperimental.  Do
> you still dislike this?

Again, and if I haven't made this clear, my apologies, I don't have any issue with this being done though the experimental features infrastructure as long as it is actually done via the experimental features infrastructure, which means making per-WebPreferences, not global as it currently is. This would likely mean piping the state preferences state with requests, but could also be implemented by other means. 

I also do not have any issue with having a new type of generated feature flags that are not based on WebPreferences.