Bug 222945 – Preferences do not need to be passed to the WebProcess via WebPageCreationParameters since the store already is

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Preferences do not need to be passed to the WebProcess via WebPageCreationParameters since the store already is

[Sam Weinig]

Created attachment 422628 [details]
Patch

[Sam Weinig]

Created attachment 422707 [details]
Patch

[Alex Christensen]

Comment on attachment 422707 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:-7988
> -    parameters.httpsUpgradeEnabled = preferences().upgradeKnownHostsToHTTPSEnabled() ? m_configuration->httpsUpgradeEnabled() : false;

This is not equivalent to its replacement.  This uses the value from WKWebViewConfiguration only if the preference is on, which allows you to turn off the feature with the internal preference but by default it will use the value from WKWebViewConfiguration.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:554
> +    pageConfiguration->preferences()->setUpgradeKnownHostsToHTTPSEnabled(!![_configuration upgradeKnownHostsToHTTPS]);

This always uses the value from WKWebViewConfiguration, which Simon is about to discover breaks his internal performance benchmark.

[Sam Weinig]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 422707 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422707&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:-7988
> > -    parameters.httpsUpgradeEnabled = preferences().upgradeKnownHostsToHTTPSEnabled() ? m_configuration->httpsUpgradeEnabled() : false;
> 
> This is not equivalent to its replacement.  This uses the value from
> WKWebViewConfiguration only if the preference is on, which allows you to
> turn off the feature with the internal preference but by default it will use
> the value from WKWebViewConfiguration.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:554
> > +    pageConfiguration->preferences()->setUpgradeKnownHostsToHTTPSEnabled(!![_configuration upgradeKnownHostsToHTTPS]);
> 
> This always uses the value from WKWebViewConfiguration, which Simon is about
> to discover breaks his internal performance benchmark.

Ah right, this needs to follow the mediaDevicesEnabled model.

    if (pageConfiguration->preferences().upgradeKnownHostsToHTTPSEnabled())
        pageConfiguration->preferences()->setUpgradeKnownHostsToHTTPSEnabled(!![_configuration upgradeKnownHostsToHTTPS]);

We should probably do that for a few others.

I'll add a test for this.

[Sam Weinig]

The whole concept of having something on the configuration is supposed to mean it is immutable, but if you also make it a preference, things get really weird, and don't really make sense. We should really try to avoid that whenever we can.

In reality, what this "internal feature" really is is "force disable upgradeKnownHostsToHTTPS", so perhaps the cleanest way to model this would be with two preferences, one for mapping the configuration state, and one for mapping the preferences state, annoying as that is.

What a mess.

[Sam Weinig]

I filed https://bugs.webkit.org/show_bug.cgi?id=222999 to actually rationalize this stuff.

[Sam Weinig]

Created attachment 422775 [details]
Patch

[Sam Weinig]

This version is a bit scaled back while I work out a general solution for the configuration / preference fiasco.

[EWS]

Committed r274192: <https://commits.webkit.org/r274192>

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

[Radar WebKit Bug Importer]

<rdar://problem/75246653>