google_apis: Implement google_apis::ResponseWriter

google_apis: Implement google_apis::ResponseWriter

Remove URLFetcherDelegate::OnURLFetchDownloadData.
Stop resetting response_writer_ in URLFetcherCore::CancelURLRequest() to guarantee that the writer object is always alive if URLFetcher is alive.
To close the file, call URLFetcherResponseWriter::Finish when URLFetcherCore::URLFetcherCore::WriteBuffer fails.
google_apis::ResponseWriter is responsible to call GetContentCallback.

BUG=126753
TEST=unit_tests

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

[hashimoto, 2013-10-16 06:38:05]

Please review.
This change depends on https://codereview.chromium.org/26230003/ ("net: Allow
URLFetcher users to inject custom URLFetcherResponseWriter")

[kinaba, 2013-10-16 08:38:04]

https://codereview.chromium.org/26784003/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/base_requests.cc (right):

https://codereview.chromium.org/26784003/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/base_requests.cc:195:
GetOutputFilePath(&output_file_path, &get_content_callback);
Either the return value of GetOutputFilePath should be checked, or the return
type can be changed to void.

https://codereview.chromium.org/26784003/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/base_requests.cc:199:
CreateRelayCallback(get_content_callback));
get_content_callback can be null (according to DownloadFileRequestBase's
constructor; even in the case when output_file_path is valid), but as far as I
quickly searched, CreateComposedCallback (hence CreateRelayCallback) has DCHECK
on !callback.is_null.

[hashimoto, 2013-10-17 03:33:14]

https://codereview.chromium.org/26784003/diff/4001/chrome/browser/google_apis...
File chrome/browser/google_apis/base_requests.cc (right):

https://codereview.chromium.org/26784003/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/base_requests.cc:195:
GetOutputFilePath(&output_file_path, &get_content_callback);
On 2013/10/16 08:38:04, kinaba wrote:
> Either the return value of GetOutputFilePath should be checked, or the return
> type can be changed to void.

Changed the return type to void.

https://codereview.chromium.org/26784003/diff/4001/chrome/browser/google_apis...
chrome/browser/google_apis/base_requests.cc:199:
CreateRelayCallback(get_content_callback));
On 2013/10/16 08:38:04, kinaba wrote:
> get_content_callback can be null (according to DownloadFileRequestBase's
> constructor; even in the case when output_file_path is valid), but as far as I
> quickly searched, CreateComposedCallback (hence CreateRelayCallback) has
DCHECK
> on !callback.is_null.

Good catch, my carelessness.
This should have been caught by running
DriveApiRequestsTest.DownloadFileRequest.

[kinaba, 2013-10-18 07:28:45]

lgtm

[hashimoto, 2013-10-18 10:40:00]

wtc@,
Could you take a look at this change as a net/ owner?

[hashimoto, 2013-10-23 06:43:14]

On 2013/10/18 10:40:00, hashimoto wrote:
> wtc@,
> Could you take a look at this change as a net/ owner?

ping?

[wtc, 2013-10-23 21:53:14]

Patch set 3 LGTM.

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
File chrome/browser/google_apis/base_requests.cc (right):

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.cc:137: make_scoped_ptr(new
std::string(buffer->data(), num_bytes)));

Nit: it is too bad that we have to copy buffer->data() to a std::string here.

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
File chrome/browser/google_apis/base_requests.h (right):

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:39: // Callback type for getting the
content from DownloadFileRequest.

Nit: Callback type for getting => Callback used to get ?

to match the style of the comments above.

(I know this is an old comment.)

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:92: // Saves the response for the
request to file or string.

Nit: add "a" befoe "file".

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:167: // Used by a derived class to
set an output file path if they want to save

The method's name is "GetOutputFilePath". Do you know why this comment says "to
set an output file path"?

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:170: GetContentCallback*
get_content_callback);

Nit: we should document the get_content_callback output parameter.

https://codereview.chromium.org/26784003/diff/24001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (left):

https://codereview.chromium.org/26784003/diff/24001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:653: response_writer_.reset();

Can we call
  response_writer_->Finish(base::Bind(&EmptyCompletionCallback));
  response_writer_.reset();
