[webkit-reviews] review denied: [Bug 72227] [GTK] improve platformSetDefersLoading : [Attachment 114861] Fix platformDefersLoading

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Dan Winship
<danw at gnome.org>'s request for review:
Bug 72227: [GTK] improve platformSetDefersLoading
https://bugs.webkit.org/show_bug.cgi?id=72227

Attachment 114861: Fix platformDefersLoading
https://bugs.webkit.org/attachment.cgi?id=114861&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114861&action=review


I really like how this patch removes a lot of hacks for using
soup_session_(un)pause_message. A couple comments follow.

> Source/WebCore/ChangeLog:9
> +	   No new tests. (OOPS!)

You'll need to remove the OOPS here for the cq to land this patch. You can just
say something like, "No new tests. This is covered by existing tests."

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:564
>	   return;

It seems like there's nothing now to do in this method if defersLoading ==
true? If so, you can do an early return right away and avoid all the other
checking later.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:567
> +    // Requests that haven't yet been sent
> +    if (!d->m_cancellable) {

I think it makes sense to actually remove the comment and add a helper that
makes this check explicit. You might do something like static bool
requestStarted(...). It makes sense to have a canonical idea of when the
request has started.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:578
> +    // Do not pause or unpause operations that have not reached
sendRequestCallback yet.
> +    // If m_defersLoading is true at that point, we'll pause.
>      if (!d->m_inputStream)
>	   return;

This check existed only because calling soup_session_pause_message at the wrong
time led to CRITICAL warnings. I'm pretty sure you can remove it competely now.


> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:582
> +	   GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult;
> +	   d->m_deferredResult = 0;

I think you can just do: RefPtr<GAsyncResult> asyncResult =
adoptGRef(d->m_deferredResult.leakRef());