[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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #141142|review?                     |review+
               Flag|                            |




--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2012-05-10 11:21:55 PST ---
(From update of attachment 141142)
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. 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.

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