[webkit-reviews] review granted: [Bug 94796] [soup] Obey setTimeoutInterval in soup backend : [Attachment 163075] setTimeoutInterval support for soup, clear conditionally v6.

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Dominik Röttsches (drott)
<dominik.rottsches at intel.com>'s request for review:
Bug 94796: [soup] Obey setTimeoutInterval in soup backend
https://bugs.webkit.org/show_bug.cgi?id=94796

Attachment 163075: setTimeoutInterval support for soup, clear conditionally v6.
https://bugs.webkit.org/attachment.cgi?id=163075&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163075&action=review


Looks good to me, cq- because of the comments.

> Source/WebCore/ChangeLog:10
> +	   has been successfully tested in combination with with my
work-in-progress

with with

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:389
> +	   g_source_destroy(d->m_timeoutSource.get());
> +	   d->m_timeoutSource.clear();

So, what happens if a request times out? If I'm reading the patch right the
callback is going to be called, which will call this function. Then the source
will be destroyed (thus removed from the context and unrefed), then cleared
here which will cause another unref, and then the callback will return FALSE,
also causing it to be removed again. It looks like glib protects us from this
case by checking whether the source has already been destroyed, though, so
shouldn't be a problem... ok

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:768
> +	       // soup_add_timeout returns a non-owned GSource*, not using
adoptRef.

I think it would be better to be a bit more descriptive here, saying that
soup_add_timeout returns a GSource whose only reference is owned by the
context, so we need an additional one here.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:855
> +    // Except canceling a possible timeout timer, we only need to take
action here to UN-defer loading.

canceling -> when canceling

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:874
> +	   if (d->m_firstRequest.timeoutInterval() > 0)
> +	       // soup_add_timeout returns a non-owned GSource*, not using
adoptRef.
> +	       d->m_timeoutSource =
soup_add_timeout(g_main_context_get_thread_default(),
d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, this);

Please add braces to the if, so it's easier to read.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1001
> +    // Error domain is identical for ErrorsGtk and ErrorsEfl.

I think this comment isn't very useful, I'd take it out.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1007
> +    // Do not run this callback again.

Same here, this is pretty standard glib, so no need to be explicit.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1041
> +	   if (d->m_firstRequest.timeoutInterval() > 0)
> +	       // soup_add_timeout returns a non-owned GSource*, not using
adoptRef.
> +	       d->m_timeoutSource =
soup_add_timeout(g_main_context_get_thread_default(),
d->m_firstRequest.timeoutInterval() * 1000, requestTimeoutCallback, handle);

Ditto.