[webkit-reviews] review granted: [Bug 85880] [SOUP] Allow sending URI request data in chunks : [Attachment 141142] Updated patch according to review comments

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 85880: [SOUP] Allow sending URI request data in chunks
https://bugs.webkit.org/show_bug.cgi?id=85880

Attachment 141142: Updated patch according to review comments
https://bugs.webkit.org/attachment.cgi?id=141142&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.