[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 #7 from Martin Robinson <mrobinson at webkit.org>  2012-05-09 17:59:23 PST ---
(From update of attachment 140735)
View in context: https://bugs.webkit.org/attachment.cgi?id=140735&action=review

This looks pretty good to me! I have a few outstanding questions though, before I feel comfortable giving an r+.

> Source/WebKit2/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

It's probably a good idea to write a couple sentences describing this change in general, for the sake of future readers.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:78
>      if (!m_client.didReceiveURIRequest(this, WebURL::create(uriString).get(), requestID))
> -        handleURIRequest(WebData::create(0, 0).get(), String(), requestID);
> +        handleURIRequest(WebData::create(0, 0).get(), 0, String(), requestID);

Hrm. Perhaps this should return an error?

> Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:45
> +    uint64_t bytesAdded;

bytesReceivedFromWebProcess or bytesReceived? bytesAdded could easily refer to "bytes added to the output."

> Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:48
> +    Mutex readLock;

I'm a little confused as to what exactly this mutex is protecting. Is it protecting just the data in this private structure or any operations at all?

> Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:81
> +        stream->priv->asyncReadData = adoptPtr(new AsyncReadData(result.get(), buffer, count, cancellable));

Won't this break if g_input_stream_read_async is called twice in a row?

> Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:135
> +    if (webkitSoupRequestInputStreamFinished(stream))
> +        return;

Does this need to be protected by the mutex or can the mutex be locked after this early return¿

> Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:137
> +    if (stream->priv->bytesAdded + dataLength > stream->priv->contentLength)
> +        dataLength = stream->priv->contentLength - stream->priv->bytesAdded;

Dan should confirm, but is it really an error to pass more data to soup than specified by the content length? What about the case where contentLength = -1 that he mentioned above?

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