Bug 118598 – [WK2][Soup] Use didReceiveBuffer instead of didReceiveData

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kwang Yul Seo]

Use didReceiveBuffer introduced in Bug 112310.

[Kwang Yul Seo]

Created attachment 206544 [details]
Patch

[Gustavo Noronha (kov)]

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.

[Kwang Yul Seo]

(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

[Kwang Yul Seo]

Closing this bug as invalid in favor of Bug 118448.

[Csaba Osztrogonác]

Reopen, because we have to use didReceiveBuffer, there is no choice.
See https://bugs.webkit.org/show_bug.cgi?id=118448#c32 for details.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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)

[Csaba Osztrogonác]

(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?

[Gustavo Noronha (kov)]

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.

[Martin Robinson]

Created attachment 218729 [details]
Patch

[EFL EWS Bot]

Comment on attachment 218729 [details]
Patch

Attachment 218729 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/45888018

[EFL EWS Bot]

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

[Martin Robinson]

Created attachment 218734 [details]
Patch

[Sergio Villar Senin]

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?

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

Comment on attachment 218734 [details]
Patch

Let's do this!

[Martin Robinson]

Committed r160310: <http://trac.webkit.org/changeset/160310>