net: Make UploadDataStream::Read() asynchronous

net: Make UploadDataStream::Read() asynchronous

Rename existing Read() to ReadSync()
Change type of |buf| from char* to scoped_refptr<IOBuffer> so that async operation is always performed safely
Add an asynchronous implementation, Read()
The newly added Read() is not used yet, all users still use the old version ReadSync()

BUG=72001
TEST=net_unittests

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

[hashimoto, 2012-09-24 12:49:12]

Could you review this?

[willchan no longer on Chromium, 2012-09-24 23:37:08]

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:72: // with ReadSync() since it requires special
logic.
Are we going to be able to fix the chunked data to be async eventually? If so,
is there a bug open for that?

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:236: // Post task to the message loop to keep the
execution order unchanged even if
What does this mean?

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:179: if (BytesRemaining() <= 0)
Why <= 0? It returns a uint64.

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:182: // Save the value of
file_stream_.get() before base::Passed() invalidats it.
invalidates

Also, why does this make sense? file_stream_.get() gets called before
base::Passed(&file_stream_) anyway.

http://codereview.chromium.org/10910268/diff/16001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10910268/diff/16001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.cc:286: const int bytes_read =
request_body_stream_->ReadSync(
It'd be great if we could switch to Read() instead in the future.

[hashimoto, 2012-09-25 10:47:56]

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:72: // with ReadSync() since it requires special
logic.
On 2012/09/24 23:37:08, willchan wrote:
> Are we going to be able to fix the chunked data to be async eventually? If so,
> is there a bug open for that?

Filed http://crbug.com/152148

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:236: // Post task to the message loop to keep the
execution order unchanged even if
On 2012/09/24 23:37:08, willchan wrote:
> What does this mean?
Sorry for my badly written comment.
Introduced more sophisticated way to determine if callback should be run or not.

Running the callback directly here may change the execution order of the
callback and the code placed after Read(), please consider the following
example.  What we want is always outputting "BeginEnd".

If there are functions below:
  void FuncA(const base::Closure& callback) {
    PostTask(FROM_HERE, callback);
  }
  void FuncB(const base::Closure& callback) {
    callback.Run();
  }
  void Begin() { std::cout << "Begin"; }
  void End() { std::cout << "End"; }

The code below outputs "BeginEnd":
  FuncA(base::Bind(&End));
  Begin();

But, the code below outputs "EndBegin":
  FuncB(base::Bind(&End));
  Begin();

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:179: if (BytesRemaining() <= 0)
On 2012/09/24 23:37:08, willchan wrote:
> Why <= 0? It returns a uint64.

Replaced with '=='.

http://codereview.chromium.org/10910268/diff/16001/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:182: // Save the value of
file_stream_.get() before base::Passed() invalidats it.
On 2012/09/24 23:37:08, willchan wrote:
> invalidates
> 
> Also, why does this make sense? file_stream_.get() gets called before
> base::Passed(&file_stream_) anyway.

We must save the value here because the C++ standard does not specify the
evaluation order of function arguments. (ISO/IEC 14882:2011 8.3.6.9)
This can result in a real crash.
See http://crrev.com/156365 as an example.

http://codereview.chromium.org/10910268/diff/16001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10910268/diff/16001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.cc:286: const int bytes_read =
request_body_stream_->ReadSync(
On 2012/09/24 23:37:08, willchan wrote:
> It'd be great if we could switch to Read() instead in the future.

I will do it soon :)

[hashimoto, 2012-10-08 16:38:34]

willchan,
Could you revisit this change?
I think you are no longer super-busy now :)

[willchan no longer on Chromium, 2012-10-08 23:16:23]

Matt, not sure if you're overloaded, but I am :) I'm probably going to start
sending these UploadData[Stream] CLs your way.

[mmenke, 2012-10-08 23:18:21]

On 2012/10/08 23:16:23, willchan wrote:
> Matt, not sure if you're overloaded, but I am :) I'm probably going to start
> sending these UploadData[Stream] CLs your way.

SGTM.  Not at all overloaded this week (So far).

[mmenke, 2012-10-09 16:42:04]

On 2012/10/08 23:18:21, Matt Menke wrote:
> On 2012/10/08 23:16:23, willchan wrote:
> > Matt, not sure if you're overloaded, but I am :) I'm probably going to start
> > sending these UploadData[Stream] CLs your way.
> 
> SGTM.  Not at all overloaded this week (So far).

