Bug 118598 - [WK2][Soup] Use didReceiveBuffer instead of didReceiveData
Summary: [WK2][Soup] Use didReceiveBuffer instead of didReceiveData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks: 108832
  Show dependency treegraph
 
Reported: 2013-07-12 05:52 PDT by Kwang Yul Seo
Modified: 2013-12-09 06:07 PST (History)
13 users (show)

See Also:


Attachments
Patch (13.31 KB, patch)
2013-07-12 07:17 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (14.50 KB, patch)
2013-10-11 06:48 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (23.69 KB, patch)
2013-12-09 00:42 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (23.96 KB, patch)
2013-12-09 01:02 PST, Martin Robinson
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2013-07-12 05:52:43 PDT
Use didReceiveBuffer introduced in Bug 112310.
Comment 1 Kwang Yul Seo 2013-07-12 07:17:25 PDT
Created attachment 206544 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2013-07-15 08:14:14 PDT
Comment on attachment 206544 [details]
Patch

As discussed in bug 118448, the other approach (using didReceiveData) seems to be more sound for soup, since we already use a data pointer and length instead of a buffer object. Wrapping the data in SoupBuffer just adds unnecessary churn and complexity. I took it from what Brady said that he'd be OK with our having didReceiveData in our own NetworkResourceLoaderSoup.cpp, right? I'm setting r- for that reason, we can re-visit if there is actually opposition to the patch from bug 118448.
Comment 3 Kwang Yul Seo 2013-07-15 08:47:14 PDT
(In reply to comment #2)
> (From update of attachment 206544 [details])
> As discussed in bug 118448, the other approach (using didReceiveData) seems to be more sound for soup, since we already use a data pointer and length instead of a buffer object. Wrapping the data in SoupBuffer just adds unnecessary churn and complexity. I took it from what Brady said that he'd be OK with our having didReceiveData in our own NetworkResourceLoaderSoup.cpp, right? I'm setting r- for that reason, we can re-visit if there is actually opposition to the patch from bug 118448.

I agree. This approach makes sense only when we use a signal like "got-chunk" which gives us a buffer object instead of a data pointer length. Otherwise it just adds unnecessary complexity.

I will revisit this approach after we solve the problem Sergio mentioned https://bugs.webkit.org/show_bug.cgi?id=118448#c12
Comment 4 Kwang Yul Seo 2013-07-25 16:54:34 PDT
Closing this bug as invalid in favor of Bug 118448.
Comment 5 Csaba Osztrogonác 2013-10-10 07:58:08 PDT
Reopen, because we have to use didReceiveBuffer, there is no choice.
See https://bugs.webkit.org/show_bug.cgi?id=118448#c32 for details.
Comment 6 Csaba Osztrogonác 2013-10-11 06:23:19 PDT
Comment on attachment 206544 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:318
> -#if !USE(CF) || PLATFORM(QT)
> +#if !(USE(CF) || USE(SOUP)) || PLATFORM(QT)

It should be updated, because Qt was removed from trunk.
Comment 7 Csaba Osztrogonác 2013-10-11 06:48:31 PDT
Created attachment 213986 [details]
Patch

Updated to the ToT with the following minor changes: resolved a trivial conflict in Source/WebCore/platform/SharedBuffer.cpp after Qt removal, initialized destination variable in SharedBuffer.cpp (-Wmaybe-uninitialized), added the new SharedBufferSoup.cpp to PlatformGTK.cmake too. It builds and works for me (at least on EFL MiniBrowser)
Comment 8 Csaba Osztrogonác 2013-10-14 10:11:43 PDT
(In reply to comment #2)
> (From update of attachment 206544 [details])
> As discussed in bug 118448, the other approach (using didReceiveData) seems to be more sound for soup, since we already use a data pointer and length instead of a buffer object. Wrapping the data in SoupBuffer just adds unnecessary churn and complexity. I took it from what Brady said that he'd be OK with our having didReceiveData in our own NetworkResourceLoaderSoup.cpp, right? I'm setting r- for that reason, we can re-visit if there is actually opposition to the patch from bug 118448.

There was strong objection in https://bugs.webkit.org/show_bug.cgi?id=118448#c32, so could you revisit this bug, please?
Comment 9 Gustavo Noronha (kov) 2013-10-16 01:48:23 PDT
Comment on attachment 213986 [details]
Patch

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

Looks good in general but we should probably deal with the external buffer in a different way. I can give that a go early next week if you don't beat me to it.

> Source/WebCore/platform/network/soup/GOwnPtrSoup.h:23
> +#include <libsoup/soup.h>

can you use a typedef forward-declaration like the one we have for the other soup types?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1337
> +    SoupBuffer* buffer = soup_buffer_new(d->m_useExternalReadBuffer ? SOUP_MEMORY_COPY : SOUP_MEMORY_TAKE, d->m_readBufferPtr, bytesRead);
> +    handle->client()->didReceiveBuffer(handle.get(), SharedBuffer::wrapSoupBuffer(buffer), bytesRead);

The reason why we originally added the 'external buffer' thing was to avoid any copying from the http backend to the media player backend (memcpy may be surprisingly slow in some systems). This breaks that, and I guess a copy is something we'll have to live with in the network process future. So I'm wondering if it still makes sense to keep the external buffer stuff in, or if we should just resimplify this by tearing it out.
Comment 10 Martin Robinson 2013-12-09 00:42:58 PST
Created attachment 218729 [details]
Patch
Comment 11 EFL EWS Bot 2013-12-09 00:51:55 PST
Comment on attachment 218729 [details]
Patch

Attachment 218729 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/45888018
Comment 12 EFL EWS Bot 2013-12-09 00:58:03 PST
Comment on attachment 218729 [details]
Patch

Attachment 218729 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/46278071
Comment 13 Martin Robinson 2013-12-09 01:02:23 PST
Created attachment 218734 [details]
Patch
Comment 14 Sergio Villar Senin 2013-12-09 04:27:16 PST
Comment on attachment 218734 [details]
Patch

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

Looks great to me

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:150
> +    }

Shouldn't we keep this for non-NetworkProcess usage?
Comment 15 Gustavo Noronha (kov) 2013-12-09 04:33:00 PST
Comment on attachment 218734 [details]
Patch

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

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:150
>> +    }
> 
> Shouldn't we keep this for non-NetworkProcess usage?

We would need to have #ifdefed code in our ResourceHandle implementation to use it in that case, I don't think it's worth the maintenance cost.
Comment 16 Gustavo Noronha (kov) 2013-12-09 04:40:02 PST
Comment on attachment 218734 [details]
Patch

Let's do this!
Comment 17 Martin Robinson 2013-12-09 06:07:28 PST
Committed r160310: <http://trac.webkit.org/changeset/160310>