[Webkit-unassigned] [Bug 72227] [GTK] improve platformSetDefersLoading

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72227


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114861|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2011-11-13 20:32:50 PST ---
(From update of attachment 114861)
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());

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.