112040 2013-03-11 10:55:06 -0700 [SOUP] ResourceRequest::updateSoupMessage doesn't update the URI of the soup message 2013-03-11 16:17:14 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Gtk, Soup P2 Normal --- 1 cgarcia webkit-unassigned cdumez danw gustavo mrobinson rakuco svillar webkit.review.bot oldest_to_newest 852467 0 cgarcia 2013-03-11 10:55:06 -0700 It should be updated too with the resource request URL. 852469 1 192508 cgarcia 2013-03-11 10:57:17 -0700 Created attachment 192508 Patch 852482 2 mrobinson 2013-03-11 11:10:30 -0700 Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing. 852486 3 mrobinson 2013-03-11 11:13:52 -0700 (In reply to comment #2) > Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing. And really what we should be doing here is switching to making the ResourceRequest "live" in the sense that the SoupMessage and SoupRequest are updated transparently, ala the other ResourceHandle implementations (see updatePlatformRequest and updateResourceRequest). 852491 4 cgarcia 2013-03-11 11:16:50 -0700 (In reply to comment #2) > Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing. I think this hasn't been noticed before because in most of the cases the soup message has been created with a URI that is still valid after calling updateSoupMessage. I'm currently implementing SoupMessageHeaders *webkit_uri_request_get_http_header() in WebKit2, and I've switched the WebKitURIRequest to wrap a SoupMssage. When the WebKitURIRequest is created from a ResourceRequest the URI is not updated, and since it's a construct property, the default value remains (about:blank). 852492 5 cgarcia 2013-03-11 11:17:36 -0700 (In reply to comment #3) > (In reply to comment #2) > > Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing. > > And really what we should be doing here is switching to making the ResourceRequest "live" in the sense that the SoupMessage and SoupRequest are updated transparently, ala the other ResourceHandle implementations (see updatePlatformRequest and updateResourceRequest). That's for a different bug, I'd say. 852501 6 mrobinson 2013-03-11 11:25:25 -0700 I'd rather this be part of the patch that requires it. 852502 7 192508 webkit.review.bot 2013-03-11 11:26:32 -0700 Comment on attachment 192508 Patch Clearing flags on attachment: 192508 Committed r145376: <http://trac.webkit.org/changeset/145376> 852503 8 webkit.review.bot 2013-03-11 11:26:36 -0700 All reviewed patches have been landed. Closing bug. 852515 9 mrobinson 2013-03-11 11:36:13 -0700 (In reply to comment #8) > All reviewed patches have been landed. Closing bug. I'm a little unhappy that this landed without really finishing the discussion about it. :( 852516 10 mrobinson 2013-03-11 11:36:34 -0700 (In reply to comment #9) > (In reply to comment #8) > > All reviewed patches have been landed. Closing bug. > > I'm a little unhappy that this landed without really finishing the discussion about it. :( Just unfortunate timing with the comments, I guess. Don't mean to blame anyone. 852518 11 mrobinson 2013-03-11 11:40:24 -0700 (In reply to comment #10) > Just unfortunate timing with the comments, I guess. Don't mean to blame anyone. So my objection to this is follows: Right now it seems that updateSoupMessage is only called in one place and that's directly after creating the SoupMessage. In this case the SoupMessage will have the URL that it was created with. updateSoupMessage is not meant to be called currently at any other moment. Thus, there's no bug without the other piece of this patch which calls it at a different moment. Thus, this patch just added effectively redundant code that doesn't make sense except in tandem with the next piece. 852522 12 cgarcia 2013-03-11 11:42:33 -0700 (In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > All reviewed patches have been landed. Closing bug. > > > > I'm a little unhappy that this landed without really finishing the discussion about it. :( > > Just unfortunate timing with the comments, I guess. Don't mean to blame anyone. We can just roll it out and continue discussing it. 852532 13 cgarcia 2013-03-11 11:49:52 -0700 (In reply to comment #11) > (In reply to comment #10) > > > Just unfortunate timing with the comments, I guess. Don't mean to blame anyone. > > So my objection to this is follows: > > Right now it seems that updateSoupMessage is only called in one place and that's directly after creating the SoupMessage. In this case the SoupMessage will have the URL that it was created with. updateSoupMessage is not meant to be called currently at any other moment. Thus, there's no bug without the other piece of this patch which calls it at a different moment. Thus, this patch just added effectively redundant code that doesn't make sense except in tandem with the next piece. The HTTP method is also set already in the message and updated in updateSoupMessage, maybe I understood wrong the name of the method, I though it was to update the soupMessage with the resource request info, not only with part of them. 852828 14 gustavo 2013-03-11 16:10:36 -0700 Sorry, I had not seen your comments Martin, I had the patch review window open for a little while and it seemed pretty straight-forward - I thought indeed it was something to be used in another patch, but I see your point, I wouldn't mind reverting and making it part of the other patch instead. 852839 15 mrobinson 2013-03-11 16:17:14 -0700 (In reply to comment #14) > Sorry, I had not seen your comments Martin, I had the patch review window open for a little while and it seemed pretty straight-forward - I thought indeed it was something to be used in another patch, but I see your point, I wouldn't mind reverting and making it part of the other patch instead. In the end this will either be useful in the future or harmless. I'm going to spend some time trying to remove updateSoupMessage and updateFromSoupMessage entirely. We can try to remove the code then. I don't want to demand lots of code churn this close to the release. :) 192508 2013-03-11 10:57:17 -0700 2013-03-11 11:26:32 -0700 Patch soup-message-uri.diff text/plain 1464 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA0NGExYjhjLi5hZTI0YmRjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTMtMDMtMTEgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFtTT1VQXSBSZXNvdXJjZVJlcXVlc3Q6OnVwZGF0ZVNvdXBNZXNzYWdlIGRv ZXNuJ3QgdXBkYXRlIHRoZSBVUkkgb2YgdGhlIHNvdXAgbWVzc2FnZQorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTEyMDQwCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL3NvdXAv UmVzb3VyY2VSZXF1ZXN0U291cC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZVJlcXVl c3Q6OnVwZGF0ZVNvdXBNZXNzYWdlKTogVXBkYXRlIHRoZSBzb3VwCisgICAgICAgIG1lc3NhZ2Ug VVJJIHdpdGggdGhlIFJlc291cmNlUmVxdWVzdCBVUkwuCisKIDIwMTMtMDMtMDcgIFphbiBEb2Jl cnNlayAgPHpkb2JlcnNla0BpZ2FsaWEuY29tPgogCiAgICAgICAgIFtHVEtdIExpbWl0IHRoZSBz dXBwb3J0ZWQgY29tcGlsZXJzIHRvIEdDQyA+PSA0LjcgYW5kIENsYW5nID49IDMuMApkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlUmVxdWVz dFNvdXAuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNl UmVxdWVzdFNvdXAuY3BwCmluZGV4IDllZmI0N2IuLjNjMGNjNjEgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZVJlcXVlc3RTb3VwLmNwcAor KysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VSZXF1ZXN0 U291cC5jcHAKQEAgLTM4LDYgKzM4LDkgQEAgdm9pZCBSZXNvdXJjZVJlcXVlc3Q6OnVwZGF0ZVNv dXBNZXNzYWdlKFNvdXBNZXNzYWdlKiBzb3VwTWVzc2FnZSkgY29uc3QKIHsKICAgICBnX29iamVj dF9zZXQoc291cE1lc3NhZ2UsIFNPVVBfTUVTU0FHRV9NRVRIT0QsIGh0dHBNZXRob2QoKS51dGY4 KCkuZGF0YSgpLCBOVUxMKTsKIAorICAgIEdPd25QdHI8U291cFVSST4gdXJpKHNvdXBVUkkoKSk7 CisgICAgc291cF9tZXNzYWdlX3NldF91cmkoc291cE1lc3NhZ2UsIHVyaS5nZXQoKSk7CisKICAg ICBjb25zdCBIVFRQSGVhZGVyTWFwJiBoZWFkZXJzID0gaHR0cEhlYWRlckZpZWxkcygpOwogICAg IFNvdXBNZXNzYWdlSGVhZGVycyogc291cEhlYWRlcnMgPSBzb3VwTWVzc2FnZS0+cmVxdWVzdF9o ZWFkZXJzOwogICAgIGlmICghaGVhZGVycy5pc0VtcHR5KCkpIHsK