here?

Would that work?

[hashimoto, 2013-10-24 07:23:37]

Thanks for your kind review.

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
File chrome/browser/google_apis/base_requests.cc (right):

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.cc:137: make_scoped_ptr(new
std::string(buffer->data(), num_bytes)));
On 2013/10/23 21:53:14, wtc wrote:
> 
> Nit: it is too bad that we have to copy buffer->data() to a std::string here.

It'd be nice if we could pass |buffer| to the callback without copying the data,
but it's not feasible because |get_content_callback| will post a task from IO
thread to UI thread. URLFetcher may alter the content of |buffer| before the
task gets executed on UI thread.

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
File chrome/browser/google_apis/base_requests.h (right):

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:39: // Callback type for getting the
content from DownloadFileRequest.
On 2013/10/23 21:53:14, wtc wrote:
> 
> Nit: Callback type for getting => Callback used to get ?
> 
> to match the style of the comments above.
> 
> (I know this is an old comment.)

Done.

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:92: // Saves the response for the
request to file or string.
On 2013/10/23 21:53:14, wtc wrote:
> 
> Nit: add "a" befoe "file".

Done.

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:167: // Used by a derived class to
set an output file path if they want to save
On 2013/10/23 21:53:14, wtc wrote:
> 
> The method's name is "GetOutputFilePath". Do you know why this comment says
"to
> set an output file path"?

UrlFetchRequestBase::Start() calls this method to get a file path and the file
path is passed to URLFetcher.
From URLFetcherRequestBase's point of view, this method is a getter.
From the derived class's point of view, they can pass the output file path to
URLFetcher by overriding this method. (i.e. works as a kind of setter.)

https://codereview.chromium.org/26784003/diff/24001/chrome/browser/google_api...
chrome/browser/google_apis/base_requests.h:170: GetContentCallback*
get_content_callback);
On 2013/10/23 21:53:14, wtc wrote:
> 
> Nit: we should document the get_content_callback output parameter.

Done.

https://codereview.chromium.org/26784003/diff/24001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (left):

https://codereview.chromium.org/26784003/diff/24001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:653: response_writer_.reset();
On 2013/10/23 21:53:14, wtc wrote:
> 
> Can we call
>   response_writer_->Finish(base::Bind(&EmptyCompletionCallback));
>   response_writer_.reset();
> here?
> 
> Would that work?

I'd like to stop resetting |response_writer_| in URLFetcher.
This way, there can be a contract between URLFetcher and its users that the
URLFetcherResponseWriter passed to SaveResponseWithWriter() is alive as long as
the URLFetcher is alive.
This contract makes the code in UrlFetchRequestBase easy because it does not
have to care about a case where UrlFetchRequestBase::response_writer_ becomes a
dangling pointer.

[wtc, 2013-10-24 18:36:39]

Patch set 4 LGTM.

https://codereview.chromium.org/26784003/diff/155001/chrome/browser/google_ap...
File chrome/browser/google_apis/base_requests.h (right):

https://codereview.chromium.org/26784003/diff/155001/chrome/browser/google_ap...
chrome/browser/google_apis/base_requests.h:169: // get_content_callback is
called when some part of the response is read.

Nit: Please stress the fact that get_content_callback is an output argument.
Maybe somethign like:
  Sets |get_content_callback|, which is called when some part of the response is
read.

[hashimoto, 2013-10-25 05:29:14]

https://codereview.chromium.org/26784003/diff/155001/chrome/browser/google_ap...
File chrome/browser/google_apis/base_requests.h (right):

https://codereview.chromium.org/26784003/diff/155001/chrome/browser/google_ap...
chrome/browser/google_apis/base_requests.h:169: // get_content_callback is
called when some part of the response is read.
On 2013/10/24 18:36:39, wtc wrote:
> 
> Nit: Please stress the fact that get_content_callback is an output argument.
> Maybe somethign like:
>   Sets |get_content_callback|, which is called when some part of the response
is
> read.

Done.

[commit-bot: I haz the power, 2013-10-25 05:29:27]

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

[commit-bot: I haz the power, 2013-10-25 08:53:50]

Change committed as 230992