Bug 94537 – Setting WebKitEnableHTTPPipelining doesn't work if default is true

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

The code does

    if (!ResourceRequest::httpPipeliningEnabled() && readBooleanPreference(CFSTR("WebKitEnableHTTPPipelining")))
        ResourceRequest::setHTTPPipeliningEnabled(true);

if ResourceRequest::httpPipeliningEnabled() is set to true by default, then we never read the pref and never enable/disable pipelining per the pref.

[Pratik Solanki]

<rdar://problem/11831396>

[Pratik Solanki]

Created attachment 159542 [details]
Patch

[Pratik Solanki]

Created attachment 159546 [details]
Patch

[Alexey Proskuryakov]

> if ResourceRequest::httpPipeliningEnabled() is set to true by default, then we never read the pref and never enable/disable pipelining per the pref.

Did you mean "if...set to false"?

This code looks like it's intentional, i.e. one should both set the preference, and call an API method to enable pipelining. Do you think that we now want to change that, or that the code was never actually meant to do that?

[Pratik Solanki]

(In reply to comment #4)
> > if ResourceRequest::httpPipeliningEnabled() is set to true by default, then we never read the pref and never enable/disable pipelining per the pref.
> 
> Did you mean "if...set to false"?
> 
> This code looks like it's intentional, i.e. one should both set the preference, and call an API method to enable pipelining. Do you think that we now want to change that, or that the code was never actually meant to do that?

The code, as written, would only check the pref value if ResourceRequest::s_httpPipeliningEnabled was initialized to false. If it was initialized to true (e.g. to enable pipelining by default), then we couldn't use the pref to disable pipelining. The patch ensures that the pref, if present, overrides the default value in the file.

Looking at the code some more, I guess if the pref is not present, there isn't a need to call setHTTPPipeliningEnabled(). It will just use the default in that case.

[Alexey Proskuryakov]

Ah sorry, I didn't notice the lonely "!" sign in the expression.

[David Kilzer (:ddkilzer)]

Comment on attachment 159546 [details]
Patch

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

r=me with comments addressed.

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:278
> +    Boolean keyExistsAndHasValidFormat;

Nit:  Initialize to False.

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:281
> +    bool pipeliningEnabled = keyExistsAndHasValidFormat ? prefValue : ResourceRequest::httpPipeliningEnabled();
> +    ResourceRequest::setHTTPPipeliningEnabled(pipeliningEnabled);

This could be simplified to this (as mentioned in Comment #6):

    if (keyExistsAndHasValidFormat)
        ResourceRequest::setHTTPPipeliningEnabled(prefValue);

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> This could be simplified to this (as mentioned in Comment #6):

Err...Comment #5.

[Pratik Solanki]

Committed r126346: <http://trac.webkit.org/changeset/126346>