Drive: Let DriveUploader use batch request API.

Drive: Let DriveUploader use batch request API.

The CL lets DriveUploader use batch request API. DriveUploader does asynchronous
preparation before calling API on service interface. To hold batch request
during the asynchronous preparation, the CL introduce refcounted hepler class,
which manages life time of batch request configurator.

BUG=451917
TEST=DriveUploaderTest

Committed: https://crrev.com/9cf453b74c8790f6d7a91f958ec998a9fa5a1fa0
Cr-Commit-Position: refs/heads/master@{#329599}

[hirono, 2015-05-08 05:27:22]

hirono@chromium.org changed reviewers:
+ kinaba@chromium.org

[hirono, 2015-05-08 05:27:22]

PTAL, thanks!

[kinaba, 2015-05-08 06:58:12]

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
File chrome/browser/drive/drive_uploader.cc (right):

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:48: // destroyed, it commits owned batch
request.
Please add some comment on why refcounted control is necessary.

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:179: void
DriveUploader::StartBatchProcessing() {
DCHECK(current_batch_request_ == null)?

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:284: drive_service_);
I can understand the preference to constness, but the current code
looks too much verbose to me. How about giving it up and writing in a
simple if-branch with comment, like:

DriveServiceBatchOperationsInterface* service = batch_request.get();
// If this is not a batched request, use the API service directly.
if (!service)
  service = drive_service_;

?

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:315: drive_service_);
ditto.

[hirono, 2015-05-11 05:22:06]

Thank you!

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
File chrome/browser/drive/drive_uploader.cc (right):

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:48: // destroyed, it commits owned batch
request.
On 2015/05/08 06:58:12, kinaba wrote:
> Please add some comment on why refcounted control is necessary.

Done.

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:179: void
DriveUploader::StartBatchProcessing() {
On 2015/05/08 06:58:12, kinaba wrote:
> DCHECK(current_batch_request_ == null)?

Done.

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:284: drive_service_);
On 2015/05/08 06:58:12, kinaba wrote:
> I can understand the preference to constness, but the current code
> looks too much verbose to me. How about giving it up and writing in a
> simple if-branch with comment, like:
> 
> DriveServiceBatchOperationsInterface* service = batch_request.get();
> // If this is not a batched request, use the API service directly.
> if (!service)
>   service = drive_service_;
> 
> ?

Done.

https://codereview.chromium.org/1134633003/diff/40001/chrome/browser/drive/dr...
chrome/browser/drive/drive_uploader.cc:315: drive_service_);
On 2015/05/08 06:58:12, kinaba wrote:
> ditto.

Done.

[kinaba, 2015-05-11 05:33:14]

lgtm with one request

https://codereview.chromium.org/1134633003/diff/100001/chrome/browser/drive/d...
File chrome/browser/drive/drive_uploader.cc (right):

https://codereview.chromium.org/1134633003/diff/100001/chrome/browser/drive/d...
chrome/browser/drive/drive_uploader.cc:316: service = batch_request->get();
Let's rename RefCountedBatchRequest::get to something else (like
getBatchOperationServiceInterface).

Having .get() and ->get() meaning different thing looks crazy.

[hirono, 2015-05-11 05:53:30]

Thanks!

https://codereview.chromium.org/1134633003/diff/100001/chrome/browser/drive/d...
File chrome/browser/drive/drive_uploader.cc (right):

https://codereview.chromium.org/1134633003/diff/100001/chrome/browser/drive/d...
chrome/browser/drive/drive_uploader.cc:316: service = batch_request->get();
On 2015/05/11 05:33:14, kinaba wrote:
> Let's rename RefCountedBatchRequest::get to something else (like
> getBatchOperationServiceInterface).
> 
> Having .get() and ->get() meaning different thing looks crazy.

Yes totally agree with you. Done.

[hirono, 2015-05-11 05:53:34]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-05-11 05:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/1134633003/#ps120001
(title: "Rename get() with configurator().")

[commit-bot: I haz the power, 2015-05-11 05:55:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134633003/120001

[commit-bot: I haz the power, 2015-05-11 07:14:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 07:14:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hirono, 2015-05-11 08:44:36]

hirono@chromium.org changed reviewers:
+ tzik@chromium.org

[hirono, 2015-05-11 08:44:37]

PTAL chrome/browser/sync_file_system/*
Thank you!

[hirono, 2015-05-11 08:46:55]

On 2015/05/11 08:44:37, hirono wrote:
> PTAL chrome/browser/sync_file_system/*
> Thank you!

Sorry I missed the reviewer name at #13.
I was asking @tzik.

[hirono, 2015-05-11 10:48:08]

Patchset #8 (id:140001) has been deleted

[hirono, 2015-05-13 03:28:09]

On 2015/05/11 08:46:55, hirono wrote:
> On 2015/05/11 08:44:37, hirono wrote:
> > PTAL chrome/browser/sync_file_system/*
> > Thank you!
> 
> Sorry I missed the reviewer name at #13.
> I was asking @tzik.

@tzik - PTAL chrome/browser/sync_file_system/* ? Thanks!

[tzik, 2015-05-13 04:44:45]

lgtm

[hirono, 2015-05-13 05:58:44]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-05-13 05:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/1134633003/#ps120001
(title: "Rename get() with configurator().")

[hirono, 2015-05-13 05:58:54]

On 2015/05/13 04:44:45, tzik wrote:
> lgtm

Thank you!

[commit-bot: I haz the power, 2015-05-13 05:59:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134633003/120001

[commit-bot: I haz the power, 2015-05-13 06:56:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-13 06:57:18]

Patchset 7 (id:??) landed as
https://crrev.com/9cf453b74c8790f6d7a91f958ec998a9fa5a1fa0
Cr-Commit-Position: refs/heads/master@{#329599}

[sergeyv, 2015-05-13 09:43:23]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1135353004/ by sergeyv@chromium.org.

The reason for reverting is: MigrationSingleClientTest.AllTypesIndividually is
broken by this change on Linux GN:

http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/28327

.