Bug 72227 - [GTK] improve platformSetDefersLoading
Summary: [GTK] improve platformSetDefersLoading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 71611
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-13 10:42 PST by Dan Winship
Modified: 2011-11-16 10:21 PST (History)
3 users (show)

See Also:


Attachments
Fix platformDefersLoading (7.03 KB, patch)
2011-11-13 10:46 PST, Dan Winship
mrobinson: review-
Details | Formatted Diff | Diff
updated patch (7.20 KB, patch)
2011-11-15 08:16 PST, Dan Winship
mrobinson: review-
Details | Formatted Diff | Diff
updated updated patch (6.99 KB, patch)
2011-11-16 08:22 PST, Dan Winship
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Winship 2011-11-13 10:42:45 PST
ResourceHandleSoup currently implements defersLoading by calling soup_session_pause_message()/soup_session_unpause_message(), which is bad because that API sucks, and because SoupHTTPInputStream also uses it internally, and ResourceHandleSoup's use of it interferes with libsoup's, and complicates SoupHTTPInputStream because it has to deal with receiving input even when it thought the message was paused.

This patch fixes that. It's built on top of the bug 71611 patches, although I think it could be rebased away from that pretty easily if we wanted to land it before that.

This patch also makes defersLoading work for non-http URIs, which didn't work before... I looked through the skipped tests list, but I couldn't find any that looked like they were being skipped because of that. Maybe we need a new one?
Comment 1 Dan Winship 2011-11-13 10:46:05 PST
Created attachment 114861 [details]
Fix platformDefersLoading
Comment 2 Martin Robinson 2011-11-13 20:32:50 PST
Comment on attachment 114861 [details]
Fix platformDefersLoading

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());
Comment 3 Martin Robinson 2011-11-13 20:33:41 PST
Philippe can you verify that "emulating" a message pause still fulfills the needs of the GStreamer backend.
Comment 4 Philippe Normand 2011-11-14 01:50:29 PST
(In reply to comment #3)
> Philippe can you verify that "emulating" a message pause still fulfills the needs of the GStreamer backend.

I tested the patches, haven't found any regression media playback-wise, I did some functional testing and tried the layout tests as well.
Comment 5 Dan Winship 2011-11-14 06:10:37 PST
(In reply to comment #2)
> > +        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."

Yeah, that was intentional:

> This patch also makes defersLoading work for non-http URIs, which
> didn't work before... I looked through the skipped tests list, but
> I couldn't find any that looked like they were being skipped because
> of that. Maybe we need a new one?


> It seems like there's nothing now to do in this method if defersLoading == true?

> 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.

Ah, yeah. Cruft from earlier versions of the patch.

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

OK. But is that actually nicer? :)

Is http://www.webkit.org/coding/RefPtr.html out of date? Or does GRefPtr intentionally not work like RefPtr? I originally tried

    GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult.release();

but GRefPtr doesn't have release() (and the docs on the RefPtr page say that leakRef() only exists on PassRefPtr)
Comment 6 Martin Robinson 2011-11-14 10:24:06 PST
(In reply to comment #5)

> Is http://www.webkit.org/coding/RefPtr.html out of date? Or does GRefPtr intentionally not work like RefPtr? I originally tried
> 
>     GRefPtr<GAsyncResult> asyncResult = d->m_deferredResult.release();
>
> but GRefPtr doesn't have release() (and the docs on the RefPtr page say that leakRef() only exists on PassRefPtr)

Oh. I think that GRefPtr is missing the release method? That you needed it here indicates that it should probably have it. Do you mind adding it with your patch and using it? GRefPtr just lags behind RefPtr feature-wise.
Comment 7 Dan Winship 2011-11-15 08:15:59 PST
(In reply to comment #6)
> Oh. I think that GRefPtr is missing the release method? That you needed it here indicates that it should probably have it. Do you mind adding it with your patch and using it? GRefPtr just lags behind RefPtr feature-wise.

Hm... RefPtr.release() returns a PassRefPtr, but there's no GPassRefPtr.

The docs say "If ownership and lifetime are guaranteed, a local variable can be a raw pointer." So I just did that (with leakRef).
Comment 8 Dan Winship 2011-11-15 08:16:25 PST
Created attachment 115169 [details]
updated patch
Comment 9 Dan Winship 2011-11-15 08:16:41 PST
oh, that's rebased to master btw, so it's landable
Comment 10 Martin Robinson 2011-11-15 08:42:29 PST
Comment on attachment 115169 [details]
updated patch

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

Looks good to me, except that I think you are leaking the asyncResult.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:750
> +    // We only need to take action here to UN-defer loading

Nit: Missing a period.

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

Is it actually possible for m_cancellable to be non-null and d->m_soupRequest to be null? If so it'd be good to leave a small comment here explaining when and if not, it'd be better to ASSERT(d->m_soupRequest) instead.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:763
> +        GAsyncResult* asyncResult = d->m_deferredResult.leakRef();

I think you actually need to call adoptGRef here because the naked assignment adds another reference and the pointer leaks. Sorry for the confusion.
Comment 11 Dan Winship 2011-11-16 08:22:24 PST
Created attachment 115382 [details]
updated updated patch

updated for previous comments in bugzilla and on IRC. (I ended up going with your original suggestion for the leakref thing.)
Comment 12 WebKit Review Bot 2011-11-16 10:21:04 PST
Comment on attachment 115382 [details]
updated updated patch

Clearing flags on attachment: 115382

Committed r100466: <http://trac.webkit.org/changeset/100466>
Comment 13 WebKit Review Bot 2011-11-16 10:21:08 PST
All reviewed patches have been landed.  Closing bug.