[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 #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-04-30 08:56:46 PST ---
(In reply to comment #4)
> (From update of attachment 200029 [details])
> 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?

The first time.

> Reading this code, I can see only one situation, which is if getOrCreateReadBuffer returns one that is smaller than the default size.

In such case the default buffer is not used. The caller is not supposed to change the size parameter unless a valid pointer is returned.

> If that's the case that you're dealing with here, why not check the size instead of just the return value of bufferPtr?

The default implementation returns NULL and the size out parameter is not changed. We only expect to receive a size when a valid pointer is returned by the client.

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

That's for redirections, in case of redirection the current soup request is cleaned up (and Ptr is set to 0) and a new one is started for the new request, but we still want to reuse the buffer instead of free/alloc.

> Are you expecting ResourceHandleInternal to be shared between ResourceHandles?

No.

> Even in that case, the internal buffer (m_readBuffer) is always the same size.

That's true, we could just check if the m_buffer is NULL or not like the current code does, instead of checking the size. I added the size check in case the size could change in the future, but maybe it's confusing, so I can change it to check the default buffer pointer instead.

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

I don't really mind to change the name, but the method, creates a buffer or returns an existing one, it's pretty clear to me. Regarding the variable name, embedded sounds confusing to me. What bout m_readBufferPtr and m_defaultReadBuffer?

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