Probably won't get to commenting until tomorrow.  Skimmed over the upload stuff
before, but need to carefully go over it again.

[mmenke, 2012-10-11 18:05:40]

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:90: DCHECK_EQ(UploadElement::TYPE_BYTES,
element.type());
Wait...so we allow TYPE_BYTES to be added after construction, but not TYPE_FILE?
 Why?  Doesn't doing this mess up any total_size_ guarantee?  The header only
suggests total_size may be an overestimate, not an underestimate.  Do we have
integration tests for this?

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

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:21: class NET_EXPORT UploadDataStream {
While you're here, could you add a description of this class?  The whole
"UploadElement is a constant object containing information about data to be
read, UploadElementReader is a class used to read the data identified by such an
element, UploadData is a constant object with one or more UploadElements in
sequence, UploadDataStream is the object to read all elements from an UploadData
object" seems very non-obvious to me, and I think explanations at the top of
these files that let one understand this would rather useful.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:39: // Use this method only in tests and Chrome
Frame.
While you're here, mentioning Chrome frame in net/ is a layering violation. 
Suggest you mention this can cause jank instead.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:54: // moment, but more data to come at a later
time.
This comment isn't accurate:  HttpStreamParser allows chunked streams to be read
synchronously, and as of the CL after this one, it will also allow non-chunked
streams to be read asynchronously.  You should also say what |callback| is
called with (Bytes read or error code, presumably).

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:58: // Use this method only in tests and Chrome
Frame.
Mentioning Chrome Frame here is a layering violation.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_element_re...
net/base/upload_element_reader.h:55: // Use this method only in tests and Chrome
Frame.
Mentioning Chrome Frame in net/ is a layering violation.  Suggest you just say
something like "Using this method when IsInMemory() is false can cause jank."

[hashimoto, 2012-10-12 07:36:56]

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:90: DCHECK_EQ(UploadElement::TYPE_BYTES,
element.type());
On 2012/10/11 18:05:40, Matt Menke wrote:
> Wait...so we allow TYPE_BYTES to be added after construction, but not
TYPE_FILE?
We allow adding data after construction only if the data is chunked.
Since chunked data is always TYPE_BYTES, we DCHECK it here.
>  Why?  Doesn't doing this mess up any total_size_ guarantee?  The header only
> suggests total_size may be an overestimate, not an underestimate.  Do we have
> integration tests for this?
total_size_ is forced to be zero when the data is chunked.
We check this in UploadDataStreamTest.Chunk and other chunked data upload
realted tests.

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

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:21: class NET_EXPORT UploadDataStream {
On 2012/10/11 18:05:40, Matt Menke wrote:
> While you're here, could you add a description of this class?  The whole
> "UploadElement is a constant object containing information about data to be
> read, UploadElementReader is a class used to read the data identified by such
an
> element, UploadData is a constant object with one or more UploadElements in
> sequence, UploadDataStream is the object to read all elements from an
UploadData
> object" seems very non-obvious to me, and I think explanations at the top of
> these files that let one understand this would rather useful.

Added a comment about this class.
Or, should I also write the description about the relationship between these
four classes here?

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:39: // Use this method only in tests and Chrome
Frame.
On 2012/10/11 18:05:40, Matt Menke wrote:
> While you're here, mentioning Chrome frame in net/ is a layering violation. 
> Suggest you mention this can cause jank instead.

Stopped mentioning Chrome Frame.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:54: // moment, but more data to come at a later
time.
On 2012/10/11 18:05:40, Matt Menke wrote:
> This comment isn't accurate:  HttpStreamParser allows chunked streams to be
read
> synchronously, and as of the CL after this one, it will also allow non-chunked
> streams to be read asynchronously.  You should also say what |callback| is
> called with (Bytes read or error code, presumably).

Revised the comment.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.h:58: // Use this method only in tests and Chrome
Frame.
On 2012/10/11 18:05:40, Matt Menke wrote:
> Mentioning Chrome Frame here is a layering violation.

Done.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_element_re...
net/base/upload_element_reader.h:55: // Use this method only in tests and Chrome
Frame.
On 2012/10/11 18:05:40, Matt Menke wrote:
> Mentioning Chrome Frame in net/ is a layering violation.  Suggest you just say
> something like "Using this method when IsInMemory() is false can cause jank."

Revised the comment.

[mmenke, 2012-10-12 21:30:29]

Some more comments - didn't have time for a full round of comments today, so if
you prefer to wait to respond to them, feel free.

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:90: DCHECK_EQ(UploadElement::TYPE_BYTES,
element.type());
On 2012/10/12 07:36:56, hashimoto wrote:
> On 2012/10/11 18:05:40, Matt Menke wrote:
> > Wait...so we allow TYPE_BYTES to be added after construction, but not
> TYPE_FILE?
> We allow adding data after construction only if the data is chunked.
> Since chunked data is always TYPE_BYTES, we DCHECK it here.
> >  Why?  Doesn't doing this mess up any total_size_ guarantee?  The header
only
> > suggests total_size may be an overestimate, not an underestimate.  Do we
have
> > integration tests for this?
> total_size_ is forced to be zero when the data is chunked.
> We check this in UploadDataStreamTest.Chunk and other chunked data upload
> realted tests.
> 

None of this is documented anywhere.  Could we fix that?

I've inlined suggested places to mention the size behavior.  Also, in
upload_data.h, please mention that chunked upload data cannot include TYPE_FILE.
 Please also add a comment to that effect here, too.

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:72: // with ReadSync() since it requires special
logic.
"since it requires special logic, and is already in memory"

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

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
net/base/upload_data_stream.h:73: // for example, if the file is truncated.
Could you mention that size() returns 0 for chunked transfers?

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
net/base/upload_data_stream.h:130: // Size and current read position within the
upload data stream.
"Size is zero for chunked transfers."

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_file_eleme...
File net/base/upload_file_element_reader_unittest.cc (right):

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_file_eleme...
net/base/upload_file_element_reader_unittest.cc:90: EXPECT_EQ(bytes_, buf);
How about reading the rest of the file, too, in a second read?

http://codereview.chromium.org/10910268/diff/62001/net/http/http_stream_parse...
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/10910268/diff/62001/net/http/http_stream_parse...
net/http/http_stream_parser.cc:915: return
UploadProgress(request_body_->position(), request_body_->size());
size will be 0 for chunked uploads, but position will not be.  This should be
mentioned either in UploadProgress or in the header files (Both the stream
parsers's and in URLRequest's).

[hashimoto, 2012-10-15 11:21:37]

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/46001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:90: DCHECK_EQ(UploadElement::TYPE_BYTES,
element.type());
On 2012/10/12 21:30:29, Matt Menke wrote:
> On 2012/10/12 07:36:56, hashimoto wrote:
> > On 2012/10/11 18:05:40, Matt Menke wrote:
> > > Wait...so we allow TYPE_BYTES to be added after construction, but not
> > TYPE_FILE?
> > We allow adding data after construction only if the data is chunked.
> > Since chunked data is always TYPE_BYTES, we DCHECK it here.
> > >  Why?  Doesn't doing this mess up any total_size_ guarantee?  The header
> only
> > > suggests total_size may be an overestimate, not an underestimate.  Do we
> have
> > > integration tests for this?
> > total_size_ is forced to be zero when the data is chunked.
> > We check this in UploadDataStreamTest.Chunk and other chunked data upload
> > realted tests.
> > 
> 
> None of this is documented anywhere.  Could we fix that?
> 
> I've inlined suggested places to mention the size behavior.  Also, in
> upload_data.h, please mention that chunked upload data cannot include
TYPE_FILE.
>  Please also add a comment to that effect here, too.

Thank you for your kind suggestion.
Added comments for all mentioned places.

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:72: // with ReadSync() since it requires special
logic.
On 2012/10/12 21:30:29, Matt Menke wrote:
> "since it requires special logic, and is already in memory"

Done.

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

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
net/base/upload_data_stream.h:73: // for example, if the file is truncated.
On 2012/10/12 21:30:29, Matt Menke wrote:
> Could you mention that size() returns 0 for chunked transfers?

Done.

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_data_strea...
net/base/upload_data_stream.h:130: // Size and current read position within the
upload data stream.
On 2012/10/12 21:30:29, Matt Menke wrote:
> "Size is zero for chunked transfers."

Done.

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_file_eleme...
File net/base/upload_file_element_reader_unittest.cc (right):

http://codereview.chromium.org/10910268/diff/62001/net/base/upload_file_eleme...
net/base/upload_file_element_reader_unittest.cc:90: EXPECT_EQ(bytes_, buf);
On 2012/10/12 21:30:29, Matt Menke wrote:
> How about reading the rest of the file, too, in a second read?

Done for both ReadPartially and ReadPartiallyAsync.

http://codereview.chromium.org/10910268/diff/62001/net/http/http_stream_parse...
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/10910268/diff/62001/net/http/http_stream_parse...
net/http/http_stream_parser.cc:915: return
UploadProgress(request_body_->position(), request_body_->size());
On 2012/10/12 21:30:29, Matt Menke wrote:
> size will be 0 for chunked uploads, but position will not be.  This should be
> mentioned either in UploadProgress or in the header files (Both the stream
> parsers's and in URLRequest's).

Done for both HttpStreamParser and URLRequest.

[mmenke, 2012-10-15 19:54:45]

Thanks for bearing with me as I learn this code.  Sorry I've been a little slow
in getting back to you.

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_by...
File net/base/upload_bytes_element_reader.cc (right):

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_by...
net/base/upload_bytes_element_reader.cc:45: const CompletionCallback& callback)
{
DCHECK(!callback.is_null());

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
File net/base/upload_data.h (right):

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data.h:42: // key and ensure its destruction when UploadData is
finally deleted.
How about a description of how chunked uploading works here?  I found it rather
non-obvious, since it's completely different from how async reads are handled,
and it's not handled transparently by upload_data_steam, but rather must be
micromanaged by the stream consumer.

My suggestion (In another paragraph):  "Chunked uploads are handled by
repeatedly calling AppendBytes as data becomes available.  When more data
becomes available, the ChunkCallback's OnChunkAvailable function is called.  The
UploadData creator is then responsible for checking if the UploadData stream has
more data available, if necessary."

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data.h:63: // than all at once. Only bytes can be appended for
chunked data.
Think this statement is too weak, since other functions that modify elements
don't work for initializing chunk encoded streams, either.  Suggest just
"Chunked data may only contain bytes, not files."

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data.h:67: void set_last_chunk_appended(bool set) {
last_chunk_appended_ = set; }
Maybe a comment that "set_last_chunk_appended() is not safe to call once an
UploadData starts being read from by an UploadDataStream."

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
File net/base/upload_data_stream.cc (right):

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.cc:69: DCHECK(initialized_successfully_);
DCHECK(!callback.is_null());

Probably also a good idea to DCHECK the |buf_len| > 0 both here and in ReadSync.

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.cc:89: // We can initialize readeres synchrnously
here because only bytes can be
nit:  readeres -> readers
synchrnously -> synchronously

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.cc:91: // |toal_size_| because we treat the size of
chunked data as zero.
nit:  |toal_size_| -> |total_size_|

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.cc:91: // |toal_size_| because we treat the size of
chunked data as zero.
"because we treat the size of chunked data as zero" isn't quite right.  Maybe
just remove that sentence, now that we've spelled out that it's zero in the
header.  Alternatively, "We leave |total_size_| at zero, since for chunked
uploads, we may not know the total size".

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.cc:204: int bytes_copied,
I think this would be simpler if instead of passing around buf, buf_len, and
bytes_copied, we just created a DrainableIOBuffer (what you call sub_buffer)
before calling ReadInternal.  It already tracks buf_len (Its size() function)
and bytes_copies (Its BytesConsumed() function).

Also, I suggest making both the DrainableIOBuffer and the callback class
variables - this allows us to DCHECK that there's no other ReadInternal call
pending when Read and ReadSync are called, and is more consistent with other
net/ code.  You can also get rid of the invoked_asynchronously argument in that
case (Set callback after the call to ReadInternal, only if it returns
IO_PENDING).

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
File net/base/upload_data_stream.h (right):

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.h:44: // data stream to |buf| and returns the number
of bytes read, otherwise,
nit:  The comma before otherwise should be a semicolon.

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_da...
net/base/upload_data_stream.h:132: // total_size_ is set to zero when the data
is chunked.
nit:  |total_size_|

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_fi...
File net/base/upload_file_element_reader.cc (right):

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_fi...
net/base/upload_file_element_reader.cc:89: // file_stream is NULL if the target
file is
nit:  |file_stream|.

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_fi...
net/base/upload_file_element_reader.cc:180: const CompletionCallback& callback)
{
DCHECK(!callback.is_null());

[mmenke, 2012-10-15 21:34:01]

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_fi...
File net/base/upload_file_element_reader.cc (right):

https://chromiumcodereview.appspot.com/10910268/diff/74002/net/base/upload_fi...
net/base/upload_file_element_reader.cc:125: file_stream_.release()),
Are we leaking stuff here?  It looks like FileStreamWin, at least, only
automatically closes its Handle when created with the constructor that only
takes a NetLog as an argument, and FileStream does not use that constructor.

[hashimoto, 2012-10-16 11:52:19]

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_bytes_elem...
File net/base/upload_bytes_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_bytes_elem...
net/base/upload_bytes_element_reader.cc:45: const CompletionCallback& callback)
{
On 2012/10/15 19:54:45, Matt Menke wrote:
> DCHECK(!callback.is_null());

Done.

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

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data.h#new...
net/base/upload_data.h:42: // key and ensure its destruction when UploadData is
finally deleted.
On 2012/10/15 19:54:45, Matt Menke wrote:
> How about a description of how chunked uploading works here?  I found it
rather
> non-obvious, since it's completely different from how async reads are handled,
> and it's not handled transparently by upload_data_steam, but rather must be
> micromanaged by the stream consumer.
> 
> My suggestion (In another paragraph):  "Chunked uploads are handled by
> repeatedly calling AppendBytes as data becomes available.  When more data
> becomes available, the ChunkCallback's OnChunkAvailable function is called. 
The
> UploadData creator is then responsible for checking if the UploadData stream
has
> more data available, if necessary."

Thank you for this great documentation, though the comment will be soon removed
when http://crbug.com/152148 gets fixed after this change is landed :)
Done.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data.h#new...
net/base/upload_data.h:63: // than all at once. Only bytes can be appended for
chunked data.
On 2012/10/15 19:54:45, Matt Menke wrote:
> Think this statement is too weak, since other functions that modify elements
> don't work for initializing chunk encoded streams, either.  Suggest just
> "Chunked data may only contain bytes, not files."

Done.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data.h#new...
net/base/upload_data.h:67: void set_last_chunk_appended(bool set) {
last_chunk_appended_ = set; }
On 2012/10/15 19:54:45, Matt Menke wrote:
> Maybe a comment that "set_last_chunk_appended() is not safe to call once an
> UploadData starts being read from by an UploadDataStream."

This setter is exposed only for serialization code for IPC. (in
chrome/common/automation_messages.cc)
Added a comment.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:69: DCHECK(initialized_successfully_);
On 2012/10/15 19:54:45, Matt Menke wrote:
> DCHECK(!callback.is_null());
> 
> Probably also a good idea to DCHECK the |buf_len| > 0 both here and in
ReadSync.

Done.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:89: // We can initialize readeres synchrnously
here because only bytes can be
On 2012/10/15 19:54:45, Matt Menke wrote:
> nit:  readeres -> readers
> synchrnously -> synchronously

Oops, thanks.
Done.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:91: // |toal_size_| because we treat the size of
chunked data as zero.
On 2012/10/15 19:54:45, Matt Menke wrote:
> "because we treat the size of chunked data as zero" isn't quite right.  Maybe
> just remove that sentence, now that we've spelled out that it's zero in the
> header.  Alternatively, "We leave |total_size_| at zero, since for chunked
> uploads, we may not know the total size".

Done.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:204: int bytes_copied,
On 2012/10/15 19:54:45, Matt Menke wrote:
> I think this would be simpler if instead of passing around buf, buf_len, and
> bytes_copied, we just created a DrainableIOBuffer (what you call sub_buffer)
> before calling ReadInternal.  It already tracks buf_len (Its size() function)
> and bytes_copies (Its BytesConsumed() function).
Sounds good, done.
> 
> Also, I suggest making both the DrainableIOBuffer and the callback class
> variables - this allows us to DCHECK that there's no other ReadInternal call
> pending when Read and ReadSync are called, and is more consistent with other
> net/ code.  You can also get rid of the invoked_asynchronously argument in
that
> case (Set callback after the call to ReadInternal, only if it returns
> IO_PENDING).

I think we should make UploadDataStream as stateless as possible and limit the
lifetime of variables even if it results in increasing the number of arguments.
If we really want to check that there is no pending tasks, we can have
DCHECK(!weak_ptr_factory_.HasWeakPtrs()).

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

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.h:44: // data stream to |buf| and returns the number
of bytes read, otherwise,
On 2012/10/15 19:54:45, Matt Menke wrote:
> nit:  The comma before otherwise should be a semicolon.

Done.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.h:132: // total_size_ is set to zero when the data
is chunked.
On 2012/10/15 19:54:45, Matt Menke wrote:
> nit:  |total_size_|

I don't think pipes (|) are necessary here because it is obvious that
total_size_ is a variable's name.

There was a discussion about use of pipes in comments at chromium-dev, and they
said it's not necessary to add pipes when there is no ambiguity. 
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:89: // file_stream is NULL if the target
file is
On 2012/10/15 19:54:45, Matt Menke wrote:
> nit:  |file_stream|.

I don't think pipes are necessary here.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:125: file_stream_.release()),
On 2012/10/15 21:34:01, Matt Menke wrote:
> Are we leaking stuff here?  It looks like FileStreamWin, at least, only
> automatically closes its Handle when created with the constructor that only
> takes a NetLog as an argument, and FileStream does not use that constructor.

UploadFileElementReader is using the constructor that only takes a NetLog as an
argument.
Please see line 32. (new FileStream(NULL))

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:180: const CompletionCallback& callback)
{
On 2012/10/15 19:54:45, Matt Menke wrote:
> DCHECK(!callback.is_null());

Done.

[mmenke, 2012-10-16 19:13:41]

LGTM, just minor comments.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:204: int bytes_copied,
On 2012/10/16 11:52:20, hashimoto wrote:
> On 2012/10/15 19:54:45, Matt Menke wrote:
> > I think this would be simpler if instead of passing around buf, buf_len, and
> > bytes_copied, we just created a DrainableIOBuffer (what you call sub_buffer)
> > before calling ReadInternal.  It already tracks buf_len (Its size()
function)
> > and bytes_copies (Its BytesConsumed() function).
> Sounds good, done.
> > 
> > Also, I suggest making both the DrainableIOBuffer and the callback class
> > variables - this allows us to DCHECK that there's no other ReadInternal call
> > pending when Read and ReadSync are called, and is more consistent with other
> > net/ code.  You can also get rid of the invoked_asynchronously argument in
> that
> > case (Set callback after the call to ReadInternal, only if it returns
> > IO_PENDING).
> 
> I think we should make UploadDataStream as stateless as possible and limit the
> lifetime of variables even if it results in increasing the number of
arguments.
> If we really want to check that there is no pending tasks, we can have
> DCHECK(!weak_ptr_factory_.HasWeakPtrs()).

I don't have particularly strong feelings about this, just a general dislike of
passing lots of variables around from callback to callback when we don't have
to, and hiding state.

What's the reasoning for keeping it as "stateless" as possible?  Making the
state owned by a callback rather than the parent class doesn't make the class
have less state, it just obfuscates the state.

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

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.h:132: // total_size_ is set to zero when the data
is chunked.
On 2012/10/16 11:52:20, hashimoto wrote:
> On 2012/10/15 19:54:45, Matt Menke wrote:
> > nit:  |total_size_|
> 
> I don't think pipes (|) are necessary here because it is obvious that
> total_size_ is a variable's name.
> 
> There was a discussion about use of pipes in comments at chromium-dev, and
they
> said it's not necessary to add pipes when there is no ambiguity. 
>
https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium...

While I agree that it's not necessary per google style, it is used in the
description of the previous variable and in your other new comments (Though it's
not in "element_readers_" a bit further up, come to think of it).  Using a
consistent style *is* in the Google style guide.  I don't really care which we
use, but should try to be at least a little consistent within the upload code.

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:125: file_stream_.release()),
On 2012/10/16 11:52:20, hashimoto wrote:
> On 2012/10/15 21:34:01, Matt Menke wrote:
> > Are we leaking stuff here?  It looks like FileStreamWin, at least, only
> > automatically closes its Handle when created with the constructor that only
> > takes a NetLog as an argument, and FileStream does not use that constructor.
> 
> UploadFileElementReader is using the constructor that only takes a NetLog as
an
> argument.
> Please see line 32. (new FileStream(NULL))

