[Webkit-unassigned] [Bug 85880] [SOUP] Allow sending URI request data in chunks

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-05-11 00:40:15 PST ---
(In reply to comment #13)
> (From update of attachment 141142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141142&action=review
> 
> Okay! Thanks for addressing my comments above. Before landing please consider the following simplification and clean up. I think it will make the code a lot more readable. I had some small difficulty understanding this partly because (1) I'm an idiot and (2) It's tricky to keep track of "bytes read out" and "bytes received" especially when their names both conjure images of bytes coming in.
> 
> > Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:83
> > +    // There is data in the memory stream to read.
> > +    if (stream->priv->bytesRead < stream->priv->bytesReceived) {
> > +        webkitSoupRequestInputStreamReadAsyncResultComplete(stream, result.get(), buffer, count, cancellable);
> > +        return;
> > +    }
> > +
> > +    // We are expecting more data to be added.
> > +    if (!webkitSoupRequestInputStreamFinished(stream)) {
> > +        stream->priv->asyncReadData = adoptPtr(new AsyncReadData(result.get(), buffer, count, cancellable));
> > +        return;
> > +    }
> 
> I feel that it might be slightly simpler to unconditionally create stream->priv->asyncReadData here.

That would mean adding an unnecessary alloc/dealloc in the case where we already have the data ready in the stream to read from. AsyncReadData is to postpone the async result completion until we have more data in the stream to read from.

> You could do something like:
> 
> bool moreDataToSendForRead = stream->priv->bytesRead < stream->priv->bytesReceived;
> if (!moreDataToSendForRead && webkitSoupRequestInputStreamFinished(stream)) {
>     g_simple_async_result_set_op_res_gssize(result.get(), 0);
>     g_simple_async_result_complete_in_idle(result.get());
>     return;
> }
> 
> stream->priv->asyncReadData = adoptPtr(new AsyncReadData(result.get(), buffer, count, cancellable));
> if (moreDataToSendForRead)
>     webkitSoupRequestInputStreamReadAsyncResultComplete(stream, cancellable);
> 
> 
> This way webkitSoupRequestInputStreamReadAsyncResultComplete would be responsible for calling stream->priv->asyncReadData.clear(), which seems to make more sense since it consumes it.
> 
> Also, instead of adding a comment like:
> // We are expecting more data to be added.
> if (!webkitSoupRequestInputStreamFinished(stream)) {
> 
> I think it makes sense to simply rename webkitSoupRequestInputStreamFinished to webkitSoupRequestWaitingForMoreDataToBeAdded or something better. I recommend doing the same for stream->priv->bytesRead < stream->priv->bytesReceived by creating a helper like webkitSoupRequestSentAllDataForPendingRead or something similar. Helper methods with good names are almost always better than comments, I'd say.

I'll try to use better names

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