Bug 98532 – [Soup] Simplify the way that requests are started

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Martin Robinson]

Right now there's a bit of duplicated code in the Soup backend that could be eliminated through abstraction. This is part of a larger effort to make the soup backend more hackable.

[Martin Robinson]

Created attachment 167472 [details]
Patch

[Martin Robinson]

Created attachment 167512 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 167512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167512&action=review

Looks good, just concerned about the change in behaviour for the ref/deref, and wondering about isDestroying, which seems to always be false.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:823
> +        d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(),
> +            d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this);

This indentation looks correct, but we have some broken-up parameters in this file that align with the first argument (so just after the open paren). The coding style doesn't seem to say anything explicitly, but the one example it has goes with your way here, something to keep in mind for the future I guess.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:827
> +    // balanced by a deref() in cleanupSoupRequestOperation, which should always run
> +    ref();

This has changed - it will now only ref for requests which are effectively sent, but cleanupSoupRequestOperation calls deref if isDestroying is false, which it seems to always be, from the looks of it?

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 167512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167512&action=review
> 
> Looks good, just concerned about the change in behaviour for the ref/deref, and wondering about isDestroying, which seems to always be false.
> 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:823
> > +        d->m_timeoutSource = soup_add_timeout(g_main_context_get_thread_default(),
> > +            d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this);
> 
> This indentation looks correct, but we have some broken-up parameters in this file that align with the first argument (so just after the open paren). The coding style doesn't seem to say anything explicitly, but the one example it has goes with your way here, something to keep in mind for the future I guess.

The style bot recently started complaining about indentation that aligns with the parenthesis. This is because Darin Adler said this style isn't acceptable. I find the change to be pretty odd, considering there is so much code where we align the argument, but I'm trying to roll with it. :)

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:827
> > +    // balanced by a deref() in cleanupSoupRequestOperation, which should always run
> > +    ref();
> 
> This has changed - it will now only ref for requests which are effectively sent, but cleanupSoupRequestOperation calls deref if isDestroying is false, which it seems to always be, from the looks of it?

So cleanupSoupRequestOperation is called with isDestroying == true in ~ResourceHandle. It makes sense that it shouldn't unref the handle when called from the destructor, since the destructor should only be called when the reference count is already zero. From what I can tell, my change shouldn't cause a problem because cleanupSoupRequestOperation should only called if the request is actually sent.

[Martin Robinson]

Committed r130699: <http://trac.webkit.org/changeset/130699>

[Laszlo Gombos]

Debug build fails with the following error:

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp: In function ‘bool WebCore::createSoupMessageForHandleAndRequest(WebCore::ResourceHandle*, const WebCore::ResourceRequest&)’:

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:690:8: error: ‘d’ was not declared in this scope

[Martin Robinson]

Landed a fix here: http://trac.webkit.org/changeset/130704