net: Move write flush responsiblity from URLFetcherResponseWriter to URLFetcherCore

net/url_request/url_fetcher_core.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_fetcher_core.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 413 matching lines @@
 
   bool waiting_on_write = false;
   do {
     if (!request_->status().is_success() || bytes_read <= 0)
       break;
 
     current_response_bytes_ += bytes_read;
     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)
         waiting_on_write = true;
       break;
     }
   } while (request_->Read(buffer_, kBufferSize, &bytes_read));
 
   const URLRequestStatus status = request_->status();
 
@@ skipping 368 matching lines @@
     return;
   }
   DCHECK(is_chunked_upload_);
   DCHECK(request_.get());
   DCHECK(!content.empty());
   request_->AppendChunkToUpload(content.data(),
                                 static_cast<int>(content.length()),
                                 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.

 }
 
 void URLFetcherCore::ReadResponse() {
   // Some servers may treat HEAD requests as GET requests.  To free up the
   // network connection as soon as possible, signal that the request has
   // completed immediately, without trying to read any data back (all we care
   // about is the response code and headers, which we already have).
   int bytes_read = 0;
   if (request_->status().is_success() &&
       (request_type_ != URLFetcher::HEAD))
@@ skipping 66 matching lines @@
 }
 
 void URLFetcherCore::InformDelegateDownloadDataInDelegateThread(
     scoped_ptr<std::string> download_data) {
   DCHECK(delegate_task_runner_->BelongsToCurrentThread());
   if (delegate_)
     delegate_->OnURLFetchDownloadData(fetcher_, download_data.Pass());
 }
 
 }  // namespace net