Bug 44158 - [soup] implement ResourceHandle::platformSetDefersLoading
Summary: [soup] implement ResourceHandle::platformSetDefersLoading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 44261 50009
Blocks: 44157
  Show dependency treegraph
 
Reported: 2010-08-18 02:14 PDT by Philippe Normand
Modified: 2010-12-06 04:32 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (1.71 KB, patch)
2010-08-18 03:19 PDT, Philippe Normand
gustavo: review-
Details | Formatted Diff | Diff
proposed patch (2.07 KB, patch)
2010-08-23 05:08 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (2.24 KB, patch)
2010-08-30 08:38 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
updated patch (3.04 KB, patch)
2010-10-19 09:01 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
updated patch (3.00 KB, patch)
2010-10-19 09:07 PDT, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
updated patch (3.41 KB, patch)
2010-11-23 08:17 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff
updated patch (4.00 KB, patch)
2010-12-06 03:38 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-08-18 02:14:59 PDT
as in subject, using soup_session_pause_message and soup_session_unpause_message
Comment 1 Philippe Normand 2010-08-18 03:19:25 PDT
Created attachment 64680 [details]
proposed patch
Comment 2 Gustavo Noronha (kov) 2010-08-18 10:06:37 PDT
Comment on attachment 64680 [details]
proposed patch

I think this is a good thing to have, since other platforms support it, but do we have an idea of what this gains us? Is there a way to test it? It looks like we want to also check whether the message should start deferred: the constructor has a boolean: ResourceHandle(const ResourceRequest&, ResourceHandleClient*, bool defersLoading, bool shouldContentSniff);. Mac checks m_defersLoading when a new message is queued, and pauses it, it seems. I'll say r- for the lack of checking if we should start deferred, but it would be great if we could know why we are implementing this before we do.
Comment 3 Martin Robinson 2010-08-18 10:11:54 PDT
A quick grep shows that this is used throughout WebCore including extensively in PluginStream code to avoid bugs with plugins. It will also soon be used to pause/resume the download in WebKitWebSourceGStreamer.
Comment 4 Martin Robinson 2010-08-18 10:12:58 PDT
I should also mention that I originally suggested to pnormand that he use this instead of reimplementing the functionality behind #ifdefs in https://bugs.webkit.org/show_bug.cgi?id=41512 .
Comment 5 Philippe Normand 2010-08-20 04:48:18 PDT
Oh right, I forgot about this one.

Not sure I have time today but monday for sure I will revise the patch. Thanks kov for this first look at the code.
Comment 6 Philippe Normand 2010-08-23 05:08:38 PDT
Created attachment 65105 [details]
proposed patch

About a test that would cover this I was thinking that maybe bug 42152
could be a good candidate but not sure yet. Opinions?
Comment 7 Martin Robinson 2010-08-23 08:39:36 PDT
Yes, that's an excellent idea. If you implement those things, pass the test and then add them to this patch I will r+ it. Thanks!
Comment 8 Philippe Normand 2010-08-25 23:41:49 PDT
(In reply to comment #7)
> Yes, that's an excellent idea. If you implement those things, pass the test and then add them to this patch I will r+ it. Thanks!

Filed a patch there but I only had to add the required DRT stuff to make the test pass, so not sure anymore it is a good test to test this patch ;)
Comment 9 Philippe Normand 2010-08-30 08:38:24 PDT
Created attachment 65917 [details]
proposed patch

Added a check for d->m_msg before using it and a note about GIO
Comment 10 Philippe Normand 2010-08-30 09:35:11 PDT
The WebCore/manual-tests/xhr-failure-behind-alert.html test triggers the platformSetDefersLoading() code path but in that case d->m_msg is NULL because no connection to the server was made (because none is running at 127.0.0.1:7 ...) ;)
Comment 11 Dan Winship 2010-08-30 11:35:23 PDT
Comment on attachment 65917 [details]
proposed patch

>      soup_session_queue_message(session, d->m_msg, finishedCallback, handle);
>  
> +    if (d->m_defersLoading)
> +        soup_session_pause_message(session, d->m_msg);

