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.
<rdar://problem/11831396>
Created attachment 159542 [details] Patch
Created attachment 159546 [details] Patch
> 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?
(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.
Ah sorry, I didn't notice the lonely "!" sign in the expression.
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);
(In reply to comment #7) > This could be simplified to this (as mentioned in Comment #6): Err...Comment #5.
Committed r126346: <http://trac.webkit.org/changeset/126346>