net: Move write flush responsiblity from URLFetcherResponseWriter to URLFetcherCore

net/url_request/url_fetcher_core.cc

Index: net/url_request/url_fetcher_core.cc
diff --git a/net/url_request/url_fetcher_core.cc b/net/url_request/url_fetcher_core.cc
index 24d4b46889f50c8db220bd7459b8ff1b76c17d13..c1a9b3f3eef3ce091925f158fbc4738c14c5858c 100644
--- a/net/url_request/url_fetcher_core.cc
+++ b/net/url_request/url_fetcher_core.cc
@@ -431,9 +431,7 @@ void URLFetcherCore::OnReadCompleted(URLRequest* request,
     InformDelegateDownloadProgress();
     InformDelegateDownloadDataIfNecessary(bytes_read);
 
-    const int result = response_writer_->Write(
-        buffer_, bytes_read,
-        base::Bind(&URLFetcherCore::DidWriteBuffer, this));
+    const int result = WriteBuffer(new DrainableIOBuffer(buffer_, bytes_read));
     if (result < 0) {
       // Write failed or waiting for write completion.
       if (result == ERR_IO_PENDING)
@@ -822,14 +820,35 @@ void URLFetcherCore::CompleteAddingUploadDataChunk(
                                 is_last_chunk);
 }
 
-void URLFetcherCore::DidWriteBuffer(int result) {
-  if (result < 0) {
-    delegate_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));
+int URLFetcherCore::WriteBuffer(scoped_refptr<DrainableIOBuffer> data) {
+  while (data->BytesRemaining() > 0) {
+    const int result = response_writer_->Write(
+        data, data->BytesRemaining(),
+        base::Bind(&URLFetcherCore::DidWriteBuffer, this, data));
+    if (result < 0)
+      return result;
+    data->DidConsume(result);
+  }
+  return OK;

[wtc, 2013/05/23 01:00:16]

An alternative is to return data->size() on success.

[hashimoto, 2013/05/23 04:33:24]

Returning data->size() seems appropriate when writing has finished
synchronously.
But when writing has been done asynchronously, I'm not sure about what we should
return. (i.e. When Write() is performed asynchronously, the last WriteBuffer()
call results in writing 0 bytes, just checking |data->BytesRemaining() > 0|. In
this case, I cannot not be sure which of |data->size()| or 0 should be
returned.)
So I thought returning OK is enough since we are not interested in the exact
number of bytes written here.

+}
+
+void URLFetcherCore::DidWriteBuffer(scoped_refptr<DrainableIOBuffer> data,
+                                    int result) {
+  if (result >= 0) {  // Continue writing.
+    data->DidConsume(result);
+    result = WriteBuffer(data);
+  }
+
+  if (result < 0) {  // Handle errors.
+    if (result != ERR_IO_PENDING) {
+      delegate_task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));

[wtc, 2013/05/23 01:00:16]

[I know this is pre-existing code.]

Hmm... this means the delegate only knows the fetch is
complete, but doesn't know whether the writing succeeded
or failed.

[hashimoto, 2013/05/23 04:33:24]

Correct, user code should call FileErrorOccured() manually to check the write
result.
Perhaps, we should remove FileErrorOccured(), and overwrite status_.error_ to
report writer errors instead.

+    }
     return;
   }
   // Finished writing buffer_. Read some more.
+  DCHECK_EQ(0, data->BytesRemaining());
   ReadResponse();

[wtc, 2013/05/23 01:00:16]

Nit: it may be better to write this function as follows.
Feel free to ignore this suggestion. The advantage is that
it doesn't allow the original |result| input argument to be
ERR_IO_PENDING.

  if (result >= 0) { // Continue writing.
    data->DidConsume(result);
    result = WriteBuffer(data);
    if (result == ERR_IO_PENDING)
      return;
  }

  if (result < 0) { // Handle errors.
    delegate_task_runner_->PostTask(...);
    return;
  }
  // Finished writing buffer_. Read some more.
  DCHECK_EQ(0, data->BytesRemaining());
  ReadResponse();

[hashimoto, 2013/05/23 04:33:24]

Looks easier to read.
Done.

 }