Bug 112040 – [SOUP] ResourceRequest::updateSoupMessage doesn't update the URI of the soup message

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It should be updated too with the resource request URL.

[Carlos Garcia Campos]

Created attachment 192508 [details]
Patch

[Martin Robinson]

Does this break something or just for completeness? This code is really fragile, so I wouldn't change it without a new test passing.

[Martin Robinson]

(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).

[Carlos Garcia Campos]

(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).

[Carlos Garcia Campos]

(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.

[Martin Robinson]

I'd rather this be part of the patch that requires it.

[WebKit Review Bot]

Comment on attachment 192508 [details]
Patch

Clearing flags on attachment: 192508

Committed r145376: <http://trac.webkit.org/changeset/145376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(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. :(

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Gustavo Noronha (kov)]

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.

[Martin Robinson]

(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. :)