[webkit-reviews] review granted: [Bug 98532] [Soup] Simplify the way that requests are started : [Attachment 167512] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 98532: [Soup] Simplify the way that requests are started
https://bugs.webkit.org/show_bug.cgi?id=98532

Attachment 167512: Patch
https://bugs.webkit.org/attachment.cgi?id=167512&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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?