This won't work; pause/unpause are sort of lame, and only work once I/O has actually started (eg, after the message has been assigned to a connection). If you want the message to be deferred right away, you need to just not queue it.
Comment 12 Philippe Normand 2010-08-31 01:56:21 PDT
Comment on attachment 65917 [details]
proposed patch

Unmarking for review, I think it'd be better to wait the patch from bug 44261 to land so we can implement a better patch for this bug.
Comment 13 Philippe Normand 2010-10-19 09:01:56 PDT
Created attachment 71174 [details]
updated patch

As Dan suggested, in startHttp I avoid sending the requested if it's
initially deferred.
Comment 14 Philippe Normand 2010-10-19 09:07:59 PDT
Created attachment 71176 [details]
updated patch
Comment 15 Martin Robinson 2010-10-19 09:48:32 PDT
Comment on attachment 71176 [details]
updated patch

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

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:739
> +    // Only supported for http(s) transfers. Something similar would
> +    // probably be needed for data transfers done with GIO.

This comment is inaccurate now that data URLs are handled by soup, I think. :)

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:746
> +    else
> +        soup_session_unpause_message(defaultSession(), d->m_soupMessage.get());

I think this presents a problem. The time between the creation of m_soupRequest and m_soupMessage is an asynchronous call. If setDefersLoading(...) was called while that call was in progress, the message would not be paused properly. Perhaps there should be a companion call to soup_session_pause_message if the message is m_defersLoading is true in sendRequestCallback.
Comment 16 Philippe Normand 2010-10-20 01:26:11 PDT
(In reply to comment #15)
> (From update of attachment 71176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71176&action=review
> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:739
> > +    // Only supported for http(s) transfers. Something similar would
> > +    // probably be needed for data transfers done with GIO.
> 
> This comment is inaccurate now that data URLs are handled by soup, I think. :)
> 

Well I think it's still accurate ;) m_soupMessage is created only in startHttp(). startData and the data uri loading doesn't use it, afaics.

> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:746
> > +    else
> > +        soup_session_unpause_message(defaultSession(), d->m_soupMessage.get());
> 
> I think this presents a problem. The time between the creation of m_soupRequest and m_soupMessage is an asynchronous call. If setDefersLoading(...) was called while that call was in progress, the message would not be paused properly. Perhaps there should be a companion call to soup_session_pause_message if the message is m_defersLoading is true in sendRequestCallback.

Hum, that's right indeed. Will send a new patch, thanks for reviewing :)
Comment 17 Martin Robinson 2010-10-21 18:00:01 PDT
This should be fixed since r67722. Closing.
Comment 18 Philippe Normand 2010-10-21 23:42:54 PDT
How come a style bot commit have fixed this bug?
Comment 19 Martin Robinson 2010-10-22 08:54:50 PDT
(In reply to comment #18)
> How come a style bot commit have fixed this bug?

Sorry, wrong bug.
Comment 20 Philippe Normand 2010-11-23 08:17:09 PST
Created attachment 74669 [details]
updated patch
Comment 21 Martin Robinson 2010-11-23 08:36:38 PST
Comment on attachment 74669 [details]
updated patch

Great.
Comment 22 Philippe Normand 2010-11-24 00:26:38 PST
Committed r72647: <http://trac.webkit.org/changeset/72647>
Comment 23 Philippe Normand 2010-11-24 01:21:21 PST
This patch broke the build (yes I checked here before landing :/). Rollout is in progress, see bug 50009
Comment 24 WebKit Review Bot 2010-11-24 01:32:56 PST
http://trac.webkit.org/changeset/72647 might have broken GTK Linux 32-bit Release
Comment 25 Philippe Normand 2010-12-06 03:38:36 PST
Created attachment 75664 [details]
updated patch

The tests were failing because loadResourceSynchronously() was
creating a deferred ResourceHandle.
Comment 26 Martin Robinson 2010-12-06 04:25:16 PST
Comment on attachment 75664 [details]
updated patch

Nice catch.
Comment 27 Philippe Normand 2010-12-06 04:32:06 PST
Committed r73357: <http://trac.webkit.org/changeset/73357>