It should be updated too with the resource request URL.
Created attachment 192508 [details] Patch
Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing.
(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).
(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).
(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.
I'd rather this be part of the patch that requires it.
Comment on attachment 192508 [details] Patch Clearing flags on attachment: 192508 Committed r145376: <http://trac.webkit.org/changeset/145376>
All reviewed patches have been landed. Closing bug.
(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. :(
(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.
(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.
(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.
(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.
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 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. :)