Prototype of chunked transfer encoded POST.

Prototype of chunked transfer encoded POST.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72471

[wtc, 2011-01-12 02:38:57]

I removed eroman and willchan as reviewers.  After the
first round of review, I will add rdsmith as a second
reviewer.

Here are my preliminary review comments.

High-level comments:

1. It is tempting to move the encoding of the chunks
from UploadData::Element::SetToChunk to the UploadDataStream
class, so that we can chunk-encode any type of
UploadData::Element (bytes, file, or blob).  Did you
consider this?

But this approach may complicate the FillBuf method of
UploadDataStream.  I haven't given much thought to this.

2. The "data callback" of UploadData should be a
Delegate interface with a OnChunkAvaiable method instead.

The old chunks don't need to be saved in the
elements_ array of UploadData.  When a new chunk is
added to UploadData, it should be passed to the
OnChunkAvailable method of the delegate, and then discarded.

If you keep the old chunks around, you may run out of
memory for a long speech streaming POST.

3. UploadDataStream::OnDataAvailable can bypass FillBuf
and directly copy the new chunk into its internal buf_.

Detailed comments follow.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.cc#newcode79
net/base/upload_data.cc:79: // we have no more. This is used by
UploadDataStream.
Why don't you just use the 0 chunk size to indicate that
we have no more?

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.cc#newcode153
net/base/upload_data.cc:153: //data_callback_ = NULL;
This is a sign that CompletionCallback is the wrong type.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h#newcode27
net/base/upload_data.h:27: TYPE_CHUNK,  // Indicates that more data is to come.
The more important info is "send the data immediately".

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h#newcode129
net/base/upload_data.h:129: void set_data_callback(CompletionCallback*
callback);
Rename this method set_chunk_callback and the member
chunk_callback_.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h#newcode132
net/base/upload_data.h:132: // than all at once at connection time.
The UploadData class doesn't know about the connection, so
it is confusing to mention connection time here.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.cc#...
net/base/upload_data_stream.cc:79: c->Run(0);
Should you pass 'result' instead of 0 to c->Run()?

Alternatively, I think the callback should be a Task
instead of a CompletionCallback.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:52: bool waiting_for_data() const { return
waiting_for_data_; }
It seems that we don't need to add a waiting_for_data method.
We can just test
  !eof() && buf_len() == 0
to see if we need to wait for data.

http://codereview.chromium.org/6134003/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

http://codereview.chromium.org/6134003/diff/1/net/http/http_util.cc#newcode679
net/http/http_util.cc:679: HttpRequestHeaders::kTransferEncoding, "Chunked");
Use lowercase "chunked", which is the string used in
HTTP 1.1 RFC 2616.

http://codereview.chromium.org/6134003/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/1/net/url_request/url_request.h#n...
net/url_request/url_request.h:316: // that more is to come. This is useful when
the POST data is not available
I think "more is to come" is less useful info than "send the
chunk to the server immediately", so this comment could be
improved.

Document the MarkEndOfChunks method.  Alternatively, just
use a AppendChunkToUpload(NULL, 0) call to mark the end of
chunks.

Compared with the AppendXXXToUpload methods above, it's
important to point out that AppendChunkToUpload may only
be called after Start() is called.

Also, since you have a chunked_transfer_upload_ mode, why
don't you just reuse the existing AppendBytesToUpload method
to append a chunk?

http://codereview.chromium.org/6134003/diff/1/net/url_request/url_request.h#n...
net/url_request/url_request.h:641: bool chunked_transfer_upload_;
Nit: shorten this to chunked_upload_ (and the setter to
set_chunked_upload).

[Satish, 2011-01-13 17:43:27]

> 1. It is tempting to move the encoding of the chunks
> from UploadData::Element::SetToChunk to the UploadDataStream
> class, so that we can chunk-encode any type of
> UploadData::Element (bytes, file, or blob).  Did you
> consider this?

I don't have any use cases for these now, and in particular not sure why we
might upload a static file via chunked-transfer encoding when the full data is
available. Perhaps that can be done when we need such functionality, so we don't
write code which doesn't get used?
 
> The old chunks don't need to be saved in the
> elements_ array of UploadData.  When a new chunk is
> added to UploadData, it should be passed to the
> OnChunkAvailable method of the delegate, and then discarded.
> 
> If you keep the old chunks around, you may run out of
> memory for a long speech streaming POST.

Agreed. However Darin gave this suggestion earlier:

"Keep in mind that we need to be able to rewind the UploadDataStream in some
cases.  If we write a POST body to a socket that is in the process of being
closed (i.e., if we get a TCP RST), then we'll need to open a new socket and
resend the POST data."

Wouldn't require keeping the chunks in memory as they are now?

> 3. UploadDataStream::OnDataAvailable can bypass FillBuf
> and directly copy the new chunk into its internal buf_.

UploadData & UploadDataStream do the buffering today without any assumptions as
to when the caller (HttpStreamParser in this case) would be reading the data and
how much. I decided to reuse the existing code paths and call FillBuf instead
because

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.cc#newcode79
net/base/upload_data.cc:79: // we have no more. This is used by
UploadDataStream.
On 2011/01/12 02:39:02, wtc wrote:
> Why don't you just use the 0 chunk size to indicate that
> we have no more?

Done, I now add a "0\r\n\r\n" chunk followed by a "" chunk to indicate the end
of chunk stream

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.cc#newcode153
net/base/upload_data.cc:153: //data_callback_ = NULL;
On 2011/01/12 02:39:02, wtc wrote:
> This is a sign that CompletionCallback is the wrong type.

Changed to an interface/method call

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h#newcode27
net/base/upload_data.h:27: TYPE_CHUNK,  // Indicates that more data is to come.
On 2011/01/12 02:39:02, wtc wrote:
> The more important info is "send the data immediately".

Done.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h#newcode129
net/base/upload_data.h:129: void set_data_callback(CompletionCallback*
callback);
On 2011/01/12 02:39:02, wtc wrote:
> Rename this method set_chunk_callback and the member
> chunk_callback_.

Done.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data.h#newcode132
net/base/upload_data.h:132: // than all at once at connection time.
On 2011/01/12 02:39:02, wtc wrote:
> The UploadData class doesn't know about the connection, so
> it is confusing to mention connection time here.

Done.

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.cc#...
net/base/upload_data_stream.cc:79: c->Run(0);
On 2011/01/12 02:39:02, wtc wrote:
> Should you pass 'result' instead of 0 to c->Run()?

No. A non-zero value is given only by the socket write code to indicate that so
many bytes can really be discarded from the input stream. Here we just want to
trigger the same code path to read more data in, but there is no change in how
many bytes were sent out so nothing to discard/consume from the buffer.

> Alternatively, I think the callback should be a Task
> instead of a CompletionCallback.

Changed to ChunkCallback interface now since this can just be set once and
invoked repeatedly like in UploadData

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:52: bool waiting_for_data() const { return
waiting_for_data_; }
On 2011/01/12 02:39:02, wtc wrote:
> It seems that we don't need to add a waiting_for_data method.
> We can just test
>   !eof() && buf_len() == 0
> to see if we need to wait for data.

Done.

http://codereview.chromium.org/6134003/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

http://codereview.chromium.org/6134003/diff/1/net/http/http_util.cc#newcode679
net/http/http_util.cc:679: HttpRequestHeaders::kTransferEncoding, "Chunked");
On 2011/01/12 02:39:02, wtc wrote:
> Use lowercase "chunked", which is the string used in
> HTTP 1.1 RFC 2616.

Done.

http://codereview.chromium.org/6134003/diff/1/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/1/net/url_request/url_request.h#n...
net/url_request/url_request.h:316: // that more is to come. This is useful when
the POST data is not available
On 2011/01/12 02:39:02, wtc wrote:
> I think "more is to come" is less useful info than "send the
> chunk to the server immediately", so this comment could be
> improved.

