net: Make UploadDataStream::Init() asynchronous.

net/base/upload_data_stream.cc

Index: net/base/upload_data_stream.cc
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index e46f2e62718479c71dadccfd7727e191b35581aa..a040027b7567fd10221925c735b40c89ec557edf 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -24,7 +24,8 @@ UploadDataStream::UploadDataStream(UploadData* upload_data)
       element_index_(0),
       total_size_(0),
       current_position_(0),
-      initialized_successfully_(false) {
+      initialized_successfully_(false),
+      weak_ptr_factory_(this) {
   const std::vector<UploadElement>& elements = *upload_data_->elements();
   for (size_t i = 0; i < elements.size(); ++i)
     element_readers_.push_back(UploadElementReader::Create(elements[i]));
@@ -33,21 +34,29 @@ UploadDataStream::UploadDataStream(UploadData* upload_data)
 UploadDataStream::~UploadDataStream() {
 }
 
-int UploadDataStream::Init() {
+int UploadDataStream::Init(const CompletionCallback& callback) {
   DCHECK(!initialized_successfully_);
 
-  uint64 total_size = 0;
+  // Use fast path when initialization can be done synchronously.
+  if (IsInMemory())
+    return InitSync();
+
+  InitInternal(callback, 0, OK);
+  return ERR_IO_PENDING;
+}
+
+int UploadDataStream::InitSync() {
+  DCHECK(!initialized_successfully_);
+
+  // Initialize all readers synchronously.
   for (size_t i = 0; i < element_readers_.size(); ++i) {
     UploadElementReader* reader = element_readers_[i];
     const int result = reader->InitSync();
     if (result != OK)
       return result;
-    if (!is_chunked())
-      total_size += reader->GetContentLength();
   }
-  total_size_ = total_size;
 
-  initialized_successfully_ = true;
+  FinalizeInitialization();
   return OK;
 }
 
@@ -118,4 +127,48 @@ bool UploadDataStream::IsInMemory() const {
   return true;
 }
 
+void UploadDataStream::InitInternal(const CompletionCallback& callback,
+                                   int index,

[willchan no longer on Chromium, 2012/09/11 19:32:40]

indentation is off

[hashimoto, 2012/09/11 22:51:31]

Done.

+                                   int last_result) {

[willchan no longer on Chromium, 2012/09/11 19:32:40]

I suggest renaming:
s/index/start_index/ - hopefully it'll make it clearer it's the starting index.
I think the confusion between i and index may have caused the bug below.
s/last_result/previous_result/ - last isn't as clear as previous IMO, since last
may have another meaning.

[hashimoto, 2012/09/11 22:51:31]

Done.

+  DCHECK(!initialized_successfully_);
+

[willchan no longer on Chromium, 2012/09/11 19:32:40]

I recommend adding a DCHECK_NE(ERR_IO_PENDING, last_result).

[hashimoto, 2012/09/11 22:51:31]

Done.

+  // Check the last result.
+  if (last_result != OK) {

[willchan no longer on Chromium, 2012/09/11 19:32:40]

I wonder if we should do element_readers_.clear(); at all these places where we
invoke the callback with a non-OK error code.

[hashimoto, 2012/09/11 22:51:31]

Done.

+    callback.Run(last_result);
+    return;
+  }
+
+  // Call Init() for all elements.
+  for (size_t i = index; i < element_readers_.size(); ++i) {
+    UploadElementReader* reader = element_readers_[i];
+    const int new_result = reader->Init(
+        base::Bind(&UploadDataStream::InitInternal,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   callback,
+                   index + 1));

[willchan no longer on Chromium, 2012/09/11 19:32:40]

Is this a bug? Should it be i + 1? Can you write a unit test that verifies this
behavior is correct? My guess is using an UploadDataStream with a
MockUploadElementReader.

[hashimoto, 2012/09/11 22:51:31]

You are absolutely right.
Thank you for finding this.
Added UploadDataStreamTest.InitAsync/InitAsyncFailureAsync/InitAsyncFailureSync
to cover all three code path which runs |callback|

+    if (new_result != OK) {
+      if (new_result != ERR_IO_PENDING)
+        callback.Run(new_result);
+      return;
+    }
+  }
+
+  // Finalize initialization.
+  FinalizeInitialization();
+  callback.Run(OK);
+}
+
+void UploadDataStream::FinalizeInitialization() {
+  DCHECK(!initialized_successfully_);
+  if (!is_chunked()) {
+    uint64 total_size = 0;
+    for (size_t i = 0; i < element_readers_.size(); ++i) {
+      UploadElementReader* reader = element_readers_[i];
+      total_size += reader->GetContentLength();

[willchan no longer on Chromium, 2012/09/11 19:32:40]

Just double checking...could this block? I'm guessing not right now...but what
happens when we have a Google Drive one? How does the content length work for
that?

[hashimoto, 2012/09/11 22:51:31]

For Google Drive, I'm intended to fetch the content length during Init().
GetContentLength() is meant be a simple getter which returns the pre-fetched
value.

+    }
+    total_size_ = total_size;
+  }
+  initialized_successfully_ = true;
+}
+
 }  // namespace net