Bug 112040

Summary: [SOUP] ResourceRequest::updateSoupMessage doesn't update the URI of the soup message
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, danw, gns, mrobinson, rakuco, svillar, webkit.review.bot
Priority: P2 Keywords: Gtk, Soup
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Carlos Garcia Campos 2013-03-11 10:55:06 PDT
It should be updated too with the resource request URL.
Comment 1 Carlos Garcia Campos 2013-03-11 10:57:17 PDT
Created attachment 192508 [details]
Patch
Comment 2 Martin Robinson 2013-03-11 11:10:30 PDT
Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing.
Comment 3 Martin Robinson 2013-03-11 11:13:52 PDT
(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).
Comment 4 Carlos Garcia Campos 2013-03-11 11:16:50 PDT
(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).
Comment 5 Carlos Garcia Campos 2013-03-11 11:17:36 PDT
(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.
Comment 6 Martin Robinson 2013-03-11 11:25:25 PDT
I'd rather this be part of the patch that requires it.
Comment 7 WebKit Review Bot 2013-03-11 11:26:32 PDT
Comment on attachment 192508 [details]
Patch

Clearing flags on attachment: 192508

Committed r145376: <http://trac.webkit.org/changeset/145376>
Comment 8 WebKit Review Bot 2013-03-11 11:26:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Martin Robinson 2013-03-11 11:36:13 PDT
(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. :(
Comment 10 Martin Robinson 2013-03-11 11:36:34 PDT
(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.
Comment 11 Martin Robinson 2013-03-11 11:40:24 PDT
(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.
Comment 12 Carlos Garcia Campos 2013-03-11 11:42:33 PDT
(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.
Comment 13 Carlos Garcia Campos 2013-03-11 11:49:52 PDT
(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.
Comment 14 Gustavo Noronha (kov) 2013-03-11 16:10:36 PDT
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.
Comment 15 Martin Robinson 2013-03-11 16:17:14 PDT
(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. :)