Ahhh...right.  For some reason, I was thinking both FileStream constructors were
using the 3-argument FileStreamWin constructor.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream.cc:208: int previous_result) {
Random comment for the future:  Once chunked encoding works asynchronously,
suggest making both the sync and async paths use the same read function.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:61: MOCK_METHOD1(Init, int(const
CompletionCallback& callback));
nit:  Should have a comment "UploadElementReader implementation:"

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:64: bool IsInMemory() const OVERRIDE {
return is_in_memory_; }
These 3 should be virtual

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:78: void SetReadExpectation(int result)
{
Shouldn't this be SetAsyncReadExpectation?

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:365: // Read() with on-memory and
not-on-memory readers.
Async Read()?

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:420: // Read with a buffer whose size is
same as the data.
Suggest you put this above the previous test, since it's kinda a subset of it
(Though the previous test doesn't check quite as much)

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:423: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
It seems completely random to me which tests store an UploadDataStream in
scoped_ptrs, and which do not.  Am I missing something?  If not, suggest not
using scoped_ptrs.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:430: ASSERT_TRUE(stream.get());
Erm...How can this fail?  You created one in the second line in this function,
and never passed ownership, or passed the scoped_ptr to anything that had the
opportunity to destroy it.

Also, on basically impossible case it did fail, the previous calls to
stream->Init() and stream->IsInMemory() would have crashed.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:129: int
UploadFileElementReader::Init(const CompletionCallback& callback) {
For future consideration:  If we automatically called init on first read, rather
than having the parent call it directly, we'd know whether we were going to be
used synchronously or asynchronously.  Then we could open the file stream with
the necessary async flags in the async case, and use its own internal asyn
handling, which is nicer than the roll-your-own solution used here.

Alternatively, could just use where Init or InitSync was called to signal which
way we're going to be read from, and then construct the file with the
appropriate flag.

I really don't like passing stuff around between threads like this when we don't
have to.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:190: const bool posted =
base::WorkerPool::PostTaskAndReply(
optional:  There's a PostTaskAndReplyWithResult that would remove the need for
passing result around.  Also makes ReadInternal's interface a little less
unusual.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader_unittest.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
net/base/upload_file_element_reader_unittest.cc:21: bytes_.assign(kData, kData +
arraysize(kData) - 1);
Suggest a comment that some tests rely on this being even.

[hashimoto, 2012-10-17 08:04:30]

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:204: int bytes_copied,
On 2012/10/16 19:13:41, Matt Menke wrote:
> On 2012/10/16 11:52:20, hashimoto wrote:
> > On 2012/10/15 19:54:45, Matt Menke wrote:
> > > I think this would be simpler if instead of passing around buf, buf_len,
and
> > > bytes_copied, we just created a DrainableIOBuffer (what you call
sub_buffer)
> > > before calling ReadInternal.  It already tracks buf_len (Its size()
> function)
> > > and bytes_copies (Its BytesConsumed() function).
> > Sounds good, done.
> > > 
> > > Also, I suggest making both the DrainableIOBuffer and the callback class
> > > variables - this allows us to DCHECK that there's no other ReadInternal
call
> > > pending when Read and ReadSync are called, and is more consistent with
other
> > > net/ code.  You can also get rid of the invoked_asynchronously argument in
> > that
> > > case (Set callback after the call to ReadInternal, only if it returns
> > > IO_PENDING).
> > 
> > I think we should make UploadDataStream as stateless as possible and limit
the
> > lifetime of variables even if it results in increasing the number of
> arguments.
> > If we really want to check that there is no pending tasks, we can have
> > DCHECK(!weak_ptr_factory_.HasWeakPtrs()).
> 
> I don't have particularly strong feelings about this, just a general dislike
of
> passing lots of variables around from callback to callback when we don't have
> to, and hiding state.
> 
> What's the reasoning for keeping it as "stateless" as possible?  Making the
> state owned by a callback rather than the parent class doesn't make the class
> have less state, it just obfuscates the state.

IMHO the destination buffer and the callback are the states of a Read request,
not the UploadDataStream itself.
Since instance variables and method arguments are in different scopes and the
latter is narrower, I would like to avoid mixing the instance's states and the
Read request's by placing variables in the argument list when possible.

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

http://codereview.chromium.org/10910268/diff/74002/net/base/upload_data_strea...
net/base/upload_data_stream.h:132: // total_size_ is set to zero when the data
is chunked.
On 2012/10/16 19:13:41, Matt Menke wrote:
> On 2012/10/16 11:52:20, hashimoto wrote:
> > On 2012/10/15 19:54:45, Matt Menke wrote:
> > > nit:  |total_size_|
> > 
> > I don't think pipes (|) are necessary here because it is obvious that
> > total_size_ is a variable's name.
> > 
> > There was a discussion about use of pipes in comments at chromium-dev, and
> they
> > said it's not necessary to add pipes when there is no ambiguity. 
> >
>
https://groups.google.com/a/chromium.org/forum/?fromgroups=#%2521topic/chromi...
> 
> While I agree that it's not necessary per google style, it is used in the
> description of the previous variable and in your other new comments (Though
it's
> not in "element_readers_" a bit further up, come to think of it).  Using a
> consistent style *is* in the Google style guide.  I don't really care which we
> use, but should try to be at least a little consistent within the upload code.

OK, done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream.cc:208: int previous_result) {
On 2012/10/16 19:13:41, Matt Menke wrote:
> Random comment for the future:  Once chunked encoding works asynchronously,
> suggest making both the sync and async paths use the same read function.

Sounds great.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:61: MOCK_METHOD1(Init, int(const
CompletionCallback& callback));
On 2012/10/16 19:13:41, Matt Menke wrote:
> nit:  Should have a comment "UploadElementReader implementation:"

Done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:64: bool IsInMemory() const OVERRIDE {
return is_in_memory_; }
On 2012/10/16 19:13:41, Matt Menke wrote:
> These 3 should be virtual

Done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:78: void SetReadExpectation(int result)
{
On 2012/10/16 19:13:41, Matt Menke wrote:
> Shouldn't this be SetAsyncReadExpectation?

This method is used to set expectations for both sync and async read.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:365: // Read() with on-memory and
not-on-memory readers.
On 2012/10/16 19:13:41, Matt Menke wrote:
> Async Read()?

Done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:420: // Read with a buffer whose size is
same as the data.
On 2012/10/16 19:13:41, Matt Menke wrote:
> Suggest you put this above the previous test, since it's kinda a subset of it
> (Though the previous test doesn't check quite as much)

Done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:423: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
On 2012/10/16 19:13:41, Matt Menke wrote:
> It seems completely random to me which tests store an UploadDataStream in
> scoped_ptrs, and which do not.  Am I missing something?  If not, suggest not
> using scoped_ptrs.

Done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_data_strea...
net/base/upload_data_stream_unittest.cc:430: ASSERT_TRUE(stream.get());
On 2012/10/16 19:13:41, Matt Menke wrote:
> Erm...How can this fail?  You created one in the second line in this function,
> and never passed ownership, or passed the scoped_ptr to anything that had the
> opportunity to destroy it.
> 
> Also, on basically impossible case it did fail, the previous calls to
> stream->Init() and stream->IsInMemory() would have crashed.

Removed.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:129: int
UploadFileElementReader::Init(const CompletionCallback& callback) {
On 2012/10/16 19:13:41, Matt Menke wrote:
> For future consideration:  If we automatically called init on first read,
rather
> than having the parent call it directly, we'd know whether we were going to be
> used synchronously or asynchronously.  Then we could open the file stream with
> the necessary async flags in the async case, and use its own internal asyn
> handling, which is nicer than the roll-your-own solution used here.
> 
> Alternatively, could just use where Init or InitSync was called to signal
which
> way we're going to be read from, and then construct the file with the
> appropriate flag.
> 
> I really don't like passing stuff around between threads like this when we
don't
> have to.

I agree with you that ReadInternal() and InitInternal() look unusual and
unclean.
These functions are used to avoid code duplication between synchronous version
and asynchronous one.

Since the synchronous version methods are needed only for Chrome Frame, we
should consider fixing Chrome Frame rather than maintaining both
asynchronous/synchronous methods.

This class can be cleaned up if we fix Chrome Frame's UrlmonUploadDataStream to
support asynchronous initialization/reading and we get rid of InitSync() and
ReadSync() methods.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:190: const bool posted =
base::WorkerPool::PostTaskAndReply(
On 2012/10/16 19:13:41, Matt Menke wrote:
> optional:  There's a PostTaskAndReplyWithResult that would remove the need for
> passing result around.  Also makes ReadInternal's interface a little less
> unusual.

Good to know, thanks.
Done.

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader_unittest.cc (right):

http://codereview.chromium.org/10910268/diff/88003/net/base/upload_file_eleme...
net/base/upload_file_element_reader_unittest.cc:21: bytes_.assign(kData, kData +
arraysize(kData) - 1);
On 2012/10/16 19:13:41, Matt Menke wrote:
> Suggest a comment that some tests rely on this being even.

Done.

[commit-bot: I haz the power, 2012-10-17 08:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10910268/101001

[commit-bot: I haz the power, 2012-10-17 10:15:23]

Change committed as 162343