[webkit-reviews] review denied: [Bug 118598] [WK2][Soup] Use didReceiveBuffer instead of didReceiveData : [Attachment 213986] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Csaba Osztrogonac
<ossy at webkit.org>'s request for review:
Bug 118598: [WK2][Soup] Use didReceiveBuffer instead of didReceiveData
https://bugs.webkit.org/show_bug.cgi?id=118598

Attachment 213986: Patch
https://bugs.webkit.org/attachment.cgi?id=213986&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.