Files.app: Implement sending batch requests.

Files.app: Implement sending batch requests.

 * Wait for preparation of each child request.
 * Then generate multipart request that consists each child request.

BUG=451917
TEST=DriveApiRequestsTest.BatchUploadRequest

Committed: https://crrev.com/289a1e6dc3bde6c00496a3ce6d19e4258dcfd073
Cr-Commit-Position: refs/heads/master@{#325209}

[hirono, 2015-04-13 08:08:17]

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

[hirono, 2015-04-13 08:08:17]

PTAL, thanks!

The method of URLGenerator for batch uploading will be added by
crrev.com/1081033002.

[kinaba, 2015-04-13 08:25:28]

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
File google_apis/drive/drive_api_requests.cc (right):

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:27: "Host: www.googleapis.com\n"
How about getting the Host domain info from UrlGenerator, rather than hand
coding it here.

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:1001: for (const auto& pair :
child_requests_) {
Maybe: id_request_pair? for readability

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:1011: void
BatchUploadRequest::AddRequest(BatchableRequestBase* request) {
I'm not familiar with batched request, but is it ok that this AddRequest does
not record the order that child request is added (because it is pushing them to
a std::map)?

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:1040: if (child_requests_.empty()) {
What about having this case as (D)CHECK? Does the empty case make any sense?

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
File google_apis/drive/drive_api_requests.h (right):

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.h:1130: DriveApiUrlGenerator
url_generator() const { return url_generator_; }
const DriveApiUrlGenerator& ?

[hirono, 2015-04-13 09:16:10]

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
File google_apis/drive/drive_api_requests.cc (right):

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:27: "Host: www.googleapis.com\n"
On 2015/04/13 08:25:27, kinaba wrote:
> How about getting the Host domain info from UrlGenerator, rather than hand
> coding it here.

Done.

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:1001: for (const auto& pair :
child_requests_) {
On 2015/04/13 08:25:27, kinaba wrote:
> Maybe: id_request_pair? for readability

Removed this line for turning child_requests_ to vector.

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:1011: void
BatchUploadRequest::AddRequest(BatchableRequestBase* request) {
On 2015/04/13 08:25:27, kinaba wrote:
> I'm not familiar with batched request, but is it ok that this AddRequest does
> not record the order that child request is added (because it is pushing them
to
> a std::map)?

The order can affect the result. I turned child_requests_ to vector to preserve
the order.

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.cc:1040: if (child_requests_.empty()) {
On 2015/04/13 08:25:27, kinaba wrote:
> What about having this case as (D)CHECK? Does the empty case make any sense?

Turned it to CHECK, and updated the comment of function.

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
File google_apis/drive/drive_api_requests.h (right):

https://codereview.chromium.org/1084523002/diff/20001/google_apis/drive/drive...
google_apis/drive/drive_api_requests.h:1130: DriveApiUrlGenerator
url_generator() const { return url_generator_; }
On 2015/04/13 08:25:27, kinaba wrote:
> const DriveApiUrlGenerator& ?

Done.

[kinaba, 2015-04-14 02:33:59]

lgtm

[hirono, 2015-04-14 02:48:26]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-04-14 02:48:34]

On 2015/04/14 02:33:59, kinaba wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2015-04-14 02:48:48]

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

[commit-bot: I haz the power, 2015-04-14 03:18:18]

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

[commit-bot: I haz the power, 2015-04-14 03:18:18]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hirono, 2015-04-14 04:07:30]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-04-14 04:07:30]

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/1084523002/#ps80001
(title: "Fix complier error on mac")

[commit-bot: I haz the power, 2015-04-14 04:07:36]

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

[commit-bot: I haz the power, 2015-04-14 06:16:41]

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

[commit-bot: I haz the power, 2015-04-14 06:16:42]

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

[hirono, 2015-04-15 05:20:58]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-04-15 05:20:58]

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/1084523002/#ps100001
(title: "Fix memory leak in the test.")

[commit-bot: I haz the power, 2015-04-15 05:21:28]

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

[commit-bot: I haz the power, 2015-04-15 08:27:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-15 08:28:11]

Patchset 6 (id:??) landed as
https://crrev.com/289a1e6dc3bde6c00496a3ce6d19e4258dcfd073
Cr-Commit-Position: refs/heads/master@{#325209}

[Nico, 2015-04-15 14:24:08]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-04-15 14:24:08]

This new test seems to fail when doing an official build:
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/342

[Nico, 2015-04-15 14:24:36]

Here too:
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%20tester/build...

[Nico, 2015-04-15 14:25:51]

https://codereview.chromium.org/1084523002/diff/100001/google_apis/drive/driv...
File google_apis/drive/drive_api_requests_unittest.cc (right):

https://codereview.chromium.org/1084523002/diff/100001/google_apis/drive/driv...
google_apis/drive/drive_api_requests_unittest.cc:2000:
EXPECT_DEATH(request->Commit(), "Check failed");
Aha, that's because official builds compile out string arguments to CHECK. Is
this message important? It's probably better to use an empty string here, rather
than have an #ifdef OFFICIAL_BUILD and an empty string there and "Check failed"
in regular builds?

[Nico, 2015-04-15 15:05:45]

Fix: https://codereview.chromium.org/1052553004/