net: Make UploadDataStream::Read() asynchronous

net/base/upload_data_stream.h

Index: net/base/upload_data_stream.h
diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h
index 8679b2c9347a91bb945b3d5242234986e2f7144c..fdbb8fc57a7a3ca6fb33837028911afa77911e11 100644
--- a/net/base/upload_data_stream.h
+++ b/net/base/upload_data_stream.h
@@ -18,6 +18,7 @@ namespace net {
 class IOBuffer;
 class UploadElementReader;
 
+// A class to read all elements from an UploadData object.
 class NET_EXPORT UploadDataStream {
  public:
   explicit UploadDataStream(UploadData* upload_data);
@@ -36,23 +37,30 @@ class NET_EXPORT UploadDataStream {
   int Init(const CompletionCallback& callback);
 
   // Initializes the stream synchronously.
-  // Use this method only in tests and Chrome Frame.
+  // Use this method only if the thread is IO allowed or the data is in-memory.
   int InitSync();
 
-  // Reads up to |buf_len| bytes from the upload data stream to |buf|. The
-  // number of bytes read is returned. Partial reads are allowed.  Zero is
-  // returned on a call to Read when there are no remaining bytes in the
-  // stream, and IsEof() will return true hereafter.
+  // When possible, reads up to |buf_len| bytes synchronously from the upload
+  // data stream to |buf| and returns the number of bytes read, otherwise,

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

nit:  The comma before otherwise should be a semicolon.

[hashimoto, 2012/10/16 11:52:20]

Done.

+  // returns ERR_IO_PENDING and calls |callback| with the number of bytes read.
+  // Partial reads are allowed. Zero is returned on a call to Read when there
+  // are no remaining bytes in the stream, and IsEof() will return true
+  // hereafter.
   //
   // If there's less data to read than we initially observed (i.e. the actual
   // upload data is smaller than size()), zeros are padded to ensure that
   // size() bytes can be read, which can happen for TYPE_FILE payloads.
   //
-  // If the upload data stream is chunked (i.e. is_chunked() is true),
-  // ERR_IO_PENDING is returned to indicate there is nothing to read at the
-  // moment, but more data to come at a later time. If not chunked, reads
-  // won't fail.
-  int Read(IOBuffer* buf, int buf_len);
+  // If the data is chunked (i.e. is_chunked() is true) and there is not enough
+  // data ready to be uploaded, ERR_IO_PENDING is returned and the callback set
+  // by set_chunk_callback is called later when data gets available.
+  // When data is available, the data is read synchronously and the number of
+  // bytes read is returned.
+  int Read(IOBuffer* buf, int buf_len, const CompletionCallback& callback);
+
+  // Reads data always synchronously.
+  // Use this method only if the thread is IO allowed or the data is in-memory.
+  int ReadSync(IOBuffer* buf, int buf_len);
 
   // Sets the callback to be invoked when new chunks are available to upload.
   void set_chunk_callback(ChunkCallback* callback) {
@@ -62,7 +70,8 @@ class NET_EXPORT UploadDataStream {
   // Returns the total size of the data stream and the current position.
   // size() is not to be used to determine whether the stream has ended
   // because it is possible for the stream to end before its size is reached,
-  // for example, if the file is truncated.
+  // for example, if the file is truncated. When the data is chunked, size()
+  // always returns zero.
   uint64 size() const { return total_size_; }
   uint64 position() const { return current_position_; }
 
@@ -86,6 +95,7 @@ class NET_EXPORT UploadDataStream {
   FRIEND_TEST_ALL_PREFIXES(UploadDataStreamTest, InitAsync);
   FRIEND_TEST_ALL_PREFIXES(UploadDataStreamTest, InitAsyncFailureAsync);
   FRIEND_TEST_ALL_PREFIXES(UploadDataStreamTest, InitAsyncFailureSync);
+  FRIEND_TEST_ALL_PREFIXES(UploadDataStreamTest, ReadAsync);
 
   // Runs Init() for all element readers.
   // This method is used to implement Init().
@@ -97,6 +107,15 @@ class NET_EXPORT UploadDataStream {
   // This method is used to implement Init().
   void FinalizeInitialization();
 
+  // Reads data from the element readers.
+  // This method is used to implement Read().
+  int ReadInternal(scoped_refptr<IOBuffer> buf,
+                   int buf_len,
+                   int bytes_copied,
+                   bool invoked_asynchronously,
+                   const CompletionCallback& callback,
+                   int previous_result);
+
   // These methods are provided only to be used by unit tests.
   static void ResetMergeChunks();
   static void set_merge_chunks(bool merge) { merge_chunks_ = merge; }
@@ -110,6 +129,7 @@ class NET_EXPORT UploadDataStream {
   size_t element_index_;
 
   // Size and current read position within the upload data stream.
+  // total_size_ is set to zero when the data is chunked.

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

nit:  |total_size_|

[hashimoto, 2012/10/16 11:52:20]

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...

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

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.

[hashimoto, 2012/10/17 08:04:31]

OK, done.

   uint64 total_size_;
   uint64 current_position_;