Bug 118598

Created attachment 206544 [details] Patch

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.

(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

Closing this bug as invalid in favor of Bug 118448.

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 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.

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)

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

Created attachment 218729 [details] Patch

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

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

Created attachment 218734 [details] Patch

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 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 on attachment 218734 [details] Patch Let's do this!

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