Done

> Document the MarkEndOfChunks method.  Alternatively, just
> use a AppendChunkToUpload(NULL, 0) call to mark the end of
> chunks.

Documented. I prefer to not reuse AppendChunkToUpload with those magic values
since that requires the caller to know how the POST body is encoded.

> Compared with the AppendXXXToUpload methods above, it's
> important to point out that AppendChunkToUpload may only
> be called after Start() is called.

Done

> Also, since you have a chunked_transfer_upload_ mode, why
> don't you just reuse the existing AppendBytesToUpload method
> to append a chunk?

UploadData is the one which does the encoding of the chunks, prefixing them with
length+cr+lf and appending them with cr+lf. I've kept all other classes as
agnostic to the POST body format as possible.

http://codereview.chromium.org/6134003/diff/1/net/url_request/url_request.h#n...
net/url_request/url_request.h:641: bool chunked_transfer_upload_;
On 2011/01/12 02:39:02, wtc wrote:
> Nit: shorten this to chunked_upload_ (and the setter to
> set_chunked_upload).

Done.

[Satish, 2011-01-13 18:06:15]

There are no new tests in this CL yet. Once the general design is agreed
upon, I'll work on them.

[wtc, 2011-01-13 18:07:58]

rdsmith: could you review this CL?  You work on the download
manager, and this change is about uploads, so it seems
related :-)

[Randy Smith (Not in Mondays), 2011-01-13 21:33:36]

On 2011/01/13 18:07:58, wtc wrote:
> rdsmith: could you review this CL?  You work on the download
> manager, and this change is about uploads, so it seems
> related :-)

Wan-Teh: Are you sure you want me to review this?  At a quick glance there
doesn't seem to me to be any parallels between the download code and this upload
code, so you're asking me to climb the learning curve on a new net subsystem. 
And Satish pinged me offline indicating that there was a fairly high priority to
getting at least a design review of the code quickly, that's that's *harder*
than doing a line-by-line review when I don't know the architecture, because
I'll have to reconstruct the architecture from the code.  So I'm not sure I'm
the right person for this review.  

(Satish: I can't give you a high-level evaluation of this code in the next
fifteen minutes, which is the time I have today.  My guess is that it'd take me
a couple of hours to do it right, as I'd want to read a lot of surrounding code
to back-derive the code context.)

[wtc, 2011-01-13 23:29:01]

vandebo, jianli: SVN shows that you are the current experts
of the UploadData code.  Could you take a look at this CL as
a second reviewer?  I will do the primary review.

rdsmith: I selected you at random as a second reviewer.
We all need exposure to unfamiliar parts of the code base.
Since I've found the best people to review this CL, I'll
remove you from the cc list.

[wtc, 2011-01-14 03:09:31]

I only reviewed the files in "net" in this pass.

High-level comments/questions:

1. Does your code support rewinding the UploadDataStream?

2. I still prefer using (NULL, 0) to set the last chunk.
It seems that you're already doing that except in URLRequest,
correct?

3. I pointed out a problem with the comments that stress
the wrong property ("more data is to come") of chunks.
The most important property is "send immediately without
waiting for more data".  Another important property is
"send in chunked encoding".

So I suggested changes to those comments.

4. I suggest shortening ChunkCallback to Delegate.
Alternatively, make ChunkCallback a class at the top level
rather than nested inside UploadData and UploadDataStream.

