|Summary:||[soup] implement ResourceHandle::platformSetDefersLoading|
|Product:||WebKit||Reporter:||Philippe Normand <pnormand>|
|Severity:||Normal||CC:||abarth, eric, mrobinson, webkit.review.bot|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:||44261, 50009|
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 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.