[Webkit-unassigned] [Bug 115364] [SOUP] Move default buffer handling from ResourceHandleClient to ResourceHandleSoup

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115364





--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2013-04-30 08:45:27 PST ---
(From update of attachment 200029)
View in context: https://bugs.webkit.org/attachment.cgi?id=200029&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:321
> +    } else if (d->m_readBufferSize < defaultReadBufferSize) {
> +        d->m_readBuffer.set(static_cast<char*>(g_malloc(defaultReadBufferSize)));
> +        d->m_readBufferPtr = d->m_readBuffer.get();
> +        d->m_readBufferSize = defaultReadBufferSize;
> +    } else
> +        d->m_readBufferPtr = d->m_readBuffer.get();

In what case is d->m_readBufferSize ever less than the defaultReadBufferSize? Reading this code, I can see only one situation, which is if getOrCreateReadBuffer returns one that is smaller than the default size. If that's the case that you're dealing with here, why not check the size instead of just the return value of bufferPtr? Likewise, I don't get the final else case. If d->m_readBuffer is allocated at all, shouldn't d->m_readBufferPtr point to it anyway -- since when you get a valid return value from getOrCreateReadBuffer you reset m_readBuffer and m_readBufferPtr. Are you expecting ResourceHandleInternal to be shared between ResourceHandles? Even in that case, the internal buffer (m_readBuffer) is always the same size.

> You have misread the patch :-) The method is in the client and the owner of the buffer is the client. But if the client doesn't provide a buffer, the resource handle creates its own one, and is the owner of that one, of course. Depending on how the buffer handling is implemented, the function will return a new buffer or reuse a existing one (like we do with the default buffer), that's why it's called getOrCreateReadBuffer.

I see now that d->m_readBuffer is not set to the return value of getOrCreateReadBuffer. The use of the word "Create" is a bit confusing here since it's used for constructors quite a bit in WebKit. Perhaps just "getReadBuffer" or "getClientReadBuffer." Another naming nit is that the distinction between readBuffer and readBufferPtr is a bit unclear. How do you feel about Cairo terminology: m_readBuffer/m_embeddedReadBuffer or m_readBufferPtr/m_embeddedReadBuffer.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.