Low-level comments follow.  Note especially the comments
marked with "IMPORTANT".  Please verify my understanding
of your code.

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:137: bool is_chunked_upload_;           // True
if using chunked transfer encoding
Please document this issue:
  If is_chunked_upload_ is true, is upload_content_ ignored?

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:80: void UploadData::Element::SetToLastChunk() {
Let's name this method SetToEndOfChunksMarker.

In HTTP 1.1, "last chunk" means
       last-chunk     = 1*("0") [ chunk-extension ] CRLF

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:157: // End of chunked stream is marked internally with
a chunk of length 0.
"a chunk of length 0" is not accurate.  You may want to be
more precise: "a chunk with an empty bytes_".

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:27: TYPE_CHUNK,  // More data is to come and this should
be sent immediately.
The comment should say:
  The data should be sent in chunked encoding immediately,
  without waiting for the other data.

"More data is to come" is confusing because it can be said
of the other types of data.  For example, SetToBytes can be
called multiple times, which means after each SetToBytes
call, more data is to come.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:89: // items after this item.
Again, the difference is that a chunk should be sent in
chunked encoding, and without waiting for the other chunks.

The fact that the stream has more such items after this item
is also true of bytes.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:150: bool is_chunked() { return is_chunked_; }
Nit: mark this method 'const'.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:35: waiting_for_data_(false),
IMPORTANT: after studying the code, I think
waiting_for_data_ should be maintained by UploadData
rather than UploadDataStream.

UploadData should initialize waiting_for_data_ to false,
and set waiting_for_data_ to true after it has added the
end-of-chunks marker.

But I'm not convinced that waiting_for_data_ is even
necessary.  It is only used in UploadDataStream::OnChunkAvailable().
UploadDataStream::OnChunkAvailable() is called when a new
chunk is added to the UploadData.  So by definition we must
be waiting for data.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.h:91: size_t next_element_;
Why does next_element_ need to be an array index?  Is it
because you need it to stay valid after reaching the end of
array?  (In contrast, an iterator becomes end() and can't be
incremented any more.)

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:14: #include "net/base/load_flags.h"
Nit: remove this header.

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:71: if (request_body)
Nit: request_body => request_body_

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:150: OnIOComplete(0);
IMPORTANT: this should do nothing unless we're in the
STATE_SENDING_BODY state:
  if (io_state_ == STATE_SENDING_BODY)
    OnIOComplete(0);

An alternative solution is to move the call
  request_body_->set_chunk_callback(this);
from line 72 to line 248, when we transition into the
STATE_SENDING_BODY.

The problem is that when a new chunk is added, this class
may still be in the STATE_SENDING_HEADERS state.  If we
call OnIOComplete(0) without checking, we will be calling
DoSendHeaders.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.cc:166: DCHECK(bytes_len > 0);
Nit: use DCHECK_GT here.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:318: // data becomes available it can be sent to
the server.
Nit: change "POST data" to "upload data" (two occurrences) in
this comment.

It seems that the main reason for chunked upload is that the
data is not available upfront, but rather that we want to
avoid the delay of buffering all the data.  If delay is not
an issue, you can accumulate all the data and then upload
it the old way.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:320: // This method can be called only after a
call to |set_chunked_upload(true)|
Typo: your set_chunked_upload method takes no argument.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:324: // Indicate the end of a chunked-transfer
encoded request body.
Nit: Indicate => Indicates

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:446: void set_chunked_upload() {
is_chunked_upload_ = true; }
Nit: set_chunked_upload => enable_chunked_upload
                        or set_is_chunked_upload

[vandebo (ex-Chrome), 2011-01-14 05:53:44]

FYI - I'm jury duty, so can only respond in the evenings.

I only gave this a once over, so I may have missed some details or hidden
implications both in my comments and in my understanding.

On 2011/01/13 17:43:27, Satish wrote:
> > 1. It is tempting to move the encoding of the chunks
> > from UploadData::Element::SetToChunk to the UploadDataStream
> > class, so that we can chunk-encode any type of
> > UploadData::Element (bytes, file, or blob).  Did you
> > consider this?
>
> I don't have any use cases for these now, and in particular not sure why we
> might upload a static file via chunked-transfer encoding when the full data is
> available. Perhaps that can be done when we need such functionality, so we
don't
> write code which doesn't get used?

There is a usecase, so if that is the only holdup, it would be better to do it
in UploadDataStream.  Sometimes a file will change size from the time we
generate the ContentLength header to the time we are able to read and send the
file.  It is not trivial to change this because of of the multiprocess
architecture.  We could always use chunked encodings for files and thus avoid
the problem.

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/net/net_pref...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:41:
prefs->RegisterBooleanPref(prefs::kDisableSpdy, true);
Is this a debugging change that shouldn't be in the final CL?

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/speech/speec...
chrome/browser/speech/speech_recognizer.cc:200: /*
Should you delete this now dead code?

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:105: void AddUploadDataChunkInThread(const
std::string& data);
Should this be private?

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:314: void URLFetcher::Core::MarkEndOfChunks() {
Seems you don't need both of these in the ::core class.  You could just as
easily call ::Core::AppendChunkToUpload("") from URLFetcher::MarkEndOfChunks.

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:520: core_->is_chunked_upload_ = false;
Seems that this should just be a DCHECK?

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:136: // Use add_upload_data_chunk() to give the
data chunks before or after calling
You may want to note that this must be called before Start() is called.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:34: if (type_ == TYPE_BYTES || type_ == TYPE_CHUNK)
GetContentLength doesn't make sense for TYPE_CHUNK.  Could you now DCHECK(type_
!= TYPE_CHUNK) ?

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:81: bytes_.clear();
Doesn't this need to be "0\r\n\r\n"?  It might be easier to just call
SetToChunk("", 0);

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:120: is_chunked_(false) {
Instead of an explicit is_chunked_, could you just check
elements_.front().type() == TYPE_CHUNK?  Especially since it's only used for
DCHECKs.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:33: // of data are received.
Note, applies only for TYPE_CHUNK.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:40: virtual ~ChunkCallback() {}
Does this need an implementation?  If so, shouldn't it be in the .cc file?

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:154: if (next_element_ == elements.size() &&
!buf_len_) {
Could this conditional be true and the next false (leading to eof_ = true) if we
get to this point before the first chunk of data is provided?  next_element_
will be 0, element.size() will be 0, but next_element_ won't be > 0.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:159: elements[next_element_ - 1].bytes().size() >
0) {
The last chunk won't have zero size at this point though, it should be
"0\r\n\r\n", size 5.  Probably cleanest to add a bool and accessor to
UploadData::Element to indicate the last chunk.  Either that, or you could
introduce a TYPE_LAST_CHUNK, but that may not be as clean.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.h:22: class ChunkCallback {
This interface class is the same as UploadData::ChunkCallback.  Is there a
reason to implement the same interface again?

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:72: request_body_->set_chunk_callback(this);
set_chunk_callback will be called even when not using chunked encoding?  In that
case, would it be better to name it something else? 
UploadDataStream::set_data_consumer(...) ?

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:150: OnIOComplete(0);
Indeed, this mustn't do anything unless io_state == STATE_SENDING_BODY

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.cc:332: if (is_chunked_upload_) {
Should this go into "set_chunked_upload" ?

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:322: void AppendChunkToUpload(const char* bytes,
int bytes_len);
This method seems different than the other AppendFoo methods.  Maybe different
enough that its name should not imply that it is the same as them?

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:325: void MarkEndOfChunks();
Is it really worthwhile to provide this method?  Why not just note in the
comments for AppendChunkToUpload that a zero length chunk is needed to signal
the end of chunked-transfer?

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:446: void set_chunked_upload() {
is_chunked_upload_ = true; }
It seems that this logically belongs with the other AppendFoo methods. i.e. it
must be called before start.  Also, it seems that it should not be combined with
them?  If so, that should be noted.

While it is a simple setter, the affect on the underlying system is such that
maybe it shouldn't be named like a simple setter?

Should this also do the following, like the other AppendFoo methods?

  if (!upload_)
    upload_ = new UploadData();

[Satish, 2011-01-14 18:09:29]

Thanks for the quick review. New patch uploaded, replies below:

> Sometimes a file will change size from the time
> we generate the ContentLength header to the time
> we are able to read and send the file.  It is not
> trivial to change this because of of the multiprocess
> architecture.  We could always use chunked encodings
> for files and thus avoid the problem.

Chunked transfer encoding is a HTTP 1.1 feature and for file uploads you need to
implement a fallback to plain uploads if some proxy or origin server is HTTP
1.0. So it is trickier than just doing it in UploadDataStream. Hence my thought
that it should be done as part of the feature which will require it.

> 1. Does your code support rewinding the UploadDataStream?

I heard rewind is done by creating a new UploadDataStream from the same
UploadData. In that case, yes this code handles it.

> 2. I still prefer using (NULL, 0) to set the last
> chunk. It seems that you're already doing that except
> in URLRequest, correct?

Yes, UploadData does it as you suggest. URLRequest provides an abstracted method
which allows URLFetcher and others to not care about these magic parameters.

> 4. I suggest shortening ChunkCallback to Delegate.

I'm not comfortable with calling this a Delegate when it is set by the caller
only for chunked uploads. How about I remove these and just use a Task* for the
callback in both UploadData and UploadDataStream? The Task* will be kept alive
and invoked multiple times throughout this object's lifetime.

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/net/net_pref...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/net/net_pref...
chrome/browser/net/net_pref_observer.cc:41:
prefs->RegisterBooleanPref(prefs::kDisableSpdy, true);
On 2011/01/14 05:53:44, vandebo wrote:
> Is this a debugging change that shouldn't be in the final CL?

Yes I will remove it. But I also wanted to ask how to force a particular
URLFetcher/URLRequest session to not use SPDY, since for the speech upload case
I really want to use plain http with chunked encoding. Is there any way to turn
off SPDY on a per-request basis?

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/browser/speech/speec...
chrome/browser/speech/speech_recognizer.cc:200: /*
On 2011/01/14 05:53:44, vandebo wrote:
> Should you delete this now dead code?

The changes in this file are just a sample showing how the network changes may
be used. I have now removed these from this CL

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:105: void AddUploadDataChunkInThread(const
std::string& data);
On 2011/01/14 05:53:44, vandebo wrote:
> Should this be private?

All these functions are in the private block, but URLFetcher is a friend and can
access them

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:137: bool is_chunked_upload_;           // True
if using chunked transfer encoding
On 2011/01/14 03:09:31, wtc wrote:
> Please document this issue:
>   If is_chunked_upload_ is true, is upload_content_ ignored?

No, only one of them can be valid and the setters ensure that. So the rest of
the code takes minimal precautions about them

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:314: void URLFetcher::Core::MarkEndOfChunks() {
On 2011/01/14 05:53:44, vandebo wrote:
> Seems you don't need both of these in the ::core class.  You could just as
> easily call ::Core::AppendChunkToUpload("") from URLFetcher::MarkEndOfChunks.

Done.

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:520: core_->is_chunked_upload_ = false;
On 2011/01/14 05:53:44, vandebo wrote:
> Seems that this should just be a DCHECK?

Done.

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6134003/diff/10001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:136: // Use add_upload_data_chunk() to give the
data chunks before or after calling
On 2011/01/14 05:53:44, vandebo wrote:
> You may want to note that this must be called before Start() is called.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:34: if (type_ == TYPE_BYTES || type_ == TYPE_CHUNK)
On 2011/01/14 05:53:44, vandebo wrote:
> GetContentLength doesn't make sense for TYPE_CHUNK.  Could you now
DCHECK(type_
> != TYPE_CHUNK) ?

This is UploadData::Element and GetContentLength makes sense for this class
since it just returns the length of bytes in this chunk alone.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:80: void UploadData::Element::SetToLastChunk() {
On 2011/01/14 03:09:31, wtc wrote:
> Let's name this method SetToEndOfChunksMarker.
> 
> In HTTP 1.1, "last chunk" means
>        last-chunk     = 1*("0") [ chunk-extension ] CRLF

Done.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:81: bytes_.clear();
On 2011/01/14 05:53:44, vandebo wrote:
> Doesn't this need to be "0\r\n\r\n"?  It might be easier to just call
> SetToChunk("", 0);

No, the previous chunk would be "0\r\n\r\n". This is the marker after that.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:120: is_chunked_(false) {
On 2011/01/14 05:53:44, vandebo wrote:
> Instead of an explicit is_chunked_, could you just check
> elements_.front().type() == TYPE_CHUNK?  Especially since it's only used for
> DCHECKs.

This flag is also used by UploadDataStream and URLRequest as the one place to
check if the upload is a chunked stream. And we need this flag for DCHECKs even
before any elements are added. Hence leaving it as is.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.cc#new...
net/base/upload_data.cc:157: // End of chunked stream is marked internally with
a chunk of length 0.
On 2011/01/14 03:09:31, wtc wrote:
> "a chunk of length 0" is not accurate.  You may want to be
> more precise: "a chunk with an empty bytes_".

Done.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:27: TYPE_CHUNK,  // More data is to come and this should
be sent immediately.
On 2011/01/14 03:09:31, wtc wrote:
> The comment should say:
>   The data should be sent in chunked encoding immediately,
>   without waiting for the other data.
> 
> "More data is to come" is confusing because it can be said
> of the other types of data.  For example, SetToBytes can be
> called multiple times, which means after each SetToBytes
> call, more data is to come.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:89: // items after this item.
On 2011/01/14 03:09:31, wtc wrote:
> Again, the difference is that a chunk should be sent in
> chunked encoding, and without waiting for the other chunks.
> 
> The fact that the stream has more such items after this item
> is also true of bytes.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data.h#newc...
net/base/upload_data.h:150: bool is_chunked() { return is_chunked_; }
On 2011/01/14 03:09:31, wtc wrote:
> Nit: mark this method 'const'.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:35: waiting_for_data_(false),
On 2011/01/14 03:09:31, wtc wrote:
> IMPORTANT: after studying the code, I think
> waiting_for_data_ should be maintained by UploadData
> rather than UploadDataStream.
> 
> UploadData should initialize waiting_for_data_ to false,
> and set waiting_for_data_ to true after it has added the
> end-of-chunks marker.
> 
> But I'm not convinced that waiting_for_data_ is even
> necessary.  It is only used in UploadDataStream::OnChunkAvailable().
> UploadDataStream::OnChunkAvailable() is called when a new
> chunk is added to the UploadData.  So by definition we must
> be waiting for data.

Good call, yes this flag is unnecessary now. It was added earlier when I was
using one-shot callbacks to verify the calling order, but with the current
always-on callback model no need to verify. I have changed the code accordingly.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:154: if (next_element_ == elements.size() &&
!buf_len_) {
On 2011/01/14 05:53:44, vandebo wrote:
> Could this conditional be true and the next false (leading to eof_ = true) if
we
> get to this point before the first chunk of data is provided?  next_element_
> will be 0, element.size() will be 0, but next_element_ won't be > 0.

There is an if statement at the very beginning of this function to catch this
case.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:159: elements[next_element_ - 1].bytes().size() >
0) {
On 2011/01/14 05:53:44, vandebo wrote:
> The last chunk won't have zero size at this point though, it should be
> "0\r\n\r\n", size 5.  Probably cleanest to add a bool and accessor to
> UploadData::Element to indicate the last chunk.  Either that, or you could
> introduce a TYPE_LAST_CHUNK, but that may not be as clean.

In http the last chunk is not zero bytes in size. But in our elements array the
"0\r\n\r\n" chunk is the last-but-one elment. The very last item in our array is
an end-of-stream marker chunk with a zero length array. Wan-Teh pointed out as
well that this is confusing and suggested a change in name and comments, I have
made those changes now.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.h:22: class ChunkCallback {
On 2011/01/14 05:53:44, vandebo wrote:
> This interface class is the same as UploadData::ChunkCallback.  Is there a
> reason to implement the same interface again?

Wan-Teh suggests I rename this to Delegate. I'm not comfortable with calling
this a Delegate when it is set by the caller only for chunked uploads. How about
I remove these and just use a Task* for the callback in both UploadData and
UploadDataStream? The Task* will be kept alive and invoked multiple times
throughout this object's lifetime.

http://codereview.chromium.org/6134003/diff/10001/net/base/upload_data_stream...
net/base/upload_data_stream.h:91: size_t next_element_;
On 2011/01/14 03:09:31, wtc wrote:
> Why does next_element_ need to be an array index?  Is it
> because you need it to stay valid after reaching the end of
> array?  (In contrast, an iterator becomes end() and can't be
> incremented any more.)

Yes

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:14: #include "net/base/load_flags.h"
On 2011/01/14 03:09:31, wtc wrote:
> Nit: remove this header.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:71: if (request_body)
On 2011/01/14 03:09:31, wtc wrote:
> Nit: request_body => request_body_

Done.

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:72: request_body_->set_chunk_callback(this);
On 2011/01/14 05:53:44, vandebo wrote:
> set_chunk_callback will be called even when not using chunked encoding?  In
that
> case, would it be better to name it something else? 
> UploadDataStream::set_data_consumer(...) ?

Changed now to only set for chunked streams

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:150: OnIOComplete(0);
On 2011/01/14 03:09:31, wtc wrote:
> IMPORTANT: this should do nothing unless we're in the
> STATE_SENDING_BODY state:
>   if (io_state_ == STATE_SENDING_BODY)
>     OnIOComplete(0);
> 
> An alternative solution is to move the call
>   request_body_->set_chunk_callback(this);
> from line 72 to line 248, when we transition into the
> STATE_SENDING_BODY.
> 
> The problem is that when a new chunk is added, this class
> may still be in the STATE_SENDING_HEADERS state.  If we
> call OnIOComplete(0) without checking, we will be calling
> DoSendHeaders.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:150: OnIOComplete(0);
On 2011/01/14 05:53:44, vandebo wrote:
> Indeed, this mustn't do anything unless io_state == STATE_SENDING_BODY

Done.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.cc:166: DCHECK(bytes_len > 0);
On 2011/01/14 03:09:31, wtc wrote:
> Nit: use DCHECK_GT here.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.cc:332: if (is_chunked_upload_) {
On 2011/01/14 05:53:44, vandebo wrote:
> Should this go into "set_chunked_upload" ?

Done.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:318: // data becomes available it can be sent to
the server.
On 2011/01/14 03:09:31, wtc wrote:
> Nit: change "POST data" to "upload data" (two occurrences) in
> this comment.
> 
> It seems that the main reason for chunked upload is that the
> data is not available upfront, but rather that we want to
> avoid the delay of buffering all the data.  If delay is not
> an issue, you can accumulate all the data and then upload
> it the old way.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:320: // This method can be called only after a
call to |set_chunked_upload(true)|
On 2011/01/14 03:09:31, wtc wrote:
> Typo: your set_chunked_upload method takes no argument.

Done.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:322: void AppendChunkToUpload(const char* bytes,
int bytes_len);
On 2011/01/14 05:53:44, vandebo wrote:
> This method seems different than the other AppendFoo methods.  Maybe different
> enough that its name should not imply that it is the same as them?

Looking at the implementation I don't see why this method is very different from
the rest. Could you clarify?

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:324: // Indicate the end of a chunked-transfer
encoded request body.
On 2011/01/14 03:09:31, wtc wrote:
> Nit: Indicate => Indicates

Done.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:325: void MarkEndOfChunks();
On 2011/01/14 05:53:44, vandebo wrote:
> Is it really worthwhile to provide this method?  Why not just note in the
> comments for AppendChunkToUpload that a zero length chunk is needed to signal
> the end of chunked-transfer?

The consumers of this class don't have to know about the chunked-transfer format
itself. Asking them to set 2 parameters to a magic value doesn't seem like a
good choice and brings memories of similar win32 apis :/ I could inline this
method if required so there is no change in terms of generated code.

http://codereview.chromium.org/6134003/diff/10001/net/url_request/url_request...
net/url_request/url_request.h:446: void set_chunked_upload() {
is_chunked_upload_ = true; }
On 2011/01/14 03:09:31, wtc wrote:
> Nit: set_chunked_upload => enable_chunked_upload
>                         or set_is_chunked_upload

Renamed to EnableChunkedUpload(), moved to .cc file and created UploadData in
there as suggested. This also meant I could remove the 'is_chunked_upload_' flag
now.

[Satish, 2011-01-14 20:24:23]

I thought again about rewinding the UploadDataStream for TCP RST cases and
retrying if a HTTP 1.0 entity responded with an error. It seems like moving
the chunked-encoding part to UploadDataStream is a simple way to handle both
cases. Here is my reasoning:

- Let UploadDataStream::FillBuf do the encoding of byte blocks into chunks
with the prefix/postfix.
- Rollback all changes made to UploadData and add the chunks as plain byte
array elements. So UploadData will be buffering all the bytes of the chunked
upload and the application layer doesn't need its own buffer for retries.
- If a TCP RST arrives, the existing code already handles it by recreating
UploadDataStream.
- If a HTTP 1.0 entity responds with an error for the chunked-transfer
encoding header, HttpStreamParser can wait until end-of-stream is reached in
UploadData and then restart the request by creating a new UploadDataStream
WITHOUT the is-chunked-upload flag enabled. So it will become a plain
upload.

This handles both the audio streaming case and file upload/random data
upload cases reasonably.

If you guys like this idea, I can make the changes and send for review
again.

[wtc, 2011-01-15 00:54:28]

Satish: I believe your current approach can support rewinding the
UploadDataStream.  If so, then the only issue is whether we need
to handle a server that doesn't support chunked encoding.

I hope that you have a priori knowledge that a server supports
chunked encoding.  This is definitely the case for your speech
recognition server.

You can certainly try to implement the new approach and see if
it complicates the code.  If not, that'll be a win.  But I don't
insist that you do that.

[Satish, 2011-01-17 16:41:10]

> You can certainly try to implement the new approach and see if
> it complicates the code.  If not, that'll be a win.  But I don't
> insist that you do that.

I tested this code with a HTTP/1.0 proxy server. The server resets the
connection without any appropriate response (ERROR_CONNECTION_RESET in code) but
we can't identify if chunking was the issue or something else. So I think it
will be cleaner to retry in the application layer where the calling code (speech
input in this case) can decide how to handle the case.

I'll wait for review comments on the latest patch. Also could you please suggest
how to get this working with SPDY? I'm wondering how to force a particular
URLFetcher/URLRequest session to not use SPDY, since for the speech upload case
I really want to use plain http with chunked encoding. Is there any way to turn
off SPDY on a per-request basis?

[wtc, 2011-01-18 18:48:58]

On 2011/01/17 16:41:10, Satish wrote:
> Also could you please suggest
> how to get this working with SPDY? I'm wondering how to force a particular
> URLFetcher/URLRequest session to not use SPDY, since for the speech upload
case
> I really want to use plain http with chunked encoding. Is there any way to
turn
> off SPDY on a per-request basis?

I don't know the answer to this question.  Also, I don't understand
why SPDY would be a problem for your speech upload case.  In any case,
please email mbelshe and willchan for their expert opinions on these
two issues.

[vandebo (ex-Chrome), 2011-01-18 21:51:17]

Basically looks good, just a couple little things.  Feel free to ignore anything
marked nit.

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:108: // before or after Start() is called.
The header says this can be called only after Start().

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:156: if (!bytes_len) {
nit: This uses two Elements to represent the last chunk.  It might be more
intuitive to add a last chunk bool to Element.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:16: #include "net/base/completion_callback.h"
This header isn't needed.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:86: if (bytes_copied > 0)
note: encoding end of chunks with a single element would mean you don't need
this addition.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:10: #include "net/base/completion_callback.h"
This header isn't needed.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:18: class UploadDataStream : public
UploadData::ChunkCallback {
nit: This class doesn't really have to implement ChunkCallback.  You could just
have UploadDataStream::set_chunk_callback call data_->set_chunk_callback.  And
change DidConsume to just do FillBuf if the amount consumed was 0 and call
FillBuf in UploadDataStream::OnChunkAvailable regardless of the amount consumed.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:22: class ChunkCallback {
This is the same interface as UploadData::ChunkCallback.  Is there a good reason
to re-define it?

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:47: // Sets the callback to be invoked when new
data is available to upload.
This comment is out dated, please update.

http://codereview.chromium.org/6134003/diff/37001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:152: // so nothing to do here.
nit:  You could DCHECK(io_state_ == STATE_SENDING_HEADERS || io_state_ ==
STATE_SENDING_BODY)

[Satish, 2011-01-19 16:05:24]

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:18: class UploadDataStream : public
UploadData::ChunkCallback {
On 2011/01/18 21:51:17, vandebo wrote:
> nit: This class doesn't really have to implement ChunkCallback.  You could
just
> have UploadDataStream::set_chunk_callback call data_->set_chunk_callback.  And
> change DidConsume to just do FillBuf if the amount consumed was 0 and call
> FillBuf in UploadDataStream::OnChunkAvailable regardless of the amount
consumed.

Could you clarify the last part? OnChunkAvailable won't be there anymore if this
doesn't implement UploadData::ChunkCallback, so I'm not sure what you mean here.

[vandebo (ex-Chrome), 2011-01-19 17:00:49]

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:18: class UploadDataStream : public
UploadData::ChunkCallback {
On 2011/01/19 16:05:25, Satish wrote:
> On 2011/01/18 21:51:17, vandebo wrote:
> > nit: This class doesn't really have to implement ChunkCallback.  You could
> just
> > have UploadDataStream::set_chunk_callback call data_->set_chunk_callback. 
And
> > change DidConsume to just do FillBuf if the amount consumed was 0 and call
> > FillBuf in UploadDataStream::OnChunkAvailable regardless of the amount
> consumed.
> 
> Could you clarify the last part? OnChunkAvailable won't be there anymore if
this
> doesn't implement UploadData::ChunkCallback, so I'm not sure what you mean
here.

Sorry, I mistyped.  I meant "... and call DidConsume in
HttpStreamParser::DoSendBody regardless of the amount consumed."

[vandebo (ex-Chrome), 2011-01-19 19:26:01]

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:18: class UploadDataStream : public
UploadData::ChunkCallback {
On 2011/01/19 17:00:49, vandebo wrote:
> On 2011/01/19 16:05:25, Satish wrote:
> > On 2011/01/18 21:51:17, vandebo wrote:
> > > nit: This class doesn't really have to implement ChunkCallback.  You could
> > just
> > > have UploadDataStream::set_chunk_callback call data_->set_chunk_callback. 
> And
> > > change DidConsume to just do FillBuf if the amount consumed was 0 and call
> > > FillBuf in UploadDataStream::OnChunkAvailable regardless of the amount
> > consumed.
> > 
> > Could you clarify the last part? OnChunkAvailable won't be there anymore if
> this
> > doesn't implement UploadData::ChunkCallback, so I'm not sure what you mean
> here.
> 
> Sorry, I mistyped.  I meant "... and call DidConsume in
> HttpStreamParser::DoSendBody regardless of the amount consumed."

It occurs to me, if you do decide to do this, you should probably change the
name of DidConsume to ConsumeAndFillBuffer or something similar.

[wtc, 2011-01-20 00:29:47]

LGTM.  Please do not check this in until you have addressed
vandebo's review comments.

I have some suggested changes below.

Like vandebo, I also wondered if it is possible for
UploadData to directly call back to HttpStreamParser,
skipping UploadDataStream.  This will require moving
ChunkCallback to the top level of the net namespace,
and making UploadDataStream::FillBuf public.  (Or you
need to make UploadDataStream::DidConsume(0) work and
rename it as vandebo suggested.)

Low-level comments follow.

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:105: void AddUploadDataChunkInThread(const
std::string& data);
Nit: we seem to use the naming convention CompleteXXX for
this kind of function.

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:296: if (content.length()) {
Nit: use content.length() or content.size() consistently
(see line 298).

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:527: core_->AppendChunkToUpload("");
Nit: "" => std::string()

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:140: // Adds the given bytes to a requests's
POST data transmitted using chunked
Nit: requests's => request's

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:144: void MarkEndOfChunks();
Document the MarkEndOfChunks method.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:10: #include "base/string_util.h"
Nit: list this header in alphabetical order.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:73: chunk_length.c_str() + chunk_length.length());
Nit: c_str() => data()

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:157: // End of chunked stream is marked by a chunk with
an empty |bytes_|.
Nit: chunked stream => chunks

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:166: DCHECK(!chunk_callback_);
IMPORTANT: I believe this DCHECK is too strong.  When you
rewind an upload data stream, you actually create a new
UploadDataStream object, which will call data_->set_chunk_callback
with a different 'callback' argument.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:36: // of data are received.
Nit: received => added

Make the same change to upload_data_stream.h.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:91: // Though similar to bytes, a chunk indicates that
the stream is sent via
Nit: stream => element

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:150: // Iniitalizes the object to receive chunks of
upload data over time rather
Nit: receive => send ?

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:71: }
I believe we can remove lines 68-71, if we make the change
I suggested to line 149 below.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:86: if (bytes_copied > 0)
memcpy should be able to copy 0 bytes.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:147: // If this is a chunked-transfer encoded
stream, the last element should
Nit: chunked-transfer => chunked transfer

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:149: if (!data_->is_chunked() ||
elements.back().bytes().size() == 0)
This should read:
    if (!data_->is_chunked() ||
        (!elements.empty() && elements.back().bytes().empty())) {
      eof_ = true;
    }

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.h:67: // Callback invoked by UploadData when new
data is available.
Nit: new data => new chunk

Make the same change to line 109 below.

http://codereview.chromium.org/6134003/diff/37001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:70: if (request_body_.get() &&
request_body_->is_chunked())
Nit: request_body_.get() => request_body_ != NULL
to match existing code (see lines 227 and 250 below).

http://codereview.chromium.org/6134003/diff/37001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:149: // This method may get called in any state
so check before processing the new
If you add the DCHECK vandebo suggested, please update
"in any state" in this comment to match the DCHECK.

http://codereview.chromium.org/6134003/diff/37001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:223: HEADER_ONLY = 1,    // There is only a
header packet (can't coalesce).
In light of your change to line 227, the HEADER_ONLY enum
should also be documented to include the chunked encoding
case, which has a request body but the request body isn't
available yet.

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.cc:179: DCHECK(upload_);
Nit: also DCHECK upload_->is_chunked(), as you do on line 173
above.

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.h:315: // Indicates that the request body should be
sent using Chunked-Transfer
Nit: Chunked-Transfer => chunked transfer

This comment also applies to line 320 and line 327 below.

The HTTP 1.1 RFC (RFC 2616) actually hyphenates it like this:
  chunked transfer-coding
I would omit the hyphen.

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.h:319: // Appends the given bytes to the requests's
upload data to be sent
Nit: requests's => request's

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.h:321: // call MarkEndOfChunks() to indicate the end
of stream.
Nit: end of stream => end of upload data
                or    end of request body

[willchan no longer on Chromium, 2011-01-20 08:51:03]

In order to get this working for SPDY, you should look into changing
SpdyHttpStream and SpdyStream.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:37: class ChunkCallback {
I don't understand this. Why don't you just typedef Callback0::Type to
ChunkCallback? Then you could also use ScopedCallbackFactory to make the
callback cancelation safe. This seems like it could be dangerous if it lives
longer than the UploadDataStream.

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.h:315: // Indicates that the request body should be
sent using Chunked-Transfer
Push back if you feel I'm being unreasonable. I just saw this and felt like I
had to chime in. We're adding an extra 3 member functions to this API. When
reading this API, it's not obvious what's the difference between all of these
APIs. It also seems to be the case that the UploadData* itself is directly
exposed.

I'm a bit concerned about the complication of the API. I feel like perhaps there
should be just two functions, a simple one and a complicated one. The simple one
could just be AppendBytesToUpload. For everything else, I think URLRequest
should just expose the UploadData since that's what we're doing already.

I'm fine with having these member functions be in URLFetcher, since that
object's purpose is to help simplify access to URLRequest from chrome/browser,
and it's far less cluttered than URLRequest. But I personally would like to push
back on additions to the URLRequest interface except where absolutely necessary.

[Satish, 2011-01-20 18:02:35]

Most comments addressed and latest patch uploaded, a few replies below.

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:105: void AddUploadDataChunkInThread(const
std::string& data);
On 2011/01/20 00:29:47, wtc wrote:
> Nit: we seem to use the naming convention CompleteXXX for
> this kind of function.

Any examples? Most functions I see are of the form OnXxxCompleted which is
different from what is done here

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:156: if (!bytes_len) {
On 2011/01/18 21:51:17, vandebo wrote:
> nit: This uses two Elements to represent the last chunk.  It might be more
> intuitive to add a last chunk bool to Element.

I avoided doing that because it would take up unwanted space in each element,
whether or not chunked encoding was used. Almost all uses of this class would be
for non-chunked encoded data and when chunked encoding is used in speech we
might typically have dozens of chunks for a few seconds of audio. All of these
will have this flag set to false which seems like a waste of space.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:37: class ChunkCallback {
On 2011/01/20 08:51:04, willchan wrote:
> I don't understand this. Why don't you just typedef Callback0::Type to
> ChunkCallback? Then you could also use ScopedCallbackFactory to make the
> callback cancelation safe. This seems like it could be dangerous if it lives
> longer than the UploadDataStream.

I used a callback initially and was suggested an interface was more suitable
here. I do agree on the live-longer-than-ChunkCallback part. @wtc, would you be
ok if I switched to a Callback as willchan suggests?

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.h:315: // Indicates that the request body should be
sent using Chunked-Transfer
On 2011/01/20 08:51:04, willchan wrote:
> I'm fine with having these member functions be in URLFetcher, since that
> object's purpose is to help simplify access to URLRequest from chrome/browser,
> and it's far less cluttered than URLRequest. But I personally would like to
push
> back on additions to the URLRequest interface except where absolutely
necessary.

I'm happy to make this change. @wtc, @vandebo - are you ok if I change the
URLRequest API as suggested here?

[willchan no longer on Chromium, 2011-01-20 19:42:36]

Btw, I was just looking for the SPDY integration mostly. Just happened to
note those other things. My non-SPDY comments are purely advisory and I
defer to wtc and vandebo here, since I haven't spent the time to read things
in detail.

On Thu, Jan 20, 2011 at 10:02 AM, <satish@chromium.org> wrote:

> Most comments addressed and latest patch uploaded, a few replies below.
>
>
>
>
>
http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
> File chrome/common/net/url_fetcher.cc (right):
>
>
>
http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
> chrome/common/net/url_fetcher.cc:105: void
> AddUploadDataChunkInThread(const std::string& data);
> On 2011/01/20 00:29:47, wtc wrote:
>
>> Nit: we seem to use the naming convention CompleteXXX for
>> this kind of function.
>>
>
> Any examples? Most functions I see are of the form OnXxxCompleted which
> is different from what is done here
>
> http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc
> File net/base/upload_data.cc (right):
>
>
>
http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
> net/base/upload_data.cc:156: if (!bytes_len) {
>
> On 2011/01/18 21:51:17, vandebo wrote:
>
>> nit: This uses two Elements to represent the last chunk.  It might be
>>
> more
>
>> intuitive to add a last chunk bool to Element.
>>
>
> I avoided doing that because it would take up unwanted space in each
> element, whether or not chunked encoding was used. Almost all uses of
> this class would be for non-chunked encoded data and when chunked
> encoding is used in speech we might typically have dozens of chunks for
> a few seconds of audio. All of these will have this flag set to false
> which seems like a waste of space.
>
>
> http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
> File net/base/upload_data.h (right):
>
>
>
http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
> net/base/upload_data.h:37: class ChunkCallback {
> On 2011/01/20 08:51:04, willchan wrote:
>
>> I don't understand this. Why don't you just typedef Callback0::Type to
>> ChunkCallback? Then you could also use ScopedCallbackFactory to make
>>
> the
>
>> callback cancelation safe. This seems like it could be dangerous if it
>>
> lives
>
>> longer than the UploadDataStream.
>>
>
> I used a callback initially and was suggested an interface was more
> suitable here. I do agree on the live-longer-than-ChunkCallback part.
> @wtc, would you be ok if I switched to a Callback as willchan suggests?
>
>
>
>
http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request.h
> File net/url_request/url_request.h (right):
>
>
>
http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
> net/url_request/url_request.h:315: // Indicates that the request body
> should be sent using Chunked-Transfer
> On 2011/01/20 08:51:04, willchan wrote:
>
>> I'm fine with having these member functions be in URLFetcher, since
>>
> that
>
>> object's purpose is to help simplify access to URLRequest from
>>
> chrome/browser,
>
>> and it's far less cluttered than URLRequest. But I personally would
>>
> like to push
>
>> back on additions to the URLRequest interface except where absolutely
>>
> necessary.
>
> I'm happy to make this change. @wtc, @vandebo - are you ok if I change
> the URLRequest API as suggested here?
>
>
> http://codereview.chromium.org/6134003/
>

[vandebo (ex-Chrome), 2011-01-20 20:48:45]

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.cc#new...
net/base/upload_data.cc:156: if (!bytes_len) {
On 2011/01/20 18:02:36, Satish wrote:
> On 2011/01/18 21:51:17, vandebo wrote:
> > nit: This uses two Elements to represent the last chunk.  It might be more
> > intuitive to add a last chunk bool to Element.
> 
> I avoided doing that because it would take up unwanted space in each element,
> whether or not chunked encoding was used. Almost all uses of this class would
be
> for non-chunked encoded data and when chunked encoding is used in speech we
> might typically have dozens of chunks for a few seconds of audio. All of these
> will have this flag set to false which seems like a waste of space.

There are already several bools in the Element class.  Adding one more is
unlikely to change the storage size because compilers align fields in
structures.  If this was your only concern, then please consider changing it.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:86: if (bytes_copied > 0)
On 2011/01/20 00:29:47, wtc wrote:
> memcpy should be able to copy 0 bytes.

As I recall, it does cause problems on some platforms though.

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/url_request/url_request...
net/url_request/url_request.h:315: // Indicates that the request body should be
sent using Chunked-Transfer
On 2011/01/20 18:02:36, Satish wrote:
> On 2011/01/20 08:51:04, willchan wrote:
> > I'm fine with having these member functions be in URLFetcher, since that
> > object's purpose is to help simplify access to URLRequest from
chrome/browser,
> > and it's far less cluttered than URLRequest. But I personally would like to
> push
> > back on additions to the URLRequest interface except where absolutely
> necessary.
> 
> I'm happy to make this change. @wtc, @vandebo - are you ok if I change the
> URLRequest API as suggested here?

Feel free to make any changes that you and Will agree on here.

Will, it seems that it might be better to clean up this API in a separate CL.  I
see your point about UploadData being exposed, but it seems that Satish has
followed the convention that's already been established here.

[Satish, 2011-01-20 20:55:28]

Cool, I'll add the boolean to Element. I can also simplify the URLRequest
API in a subsequent CL as suggested by Will if that is ok.

[willchan no longer on Chromium, 2011-01-21 01:36:00]

On 2011/01/20 20:55:28, Satish wrote:
> Cool, I'll add the boolean to Element. I can also simplify the URLRequest
> API in a subsequent CL as suggested by Will if that is ok.

If other people agree with my point, then as long as you're with cleaning up the
API later, then I'm fine with it being done in a subsequent CL. I just don't
want to have a slow build up of cruft and have no one pay down the refactoring
debt. I think vandebo is right that you're just following the existing paradigm,
but eventually it gets too crufty and we have to clean it up. Just my two cents.
Thanks!

[Satish, 2011-01-21 16:55:04]

Latest patch uploaded.

I have added a boolean to UploadData::Element to indicate the last chunk as
vandebo@ suggested. I have also added a unit test and relevant changes to
net/tools/testserver/testserver.py for testing chunked uploads.

If it looks good I'll submit it and start working on the SPDY part as well as
URLRequest API simplification as suggested by willchan@

[vandebo (ex-Chrome), 2011-01-21 18:08:41]

LGTM contingent on settling the callback issue with Will and wtc.

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:37: class ChunkCallback {
On 2011/01/20 18:02:36, Satish wrote:
> On 2011/01/20 08:51:04, willchan wrote:
> > I don't understand this. Why don't you just typedef Callback0::Type to
> > ChunkCallback? Then you could also use ScopedCallbackFactory to make the
> > callback cancelation safe. This seems like it could be dangerous if it lives
> > longer than the UploadDataStream.
> 
> I used a callback initially and was suggested an interface was more suitable
> here. I do agree on the live-longer-than-ChunkCallback part. @wtc, would you
be
> ok if I switched to a Callback as willchan suggests?

Will implied that using Callback0::Type would would be safer in some ways.  Was
this resolved out of band?

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data.h#newc...
net/base/upload_data.h:122: GURL blob_url_;
nit: Are the :1's needed?  I suspect not.

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data.h#newc...
net/base/upload_data.h:145: void AppendChunk(const char* bytes, int bytes_len);
nit: Add a comment about a zero size chunk indicating the last chunk.

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data_stream...
net/base/upload_data_stream.cc:76: if (bytes_copied)
nit: I think you can remove this now that you don't have any zero size chunks.

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:641: TEST_F(URLRequestTestHTTP,
PostChunkedDataTest) {
If I read this test right, it looks like all the data will be queued before
HttpStreamParser tries to access any of it.  It would also be good to have a
test where HttpStreamParser will exercise the ERR_IO_PENDING during upload case.

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:649: r.AppendChunkToUpload("a", 1);
I thought you weren't supposed to call AppendChunkToUpload before starting the
request.

[willchan no longer on Chromium, 2011-01-21 18:16:19]

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37001/net/base/upload_data.h#newc...
net/base/upload_data.h:37: class ChunkCallback {
On 2011/01/21 18:08:41, vandebo wrote:
> On 2011/01/20 18:02:36, Satish wrote:
> > On 2011/01/20 08:51:04, willchan wrote:
> > > I don't understand this. Why don't you just typedef Callback0::Type to
> > > ChunkCallback? Then you could also use ScopedCallbackFactory to make the
> > > callback cancelation safe. This seems like it could be dangerous if it
lives
> > > longer than the UploadDataStream.
> > 
> > I used a callback initially and was suggested an interface was more suitable
> > here. I do agree on the live-longer-than-ChunkCallback part. @wtc, would you
> be
> > ok if I switched to a Callback as willchan suggests?
> 
> Will implied that using Callback0::Type would would be safer in some ways. 
Was
> this resolved out of band?

Callback0::Type by itself isn't safer. But ScopedCallbackFactory will produce
callbacks of type Callback0::Type, so switching to Callback0::Type lets us use
ScopedCallbackFactory in UploadDataStream. The problem is, if UploadDataStream
is destroyed before UploadData is (it's not immediately clear to me the lifetime
semantics since this is refcounted), then calling into the chunk callback may
call into a dead object.

That said, it occurs to me now that that would require the callback to be heap
allocated, so UploadData would have to own the callback. Perhaps it's simply
better for UploadDataStream to call set_callback(NULL).

[wtc, 2011-01-21 19:35:54]

LGTM.

Please consider willchan's suggested changes.  He is very
up-to-date on the new tools in the source tree to solve common
problems.

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6134003/diff/37001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:105: void AddUploadDataChunkInThread(const
std::string& data);
On 2011/01/20 18:02:36, Satish wrote:
>
> Any examples? Most functions I see are of the form OnXxxCompleted which is
> different from what is done here

I found only four examples:
- src/chrome/browser/ssl/ssl_error_handler.cc
- src/chrome/browser/in_process_webkit/dom_storage_context.cc
- src/chrome/browser/transport_security_persister.cc
- src/chrome/browser/shell_integration.cc
So the CompleteXXX convention isn't as widely used as I
thought.  But I haven't seen XXXInThread used before.

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data.h#newc...
net/base/upload_data.h:122: bool is_last_chunk_ : 1;
I also recommend removing ": 1".  By listing the bool members
consecutively, you already allow the compiler to pack them.

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data_stream...
net/base/upload_data_stream.h:33: void ConsumeAndFillBuffer(size_t num_bytes);
How about "MarkConsumedAndFillBuffer"?

http://codereview.chromium.org/6134003/diff/37002/net/base/upload_data_stream...
net/base/upload_data_stream.h:36: void set_chunk_callback(ChunkCallback*
callback);
Nit: inline this method, as you inlined is_chunked().

http://codereview.chromium.org/6134003/diff/37002/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/6134003/diff/37002/net/http/http_stream_parser...
net/http/http_stream_parser.h:74: // UploadData::ChunkCallback methods.
Remove UploadData::

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
net/tools/testserver/testserver.py:598: """both plain and chunked transfer
encoded requests."""
Your multi-line comment format seems non-ideal.  Please see
the example below at lines 616-617.

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
net/tools/testserver/testserver.py:603: # Read the response body as chunks.
Typo: response body => request body

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
net/tools/testserver/testserver.py:609: break
Nit: you can check for "0\r\n" below, after line 610, by
checking if length == 0.

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
net/tools/testserver/testserver.py:610: length = int(line, 16)
What does the argument "16" mean?

[Satish, 2011-01-21 20:29:57]

Thanks all, I'll add the suggested changes and upload the latest patch on monday
(planning to submit this change only after branch point). Some replies below:

> Perhaps it's simply better for UploadDataStream to call
> set_callback(NULL).

In the latest code it is HttpStreamParser which calls
UploadDataStream::set_chunk_callback and that simply proxies it to
UploadData::set_chunk_callback. I'll add a set_chunk_callback(NULL) in
HttpStreamParser's dtor.

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
net/tools/testserver/testserver.py:598: """both plain and chunked transfer
encoded requests."""
On 2011/01/21 19:35:54, wtc wrote:
> Your multi-line comment format seems non-ideal.  Please see
> the example below at lines 616-617.

Sure :) I saw lines 558-560 which were in this format, will change

http://codereview.chromium.org/6134003/diff/37002/net/tools/testserver/testse...
net/tools/testserver/testserver.py:610: length = int(line, 16)
On 2011/01/21 19:35:54, wtc wrote:
> What does the argument "16" mean?

The second argument is the base and we pass 16 since the chunk length is sent in
hex

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:641: TEST_F(URLRequestTestHTTP,
PostChunkedDataTest) {
On 2011/01/21 18:08:41, vandebo wrote:
> If I read this test right, it looks like all the data will be queued before
> HttpStreamParser tries to access any of it.  It would also be good to have a
> test where HttpStreamParser will exercise the ERR_IO_PENDING during upload
case.

Makes sense. I could do that in this same test by posting a task before calling
r.Start() and in that task function add 2 of these chunks. That should allow
URLRequest to process the existing chunks and return to the message loop before
handling the last 2 chunks. Is that ok?

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:649: r.AppendChunkToUpload("a", 1);
On 2011/01/21 18:08:41, vandebo wrote:
> I thought you weren't supposed to call AppendChunkToUpload before starting the
> request.

Good catch, yes the code is allowing these calls before Start.. the condition is
that EnableChunkedUpload should have been called. I'll update the comment in
URLRequest to mention this.

[vandebo (ex-Chrome), 2011-01-21 20:47:12]

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/6134003/diff/37002/net/url_request/url_request...
net/url_request/url_request_unittest.cc:649: r.AppendChunkToUpload("a", 1);
On 2011/01/21 20:29:57, Satish wrote:
> On 2011/01/21 18:08:41, vandebo wrote:
> > I thought you weren't supposed to call AppendChunkToUpload before starting
the
> > request.
> 
> Good catch, yes the code is allowing these calls before Start.. the condition
is
> that EnableChunkedUpload should have been called. I'll update the comment in
> URLRequest to mention this.

Having two tests would yield better test coverage.  One with all the chunks
there before HttpStreamParser first looks, and another with none there.  That
would test both cases of the ERR_IO_PENDING mechanism as well as the base
case/exception of the EOF logic.

[Satish, 2011-01-24 19:42:49]

Latest patch uploaded. I have addressed all pending comments and added 2 unit
tests as suggested by vandebo@.

With regards to using a Callback0::Type for the chunk callback, I'm taking
willchan's simpler suggestion and calling set_chunk_callback(NULL) once done (in
HttpStreamParser's dtor).

Please confirm if this looks good and I'll submit. I'll send the SPDY changes to
willchan in a subsequent CL.

[vandebo (ex-Chrome), 2011-01-24 20:16:08]

LGTM with two comments.  No further LG's needed from me.

http://codereview.chromium.org/6134003/diff/76001/chrome_frame/urlmon_upload_...
File chrome_frame/urlmon_upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/76001/chrome_frame/urlmon_upload_...
chrome_frame/urlmon_upload_data_stream.cc:54:
request_body_stream_->ConsumeAndFillBuffer(bytes_to_copy_now);
You missed updating one name.

http://codereview.chromium.org/6134003/diff/76001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6134003/diff/76001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:132: // If this is a chunked transfer encoded
stream, the last element should
This comment is out of date.  Probably just remove it as the new code is self
documenting.

[willchan no longer on Chromium, 2011-01-24 20:24:34]

I'm fine with landing as is. If I have any issues, I'll raise them in the
follow up changelist.

On Mon, Jan 24, 2011 at 12:16 PM, <vandebo@chromium.org> wrote:

> LGTM with two comments.  No further LG's needed from me.
>
>
>
>
http://codereview.chromium.org/6134003/diff/76001/chrome_frame/urlmon_upload_...
> File chrome_frame/urlmon_upload_data_stream.cc (right):
>
>
>
http://codereview.chromium.org/6134003/diff/76001/chrome_frame/urlmon_upload_...
> chrome_frame/urlmon_upload_data_stream.cc:54:
> request_body_stream_->ConsumeAndFillBuffer(bytes_to_copy_now);
> You missed updating one name.
>
>
>
http://codereview.chromium.org/6134003/diff/76001/net/base/upload_data_stream.cc
> File net/base/upload_data_stream.cc (right):
>
>
>
http://codereview.chromium.org/6134003/diff/76001/net/base/upload_data_stream...
> net/base/upload_data_stream.cc:132: // If this is a chunked transfer
>
> encoded stream, the last element should
> This comment is out of date.  Probably just remove it as the new code is
> self documenting.
>
>
> http://codereview.chromium.org/6134003/
>