99478 2012-10-16 10:42:49 -0700 Init timeout flag in ResourceErrorMac 2012-10-16 14:05:24 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 74802 1 d-r d-r ap webkit.review.bot oldest_to_newest 743282 0 d-r 2012-10-16 10:42:49 -0700 The timeout value is not initialized from NSError immediately. Since errorCode is in lazyInit, we can initialize the timeout flag there too. 743302 1 168974 d-r 2012-10-16 10:53:58 -0700 Created attachment 168974 Patch 743309 2 168974 ap 2012-10-16 11:01:45 -0700 Comment on attachment 168974 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168974&action=review r-, because this breaks setIsTimeout(). > Source/WebCore/platform/network/ResourceErrorBase.h:50 > bool isCancellation() const { return m_isCancellation; } Notably, we don't need lazyInit() here. > Source/WebCore/platform/network/ResourceErrorBase.h:53 > void setIsTimeout(bool isTimeout) { m_isTimeout = isTimeout; } > - bool isTimeout() const { return m_isTimeout; } > + bool isTimeout() const { lazyInit(); return m_isTimeout; } This means that even after a setIsTimeout(true) call, isTimeout() might return false. That's not an intuitively correct behavior. 743456 3 169007 d-r 2012-10-16 13:07:37 -0700 Created attachment 169007 Patch 743461 4 d-r 2012-10-16 13:11:29 -0700 (In reply to comment #2) > (From update of attachment 168974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168974&action=review > > r-, because this breaks setIsTimeout(). Thanks for the quick review. > > Source/WebCore/platform/network/ResourceErrorBase.h:50 > > bool isCancellation() const { return m_isCancellation; } > > Notably, we don't need lazyInit() here. Yes, the difference with isTimeout/setIsTimeout is that the true value can also come from the network side, not only from the webcore side. So how about an immediate initialization in the constructor? 743473 5 169007 ap 2012-10-16 13:22:58 -0700 Comment on attachment 169007 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169007&action=review OK. There are still weird cases remaining (like what happens when one calls setIsTimeout() and then round-trips the error object via NSError or WebKit2 IPC), but that's true of many things in ResourceError class. > Source/WebCore/platform/network/mac/ResourceErrorMac.mm:99 > + if (!m_isNull) > + m_isTimeout = [m_platformError.get() code] == NSURLErrorTimedOut; The null check is not strictly necessary - sending messages to null receivers generally works in Objective-C (unconditionally works when returned type is an integer). 743539 6 169007 webkit.review.bot 2012-10-16 14:05:20 -0700 Comment on attachment 169007 Patch Clearing flags on attachment: 169007 Committed r131499: <http://trac.webkit.org/changeset/131499> 743540 7 webkit.review.bot 2012-10-16 14:05:24 -0700 All reviewed patches have been landed. Closing bug. 168974 2012-10-16 10:53:58 -0700 2012-10-16 13:07:32 -0700 Patch bug-99478-20121016205243.patch text/plain 2541 d-r U3VidmVyc2lvbiBSZXZpc2lvbjogMTMxNDEzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYzhlZTg4MTQ0OGY5ODhl OTdmOGU0MWRiNzIyODEyMjk0OGMxOWE3OS4uZTY2MGE2NWNjNDU2MjRkZTYxNjBlNWEwMzgzZjM2 YWE5NDdhNWZmNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDEyLTEwLTE2ICBEb21p bmlrIFLDtnR0c2NoZXMgIDxkb21pbmlrLnJvdHRzY2hlc0BpbnRlbC5jb20+CisKKyAgICAgICAg TGF6eSBpbml0IHRpbWVvdXQgZmxhZyBpbiBSZXNvdXJjZUVycm9yTWFjCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05OTQ3OAorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBtX2Vycm9yQ29kZSBpcyBvbmx5 IGluaXRpYWxpemVkIGhlcmUsIHNvIGl0IG1ha2VzIHNlbnNlIHRvIAorICAgICAgICBwdXQgdGhl IHRpbWVvdXQgZmxhZyBpbml0aWFsaXphdGlvbiBoZXJlLCB0b28uIFRoaXMgaXMgdGhlIG1pc3Np bmcgCisgICAgICAgIGxpbmsgZm9yIGdldHRpbmcgdGhlIFhIUiB0aW1lb3V0IGNvZGUgdG8gd29y ayBvbiBtYWMuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBjb3ZlcmVkIGJ5IHRlc3RzIHRoYXQg d2lsbCBiZSBhZGRlZCBpbiBidWcgNzQ4MDIuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3Jr L1Jlc291cmNlRXJyb3JCYXNlLmg6CisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZUVycm9yQmFz ZTo6aXNUaW1lb3V0KTogQ2FsbGluZyBsYXp5SW5pdCgpLgorICAgICAgICAqIHBsYXRmb3JtL25l dHdvcmsvbWFjL1Jlc291cmNlRXJyb3JNYWMubW06CisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJj ZUVycm9yOjpwbGF0Zm9ybUxhenlJbml0KTogSW5pdGlhbGl6aW5nIHRpbWVvdXQgZmxhZyBmcm9t IGVycm9yIGNvZGUuCisKIDIwMTItMTAtMTUgIE1pa2UgV2VzdCAgPG1rd3N0QGNocm9taXVtLm9y Zz4KIAogICAgICAgICBXYXJuIHdoZW4gQ1NQIGhlYWRlcnMgZG9uJ3Qgc2VwYXJhdGUgZGlyZWN0 aXZlcyB3aXRoICc7Jy4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdv cmsvUmVzb3VyY2VFcnJvckJhc2UuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsv UmVzb3VyY2VFcnJvckJhc2UuaAppbmRleCA1Yjc5MDlmZGIxZGUwMWQ2NTMzMWM3NTk5NGY5Mzhj MzczZjI3MDVjLi43MWQzMTBkMTU5MGIzN2M1MDY1ZjQ0ODZjZjE3MDE2OWQ1NGM0Mzk3IDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL1Jlc291cmNlRXJyb3JCYXNl LmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9SZXNvdXJjZUVycm9yQmFz ZS5oCkBAIC01MCw3ICs1MCw3IEBAIHB1YmxpYzoKICAgICBib29sIGlzQ2FuY2VsbGF0aW9uKCkg Y29uc3QgeyByZXR1cm4gbV9pc0NhbmNlbGxhdGlvbjsgfQogCiAgICAgdm9pZCBzZXRJc1RpbWVv dXQoYm9vbCBpc1RpbWVvdXQpIHsgbV9pc1RpbWVvdXQgPSBpc1RpbWVvdXQ7IH0KLSAgICBib29s IGlzVGltZW91dCgpIGNvbnN0IHsgcmV0dXJuIG1faXNUaW1lb3V0OyB9CisgICAgYm9vbCBpc1Rp bWVvdXQoKSBjb25zdCB7IGxhenlJbml0KCk7IHJldHVybiBtX2lzVGltZW91dDsgfQogCiAgICAg c3RhdGljIGJvb2wgY29tcGFyZShjb25zdCBSZXNvdXJjZUVycm9yJiwgY29uc3QgUmVzb3VyY2VF cnJvciYpOwogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21h Yy9SZXNvdXJjZUVycm9yTWFjLm1tIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9t YWMvUmVzb3VyY2VFcnJvck1hYy5tbQppbmRleCA0MTdlOTA1YTNiYmViNGE0ZTBiZWVkOTkzOWI2 ZmNjOTRlNzc0NzI3Li5kZDUyNzBhOWRmNzM2OWFjZWRkZGFhM2RkNGFlMGI0MTA1MzVkZTQxIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZUVy cm9yTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvbWFjL1Jlc291 cmNlRXJyb3JNYWMubW0KQEAgLTExMSw2ICsxMTEsOCBAQCB2b2lkIFJlc291cmNlRXJyb3I6OnBs YXRmb3JtTGF6eUluaXQoKQogCiAgICAgbV9kb21haW4gPSBbbV9wbGF0Zm9ybUVycm9yLmdldCgp IGRvbWFpbl07CiAgICAgbV9lcnJvckNvZGUgPSBbbV9wbGF0Zm9ybUVycm9yLmdldCgpIGNvZGVd OworICAgIAorICAgIHNldElzVGltZW91dChtX2Vycm9yQ29kZSA9PSBOU1VSTEVycm9yVGltZWRP dXQpOwogCiAgICAgTlNTdHJpbmcqIGZhaWxpbmdVUkxTdHJpbmcgPSBbW21fcGxhdGZvcm1FcnJv ci5nZXQoKSB1c2VySW5mb10gdmFsdWVGb3JLZXk6QCJOU0Vycm9yRmFpbGluZ1VSTFN0cmluZ0tl eSJdOwogICAgIGlmICghZmFpbGluZ1VSTFN0cmluZykK 169007 2012-10-16 13:07:37 -0700 2012-10-16 14:05:20 -0700 Patch bug-99478-20121016230623.patch text/plain 1904 d-r U3VidmVyc2lvbiBSZXZpc2lvbjogMTMxNDgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDgyOWY1Y2U2MWIxYTE3 YzYxZjRkNDM0NGM3ZTg1NjM3YTZlOGUzOC4uY2UwMTU5ZTg1YjE1Nzg1M2UxMTg0NDUxNzA5MjRh NTA1ZmNlZjhmMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTEwLTE2ICBEb21p bmlrIFLDtnR0c2NoZXMgIDxkb21pbmlrLnJvdHRzY2hlc0BpbnRlbC5jb20+CisKKyAgICAgICAg SW5pdCB0aW1lb3V0IGZsYWcgaW4gUmVzb3VyY2VFcnJvck1hYworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTk0NzgKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBPbiBjb25zdHJ1Y3Rpb24gZnJvbSBwbGF0Zm9y bSBlcnJvciB0eXBlcywgaW5pdGlhbGl6ZSB0aGUgdGltZW91dCBmbGFnIAorICAgICAgICB0byB0 cnVlIGlmIHRoZSBlcnJvciByZXN1bHRzIGZyb20gYSB0aW1lb3V0LgorCisgICAgICAgIE5vIG5l dyB0ZXN0cywgY292ZXJlZCBieSB0ZXN0cyB0aGF0IHdpbGwgYmUgYWRkZWQgaW4gYnVnIDc0ODAy LgorCisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9tYWMvUmVzb3VyY2VFcnJvck1hYy5tbToK KyAgICAgICAgKFdlYkNvcmU6OlJlc291cmNlRXJyb3I6OnBsYXRmb3JtTGF6eUluaXQpOiBJbml0 aWFsaXppbmcgdGltZW91dCBmbGFnIGlmIHRoZSBlcnJvciB3YXMgYSB0aW1lb3V0LgorCiAyMDEy LTEwLTE2ICBTaW1vbiBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgogCiAgICAgICAg IFJFR1JFU1NJT04gKHIxMjg3ODcpOiBGaXhlZCBwb3NpdGlvbiBkaXYgY2F1c2VzIG90aGVyIGVs ZW1lbnRzIHRvIG5vdCB1cGRhdGUgY29ycmVjdGx5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZUVycm9yTWFjLm1tIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vbmV0d29yay9tYWMvUmVzb3VyY2VFcnJvck1hYy5tbQppbmRleCA0MTdlOTA1 YTNiYmViNGE0ZTBiZWVkOTkzOWI2ZmNjOTRlNzc0NzI3Li5kZjZlOTg2OGFkMjNjY2FiOTJlOGJj MGMzOGRjYWJjZDA2MGExMzY0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9u ZXR3b3JrL21hYy9SZXNvdXJjZUVycm9yTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL25ldHdvcmsvbWFjL1Jlc291cmNlRXJyb3JNYWMubW0KQEAgLTk1LDYgKzk1LDggQEAgUmVz b3VyY2VFcnJvcjo6UmVzb3VyY2VFcnJvcihOU0Vycm9yICpuc0Vycm9yKQogICAgICwgbV9wbGF0 Zm9ybUVycm9yKG5zRXJyb3IpCiB7CiAgICAgbV9pc051bGwgPSAhbnNFcnJvcjsKKyAgICBpZiAo IW1faXNOdWxsKQorICAgICAgICBtX2lzVGltZW91dCA9IFttX3BsYXRmb3JtRXJyb3IuZ2V0KCkg Y29kZV0gPT0gTlNVUkxFcnJvclRpbWVkT3V0OwogfQogCiBSZXNvdXJjZUVycm9yOjpSZXNvdXJj ZUVycm9yKENGRXJyb3JSZWYgY2ZFcnJvcikKQEAgLTEwMiw2ICsxMDQsOCBAQCBSZXNvdXJjZUVy cm9yOjpSZXNvdXJjZUVycm9yKENGRXJyb3JSZWYgY2ZFcnJvcikKICAgICAsIG1fcGxhdGZvcm1F cnJvcigoTlNFcnJvciAqKWNmRXJyb3IpCiB7CiAgICAgbV9pc051bGwgPSAhY2ZFcnJvcjsKKyAg ICBpZiAoIW1faXNOdWxsKQorICAgICAgICBtX2lzVGltZW91dCA9IFttX3BsYXRmb3JtRXJyb3Iu Z2V0KCkgY29kZV0gPT0gTlNVUkxFcnJvclRpbWVkT3V0OwogfQogCiB2b2lkIFJlc291cmNlRXJy b3I6OnBsYXRmb3JtTGF6eUluaXQoKQo=