94537 2012-08-20 15:32:09 -0700 Setting WebKitEnableHTTPPipelining doesn't work if default is true 2012-08-22 13:56:51 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 psolanki psolanki ap ddkilzer psolanki oldest_to_newest 700068 0 psolanki 2012-08-20 15:32:09 -0700 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. 700069 1 psolanki 2012-08-20 15:32:37 -0700 <rdar://problem/11831396> 700079 2 159542 psolanki 2012-08-20 15:37:24 -0700 Created attachment 159542 Patch 700096 3 159546 psolanki 2012-08-20 15:47:54 -0700 Created attachment 159546 Patch 700975 4 ap 2012-08-21 12:01:31 -0700 > 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? 701119 5 psolanki 2012-08-21 14:18:47 -0700 (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. 701132 6 ap 2012-08-21 14:35:49 -0700 Ah sorry, I didn't notice the lonely "!" sign in the expression. 702158 7 159546 ddkilzer 2012-08-22 12:06:16 -0700 Comment on attachment 159546 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); 702159 8 ddkilzer 2012-08-22 12:06:55 -0700 (In reply to comment #7) > This could be simplified to this (as mentioned in Comment #6): Err...Comment #5. 702303 9 psolanki 2012-08-22 13:56:51 -0700 Committed r126346: <http://trac.webkit.org/changeset/126346> 159542 2012-08-20 15:37:24 -0700 2012-08-20 15:47:52 -0700 Patch bug-94537-20120820153645.patch text/plain 2851 psolanki U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2MDY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZWIwYjZiMzgzNzNiYWQx MGZmMDQxYWMyZmQzYmFhMWNmMjFlYmZjMC4uNzAwZDdmZDk2YzJiMzAzNDYwY2VkNmNiMmM3ZGE3 OGE3MmVmNTNlOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEyLTA4LTIwICBQcmF0 aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KKworICAgICAgICBTZXR0aW5nIFdlYktp dEVuYWJsZUhUVFBQaXBlbGluaW5nIGRvZXNuJ3Qgd29yayBpZiBkZWZhdWx0IGlzIHRydWUKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk0NTM3CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9u ZXR3b3JrL2NmL1Jlc291cmNlUmVxdWVzdENGTmV0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Omlu aXRpYWxpemVNYXhpbXVtSFRUUENvbm5lY3Rpb25Db3VudFBlckhvc3QpOiBSZWFkIGluIHRoZSBI VFRQIHBpcGVsaW5pbmcgcHJlZiBhbmQKKyAgICAgICAgYWxsb3cgaXQgdG8gb3ZlcnJpZGUgdGhl IGRlZmF1bHQgcGlwZWxpbmluZyBiZWhhdmlvciBpZiBpdCB3YXMgYWN0dWFsbHkgc2V0LgorCiAy MDEyLTA4LTIwICBLZW4gQnVjaGFuYW4gIDxrZW5yYkBjaHJvbWl1bS5vcmc+CiAgICAgICAgIExp bmUgYm94ZXMgbm90IGJlaW5nIGRpcnRpZWQgY29ycmVjdGx5IGR1cmluZyBpbmxpbmUgcmVtb3Zh bAogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTMxNTYK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvY2YvUmVzb3VyY2VS ZXF1ZXN0Q0ZOZXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9SZXNv dXJjZVJlcXVlc3RDRk5ldC5jcHAKaW5kZXggNTIwZDY4NTc4ODRhNTk5ZmRkMDgzZDJlZmU0ZGZi NjhjN2UyNmQyOC4uMTkwOTlhNThlY2Y2NGJhMTg5YmQwMmY1MDkzM2NmY2FiYjUyYTFkYSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9SZXNvdXJjZVJlcXVl c3RDRk5ldC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9SZXNv dXJjZVJlcXVlc3RDRk5ldC5jcHAKQEAgLTI2NSwxNSArMjY1LDYgQEAgdm9pZCBSZXNvdXJjZVJl cXVlc3Q6OnNldEhUVFBQaXBlbGluaW5nRW5hYmxlZChib29sIGZsYWcpCiAgICAgc19odHRwUGlw ZWxpbmluZ0VuYWJsZWQgPSBmbGFnOwogfQogCi0jaWYgVVNFKENGTkVUV09SSykgfHwgUExBVEZP Uk0oTUFDKSAmJiBfX01BQ19PU19YX1ZFUlNJT05fTUlOX1JFUVVJUkVEID49IDEwNzAKLXN0YXRp YyBpbmxpbmUgYm9vbCByZWFkQm9vbGVhblByZWZlcmVuY2UoQ0ZTdHJpbmdSZWYga2V5KQotewot ICAgIEJvb2xlYW4ga2V5RXhpc3RzQW5kSGFzVmFsaWRGb3JtYXQ7Ci0gICAgQm9vbGVhbiByZXN1 bHQgPSBDRlByZWZlcmVuY2VzR2V0QXBwQm9vbGVhblZhbHVlKGtleSwga0NGUHJlZmVyZW5jZXND dXJyZW50QXBwbGljYXRpb24sICZrZXlFeGlzdHNBbmRIYXNWYWxpZEZvcm1hdCk7Ci0gICAgcmV0 dXJuIGtleUV4aXN0c0FuZEhhc1ZhbGlkRm9ybWF0ID8gcmVzdWx0IDogZmFsc2U7Ci19Ci0jZW5k aWYKLQogdW5zaWduZWQgaW5pdGlhbGl6ZU1heGltdW1IVFRQQ29ubmVjdGlvbkNvdW50UGVySG9z dCgpCiB7CiAgICAgc3RhdGljIGNvbnN0IHVuc2lnbmVkIHByZWZlcnJlZENvbm5lY3Rpb25Db3Vu dCA9IDY7CkBAIC0yODQsOCArMjc1LDEzIEBAIHVuc2lnbmVkIGluaXRpYWxpemVNYXhpbXVtSFRU UENvbm5lY3Rpb25Db3VudFBlckhvc3QoKQogI2lmIFVTRShDRk5FVFdPUkspIHx8IFBMQVRGT1JN KE1BQykgJiYgX19NQUNfT1NfWF9WRVJTSU9OX01JTl9SRVFVSVJFRCA+PSAxMDcwCiAgICAgc3Rh dGljIGNvbnN0IHVuc2lnbmVkIHVubGltaXRlZENvbm5lY3Rpb25Db3VudCA9IDEwMDAwOwogCi0g ICAgaWYgKCFSZXNvdXJjZVJlcXVlc3Q6Omh0dHBQaXBlbGluaW5nRW5hYmxlZCgpICYmIHJlYWRC b29sZWFuUHJlZmVyZW5jZShDRlNUUigiV2ViS2l0RW5hYmxlSFRUUFBpcGVsaW5pbmciKSkpCi0g ICAgICAgIFJlc291cmNlUmVxdWVzdDo6c2V0SFRUUFBpcGVsaW5pbmdFbmFibGVkKHRydWUpOwor ICAgIEJvb2xlYW4ga2V5RXhpc3RzQW5kSGFzVmFsaWRGb3JtYXQ7CisgICAgQm9vbGVhbiBwcmVm VmFsdWUgPSBDRlByZWZlcmVuY2VzR2V0QXBwQm9vbGVhblZhbHVlKENGU1RSKCJXZWJLaXRFbmFi bGVIVFRQUGlwZWxpbmluZyIpLCBrQ0ZQcmVmZXJlbmNlc0N1cnJlbnRBcHBsaWNhdGlvbiwgJmtl eUV4aXN0c0FuZEhhc1ZhbGlkRm9ybWF0KTsKKyAgICBib29sIHBpcGVsaW5pbmdFbmFibGVkID0g UmVzb3VyY2VSZXF1ZXN0OjpodHRwUGlwZWxpbmluZ0VuYWJsZWQoKTsKKyAgICBpZiAoa2V5RXhp c3RzQW5kSGFzVmFsaWRGb3JtYXQpCisgICAgICAgIHBpcGVsaW5pbmdFbmFibGVkID0gcHJlZlZh bHVlOworCisgICAgUmVzb3VyY2VSZXF1ZXN0OjpzZXRIVFRQUGlwZWxpbmluZ0VuYWJsZWQocGlw ZWxpbmluZ0VuYWJsZWQpOwogCiAgICAgaWYgKFJlc291cmNlUmVxdWVzdDo6aHR0cFBpcGVsaW5p bmdFbmFibGVkKCkpIHsKICAgICAgICAgd2tTZXRIVFRQUGlwZWxpbmluZ01heGltdW1Qcmlvcml0 eSh0b0hUVFBQaXBlbGluaW5nUHJpb3JpdHkoUmVzb3VyY2VMb2FkUHJpb3JpdHlIaWdoZXN0KSk7 Cg== 159546 2012-08-20 15:47:54 -0700 2012-08-22 12:06:16 -0700 Patch bug-94537-20120820154714.patch text/plain 2813 psolanki U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2MDY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZWIwYjZiMzgzNzNiYWQx MGZmMDQxYWMyZmQzYmFhMWNmMjFlYmZjMC4uNzAwZDdmZDk2YzJiMzAzNDYwY2VkNmNiMmM3ZGE3 OGE3MmVmNTNlOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEyLTA4LTIwICBQcmF0 aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KKworICAgICAgICBTZXR0aW5nIFdlYktp dEVuYWJsZUhUVFBQaXBlbGluaW5nIGRvZXNuJ3Qgd29yayBpZiBkZWZhdWx0IGlzIHRydWUKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk0NTM3CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9u ZXR3b3JrL2NmL1Jlc291cmNlUmVxdWVzdENGTmV0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Omlu aXRpYWxpemVNYXhpbXVtSFRUUENvbm5lY3Rpb25Db3VudFBlckhvc3QpOiBSZWFkIGluIHRoZSBI VFRQIHBpcGVsaW5pbmcgcHJlZiBhbmQKKyAgICAgICAgYWxsb3cgaXQgdG8gb3ZlcnJpZGUgdGhl IGRlZmF1bHQgcGlwZWxpbmluZyBiZWhhdmlvciBpZiBpdCB3YXMgYWN0dWFsbHkgc2V0LgorCiAy MDEyLTA4LTIwICBLZW4gQnVjaGFuYW4gIDxrZW5yYkBjaHJvbWl1bS5vcmc+CiAgICAgICAgIExp bmUgYm94ZXMgbm90IGJlaW5nIGRpcnRpZWQgY29ycmVjdGx5IGR1cmluZyBpbmxpbmUgcmVtb3Zh bAogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTMxNTYK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvY2YvUmVzb3VyY2VS ZXF1ZXN0Q0ZOZXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9SZXNv dXJjZVJlcXVlc3RDRk5ldC5jcHAKaW5kZXggNTIwZDY4NTc4ODRhNTk5ZmRkMDgzZDJlZmU0ZGZi NjhjN2UyNmQyOC4uMDEyNWY4ZWYxMWJjM2Q2NGFiYTBlYTJlNzNmZDEzOTE0NWVjNDk3NyAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9SZXNvdXJjZVJlcXVl c3RDRk5ldC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9jZi9SZXNv dXJjZVJlcXVlc3RDRk5ldC5jcHAKQEAgLTI2NSwxNSArMjY1LDYgQEAgdm9pZCBSZXNvdXJjZVJl cXVlc3Q6OnNldEhUVFBQaXBlbGluaW5nRW5hYmxlZChib29sIGZsYWcpCiAgICAgc19odHRwUGlw ZWxpbmluZ0VuYWJsZWQgPSBmbGFnOwogfQogCi0jaWYgVVNFKENGTkVUV09SSykgfHwgUExBVEZP Uk0oTUFDKSAmJiBfX01BQ19PU19YX1ZFUlNJT05fTUlOX1JFUVVJUkVEID49IDEwNzAKLXN0YXRp YyBpbmxpbmUgYm9vbCByZWFkQm9vbGVhblByZWZlcmVuY2UoQ0ZTdHJpbmdSZWYga2V5KQotewot ICAgIEJvb2xlYW4ga2V5RXhpc3RzQW5kSGFzVmFsaWRGb3JtYXQ7Ci0gICAgQm9vbGVhbiByZXN1 bHQgPSBDRlByZWZlcmVuY2VzR2V0QXBwQm9vbGVhblZhbHVlKGtleSwga0NGUHJlZmVyZW5jZXND dXJyZW50QXBwbGljYXRpb24sICZrZXlFeGlzdHNBbmRIYXNWYWxpZEZvcm1hdCk7Ci0gICAgcmV0 dXJuIGtleUV4aXN0c0FuZEhhc1ZhbGlkRm9ybWF0ID8gcmVzdWx0IDogZmFsc2U7Ci19Ci0jZW5k aWYKLQogdW5zaWduZWQgaW5pdGlhbGl6ZU1heGltdW1IVFRQQ29ubmVjdGlvbkNvdW50UGVySG9z dCgpCiB7CiAgICAgc3RhdGljIGNvbnN0IHVuc2lnbmVkIHByZWZlcnJlZENvbm5lY3Rpb25Db3Vu dCA9IDY7CkBAIC0yODQsOCArMjc1LDEwIEBAIHVuc2lnbmVkIGluaXRpYWxpemVNYXhpbXVtSFRU UENvbm5lY3Rpb25Db3VudFBlckhvc3QoKQogI2lmIFVTRShDRk5FVFdPUkspIHx8IFBMQVRGT1JN KE1BQykgJiYgX19NQUNfT1NfWF9WRVJTSU9OX01JTl9SRVFVSVJFRCA+PSAxMDcwCiAgICAgc3Rh dGljIGNvbnN0IHVuc2lnbmVkIHVubGltaXRlZENvbm5lY3Rpb25Db3VudCA9IDEwMDAwOwogCi0g ICAgaWYgKCFSZXNvdXJjZVJlcXVlc3Q6Omh0dHBQaXBlbGluaW5nRW5hYmxlZCgpICYmIHJlYWRC b29sZWFuUHJlZmVyZW5jZShDRlNUUigiV2ViS2l0RW5hYmxlSFRUUFBpcGVsaW5pbmciKSkpCi0g ICAgICAgIFJlc291cmNlUmVxdWVzdDo6c2V0SFRUUFBpcGVsaW5pbmdFbmFibGVkKHRydWUpOwor ICAgIEJvb2xlYW4ga2V5RXhpc3RzQW5kSGFzVmFsaWRGb3JtYXQ7CisgICAgQm9vbGVhbiBwcmVm VmFsdWUgPSBDRlByZWZlcmVuY2VzR2V0QXBwQm9vbGVhblZhbHVlKENGU1RSKCJXZWJLaXRFbmFi bGVIVFRQUGlwZWxpbmluZyIpLCBrQ0ZQcmVmZXJlbmNlc0N1cnJlbnRBcHBsaWNhdGlvbiwgJmtl eUV4aXN0c0FuZEhhc1ZhbGlkRm9ybWF0KTsKKyAgICBib29sIHBpcGVsaW5pbmdFbmFibGVkID0g a2V5RXhpc3RzQW5kSGFzVmFsaWRGb3JtYXQgPyBwcmVmVmFsdWUgOiBSZXNvdXJjZVJlcXVlc3Q6 Omh0dHBQaXBlbGluaW5nRW5hYmxlZCgpOworICAgIFJlc291cmNlUmVxdWVzdDo6c2V0SFRUUFBp cGVsaW5pbmdFbmFibGVkKHBpcGVsaW5pbmdFbmFibGVkKTsKIAogICAgIGlmIChSZXNvdXJjZVJl cXVlc3Q6Omh0dHBQaXBlbGluaW5nRW5hYmxlZCgpKSB7CiAgICAgICAgIHdrU2V0SFRUUFBpcGVs aW5pbmdNYXhpbXVtUHJpb3JpdHkodG9IVFRQUGlwZWxpbmluZ1ByaW9yaXR5KFJlc291cmNlTG9h ZFByaW9yaXR5SGlnaGVzdCkpOwo=