net: Move write flush responsiblity from URLFetcherResponseWriter to URLFetcherCore

net: Move write flush responsiblity from URLFetcherResponseWriter to URLFetcherCore

URLFetcherCore is now responsible to write all data stored in |buffer_|.

BUG=126753
TEST=net_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202003

[hashimoto, 2013-05-22 09:03:08]

Please review.

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

Patch set 1 LGTM. Thanks!

Optional: you may ask a coworker in your office to take a
second look.

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:832: return OK;

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

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:846:
base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));

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

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:852: ReadResponse();

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();

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.h:197: // Writes all bytes stored in |data|
with |response_writer_|.

Please document that it returns OK (as opposed to the number
of bytes written) on success (all bytes in |data| have been
written).

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_response_writer.cc:98: DCHECK_LT(result, 0);

Does this DCHECK mean we expect to get either ERR_IO_PENDING
or a real error?

The comments in net/base/file_stream.h say FileStream::Write
may succeed synchronously and return the number of bytes
written.

It seems that this function is still correct even when
|result| is > 0.

[hashimoto, 2013-05-23 04:33:23]

kinaba@ could you take a look?

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:832: return OK;
On 2013/05/23 01:00:16, wtc wrote:
> 
> An alternative is to return data->size() on success.

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.

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:846:
base::Bind(&URLFetcherCore::InformDelegateFetchIsComplete, this));
On 2013/05/23 01:00:16, wtc wrote:
> 
> [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.

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.

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:852: ReadResponse();
On 2013/05/23 01:00:16, wtc wrote:
> 
> 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();

Looks easier to read.
Done.

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_core.h:197: // Writes all bytes stored in |data|
with |response_writer_|.
On 2013/05/23 01:00:16, wtc wrote:
> 
> Please document that it returns OK (as opposed to the number
> of bytes written) on success (all bytes in |data| have been
> written).

Done.

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/15711003/diff/3001/net/url_request/url_fetche...
net/url_request/url_fetcher_response_writer.cc:98: DCHECK_LT(result, 0);
On 2013/05/23 01:00:16, wtc wrote:
> 
> Does this DCHECK mean we expect to get either ERR_IO_PENDING
> or a real error?
> 
> The comments in net/base/file_stream.h say FileStream::Write
> may succeed synchronously and return the number of bytes
> written.
> 
> It seems that this function is still correct even when
> |result| is > 0.

I thought DCHECK_LT(result, 0) was a good idea because we provide
PLATFORM_FILE_ASYNC for FileStream and both file_stream_context_win.cc and
file_stream_context_posix.cc always return negative values on
FileStream::Context::WriteAsync().
But this DCHECK turns out to be quite inappropriate because, as you said,
|result >= 0| can be handled with the current code with no cost, and we should
have error handling code for result<0 cases.
Fixed.

[kinaba, 2013-05-23 04:53:13]

lgtm for patch set 2

[wtc, 2013-05-23 17:22:53]

Patch set 2 LGTM. Thanks!

https://codereview.chromium.org/15711003/diff/15001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/15711003/diff/15001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.h:198: // Returns OK when all bytes in |data|
gets written successfully.

Nit: when all bytes in |data| gets written successfully =>
       if all bytes in |data| get written synchronously

I think it's also helpful to add:

  Otherwise, returns ERR_IO_PENDING or a network error code.

[hashimoto, 2013-05-24 02:29:28]

https://codereview.chromium.org/15711003/diff/15001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.h (right):

https://codereview.chromium.org/15711003/diff/15001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.h:198: // Returns OK when all bytes in |data|
gets written successfully.
On 2013/05/23 17:22:53, wtc wrote:
> 
> Nit: when all bytes in |data| gets written successfully =>
>        if all bytes in |data| get written synchronously
> 
> I think it's also helpful to add:
> 
>   Otherwise, returns ERR_IO_PENDING or a network error code.

Thanks, done.

[commit-bot: I haz the power, 2013-05-24 02:29:42]

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

[commit-bot: I haz the power, 2013-05-24 07:15:21]

Change committed as 202003