From 09b621eaa36477b9b157bbb12fc83865fc6da4bc Mon Sep 17 00:00:00 2001 From: hirono Date: Wed, 13 May 2015 03:19:24 -0700 Subject: [PATCH] Revert of Revert of Drive: Let DriveUploader use batch request API. (patchset #1 id:1 of https://codereview.chromium.org/1135353004/) Reason for revert: The failure is not caused by the patch. MigrationSingleClientTest.AllTypesIndividually looks flaky. Original issue's description: > Revert of Drive: Let DriveUploader use batch request API. (patchset #7 id:120001 of https://codereview.chromium.org/1134633003/) > > Reason for revert: > MigrationSingleClientTest.AllTypesIndividually is broken by this change on Linux GN: > > http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/28327 > > Original issue's description: > > 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} > > TBR=kinaba@chromium.org,tzik@chromium.org,hirono@chromium.org > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=451917 > > Committed: https://crrev.com/e244812ae9a09d001f24f9aaaf780c80f976d790 > Cr-Commit-Position: refs/heads/master@{#329610} TBR=kinaba@chromium.org,tzik@chromium.org,sergeyv@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=451917 Review URL: https://codereview.chromium.org/1138883004 Cr-Commit-Position: refs/heads/master@{#329613} --- chrome/browser/chromeos/drive/job_queue.cc | 26 +-- chrome/browser/chromeos/drive/job_scheduler.cc | 39 ++-- chrome/browser/drive/drive_uploader.cc | 56 ++++- chrome/browser/drive/drive_uploader.h | 29 ++- chrome/browser/drive/drive_uploader_unittest.cc | 242 ++++++++++++++++++--- .../drive_backend/drive_uploader_on_worker.cc | 5 + .../drive_backend/drive_uploader_on_worker.h | 4 + .../drive_backend/fake_drive_uploader.cc | 6 + .../drive_backend/fake_drive_uploader.h | 2 + google_apis/drive/drive_api_requests.cc | 9 +- google_apis/drive/drive_api_requests_unittest.cc | 9 +- 11 files changed, 353 insertions(+), 74 deletions(-) diff --git a/chrome/browser/chromeos/drive/job_queue.cc b/chrome/browser/chromeos/drive/job_queue.cc index e1ea3174020f..f6070b1dc91a 100644 --- a/chrome/browser/chromeos/drive/job_queue.cc +++ b/chrome/browser/chromeos/drive/job_queue.cc @@ -41,22 +41,18 @@ void JobQueue::PopForRun(int accepted_priority, std::vector* jobs) { return; // Looks up the queue in the order of priority upto |accepted_priority|. - bool processing_batch_request = false; - int64 total_size = 0; + uint64 total_size = 0; + bool batchable = true; for (int priority = 0; priority <= accepted_priority; ++priority) { - auto it = queue_[priority].begin(); - while (it != queue_[priority].end()) { - if (!processing_batch_request || - (it->batchable && total_size + it->size <= max_batch_size_)) { - total_size += it->size; - processing_batch_request = it->batchable; - jobs->push_back(it->id); - running_.insert(it->id); - it = queue_[priority].erase(it); - if (processing_batch_request) - continue; - } - return; + while (!queue_[priority].empty()) { + const auto& item = queue_[priority].front(); + total_size += item.size; + batchable = batchable && item.batchable && total_size <= max_batch_size_; + if (!(jobs->empty() || batchable)) + return; + jobs->push_back(item.id); + running_.insert(item.id); + queue_[priority].pop_front(); } } } diff --git a/chrome/browser/chromeos/drive/job_scheduler.cc b/chrome/browser/chromeos/drive/job_scheduler.cc index 2be9669db0f0..b3a4edcaf56d 100644 --- a/chrome/browser/chromeos/drive/job_scheduler.cc +++ b/chrome/browser/chromeos/drive/job_scheduler.cc @@ -4,6 +4,8 @@ #include "chrome/browser/chromeos/drive/job_scheduler.h" +#include + #include "base/files/file_util.h" #include "base/message_loop/message_loop.h" #include "base/metrics/histogram.h" @@ -742,7 +744,9 @@ void JobScheduler::QueueJob(JobID job_id) { const JobInfo& job_info = job_entry->job_info; const QueueType queue_type = GetJobQueueType(job_info.job_type); - queue_[queue_type]->Push(job_id, job_entry->context.type, false, + const bool batchable = job_info.job_type == TYPE_UPLOAD_EXISTING_FILE || + job_info.job_type == TYPE_UPLOAD_NEW_FILE; + queue_[queue_type]->Push(job_id, job_entry->context.type, batchable, job_info.num_total_bytes); // Temporary histogram for crbug.com/229650. @@ -802,25 +806,28 @@ void JobScheduler::DoJobLoop(QueueType queue_type) { if (job_ids.empty()) return; - // TODO(hirono): Currently all requests are not batchable. So the queue always - // return just 1 job. - DCHECK_EQ(1u, job_ids.size()); - JobEntry* entry = job_map_.Lookup(job_ids.front()); - DCHECK(entry); + if (job_ids.size() > 1) + uploader_->StartBatchProcessing(); - JobInfo* job_info = &entry->job_info; - job_info->state = STATE_RUNNING; - job_info->start_time = now; - NotifyJobUpdated(*job_info); + for (JobID job_id : job_ids) { + JobEntry* entry = job_map_.Lookup(job_id); + DCHECK(entry); - entry->cancel_callback = entry->task.Run(); + JobInfo* job_info = &entry->job_info; + job_info->state = STATE_RUNNING; + job_info->start_time = now; + NotifyJobUpdated(*job_info); - UpdateWait(); + entry->cancel_callback = entry->task.Run(); + logger_->Log(logging::LOG_INFO, "Job started: %s - %s", + job_info->ToString().c_str(), + GetQueueInfo(queue_type).c_str()); + } - logger_->Log(logging::LOG_INFO, - "Job started: %s - %s", - job_info->ToString().c_str(), - GetQueueInfo(queue_type).c_str()); + if (job_ids.size() > 1) + uploader_->StopBatchProcessing(); + + UpdateWait(); } int JobScheduler::GetCurrentAcceptedPriority(QueueType queue_type) { diff --git a/chrome/browser/drive/drive_uploader.cc b/chrome/browser/drive/drive_uploader.cc index 76709fdb6608..f15d9c27fba2 100644 --- a/chrome/browser/drive/drive_uploader.cc +++ b/chrome/browser/drive/drive_uploader.cc @@ -44,6 +44,30 @@ const int64 kUploadChunkSize = (1LL << 30); // 1GB const int64 kMaxMultipartUploadSize = (1LL << 20); // 1MB } // namespace +// Refcounted helper class to manage batch request. DriveUploader uses the class +// for keeping the BatchRequestConfigurator instance while it prepares upload +// file information asynchronously. DriveUploader discard the reference after +// getting file information and the instance will be destroyed after all +// preparations complete. At that time, the helper instance commits owned batch +// request at the destrutor. +class DriveUploader::RefCountedBatchRequest + : public base::RefCounted { + public: + RefCountedBatchRequest( + scoped_ptr configurator) + : configurator_(configurator.Pass()) {} + + // Gets pointer of BatchRequestConfiguratorInterface owned by the instance. + BatchRequestConfiguratorInterface* configurator() const { + return configurator_.get(); + } + + private: + friend class base::RefCounted; + ~RefCountedBatchRequest() { configurator_->Commit(); } + scoped_ptr configurator_; +}; + // Structure containing current upload information of file, passed between // DriveServiceInterface methods and callbacks. struct DriveUploader::UploadFileInfo { @@ -155,7 +179,17 @@ CancelCallback DriveUploader::UploadNewFile( local_file_path, content_type, callback, progress_callback)), base::Bind(&DriveUploader::CallUploadServiceAPINewFile, weak_ptr_factory_.GetWeakPtr(), parent_resource_id, title, - options)); + options, current_batch_request_)); +} + +void DriveUploader::StartBatchProcessing() { + DCHECK(current_batch_request_ == nullptr); + current_batch_request_ = + new RefCountedBatchRequest(drive_service_->StartBatchRequest().Pass()); +} + +void DriveUploader::StopBatchProcessing() { + current_batch_request_ = nullptr; } CancelCallback DriveUploader::UploadExistingFile( @@ -175,7 +209,8 @@ CancelCallback DriveUploader::UploadExistingFile( scoped_ptr(new UploadFileInfo( local_file_path, content_type, callback, progress_callback)), base::Bind(&DriveUploader::CallUploadServiceAPIExistingFile, - weak_ptr_factory_.GetWeakPtr(), resource_id, options)); + weak_ptr_factory_.GetWeakPtr(), resource_id, options, + current_batch_request_)); } CancelCallback DriveUploader::ResumeUploadFile( @@ -190,8 +225,7 @@ CancelCallback DriveUploader::ResumeUploadFile( DCHECK(!callback.is_null()); scoped_ptr upload_file_info(new UploadFileInfo( - local_file_path, content_type, - callback, progress_callback)); + local_file_path, content_type, callback, progress_callback)); upload_file_info->upload_location = upload_location; return StartUploadFile( @@ -243,12 +277,17 @@ void DriveUploader::CallUploadServiceAPINewFile( const std::string& parent_resource_id, const std::string& title, const UploadNewFileOptions& options, + const scoped_refptr& batch_request, scoped_ptr upload_file_info) { DCHECK(thread_checker_.CalledOnValidThread()); UploadFileInfo* const info_ptr = upload_file_info.get(); if (info_ptr->content_length <= kMaxMultipartUploadSize) { - info_ptr->cancel_callback = drive_service_->MultipartUploadNewFile( + DriveServiceBatchOperationsInterface* service = drive_service_; + // If this is a batched request, calls the API on the request instead. + if (batch_request.get()) + service = batch_request->configurator(); + info_ptr->cancel_callback = service->MultipartUploadNewFile( info_ptr->content_type, info_ptr->content_length, parent_resource_id, title, info_ptr->file_path, options, base::Bind(&DriveUploader::OnMultipartUploadComplete, @@ -267,12 +306,17 @@ void DriveUploader::CallUploadServiceAPINewFile( void DriveUploader::CallUploadServiceAPIExistingFile( const std::string& resource_id, const UploadExistingFileOptions& options, + const scoped_refptr& batch_request, scoped_ptr upload_file_info) { DCHECK(thread_checker_.CalledOnValidThread()); UploadFileInfo* const info_ptr = upload_file_info.get(); if (info_ptr->content_length <= kMaxMultipartUploadSize) { - info_ptr->cancel_callback = drive_service_->MultipartUploadExistingFile( + DriveServiceBatchOperationsInterface* service = drive_service_; + // If this is a batched request, calls the API on the request instead. + if (batch_request.get()) + service = batch_request->configurator(); + info_ptr->cancel_callback = service->MultipartUploadExistingFile( info_ptr->content_type, info_ptr->content_length, resource_id, info_ptr->file_path, options, base::Bind(&DriveUploader::OnMultipartUploadComplete, diff --git a/chrome/browser/drive/drive_uploader.h b/chrome/browser/drive/drive_uploader.h index f558713dbeee..6d511bf8138f 100644 --- a/chrome/browser/drive/drive_uploader.h +++ b/chrome/browser/drive/drive_uploader.h @@ -43,6 +43,16 @@ class DriveUploaderInterface { public: virtual ~DriveUploaderInterface() {} + // Starts batch processing for upload requests. All requests which upload + // small files (less than kMaxMultipartUploadSize) between + // |StartBatchProcessing| and |StopBatchProcessing| are sent as a single batch + // request. + virtual void StartBatchProcessing() = 0; + + // Stops batch processing. Must be called after calling |StartBatchProcessing| + // to commit requests. + virtual void StopBatchProcessing() = 0; + // Uploads a new file to a directory specified by |upload_location|. // Returns a callback for cancelling the uploading job. // @@ -114,6 +124,8 @@ class DriveUploader : public DriveUploaderInterface { ~DriveUploader() override; // DriveUploaderInterface overrides. + void StartBatchProcessing() override; + void StopBatchProcessing() override; google_apis::CancelCallback UploadNewFile( const std::string& parent_resource_id, const base::FilePath& local_file_path, @@ -137,6 +149,7 @@ class DriveUploader : public DriveUploaderInterface { const google_apis::ProgressCallback& progress_callback) override; private: + class RefCountedBatchRequest; struct UploadFileInfo; typedef base::Callback upload_file_info)> StartInitiateUploadCallback; @@ -153,18 +166,25 @@ class DriveUploader : public DriveUploaderInterface { // Checks file size and call InitiateUploadNewFile or MultipartUploadNewFile // API. Upon completion, OnUploadLocationReceived (for InitiateUploadNewFile) // or OnMultipartUploadComplete (for MultipartUploadNewFile) should be called. - void CallUploadServiceAPINewFile(const std::string& parent_resource_id, - const std::string& title, - const UploadNewFileOptions& options, - scoped_ptr upload_file_info); + // If |batch_request| is non-null, it calls the API function on the batch + // request. + void CallUploadServiceAPINewFile( + const std::string& parent_resource_id, + const std::string& title, + const UploadNewFileOptions& options, + const scoped_refptr& batch_request, + scoped_ptr upload_file_info); // Checks file size and call InitiateUploadExistingFile or // MultipartUploadExistingFile API. Upon completion, OnUploadLocationReceived // (for InitiateUploadExistingFile) or OnMultipartUploadComplete (for // MultipartUploadExistingFile) should be called. + // If |batch_request| is non-null, it calls the API function on the batch + // request. void CallUploadServiceAPIExistingFile( const std::string& resource_id, const UploadExistingFileOptions& options, + const scoped_refptr& batch_request, scoped_ptr upload_file_info); // DriveService callback for InitiateUpload. @@ -207,6 +227,7 @@ class DriveUploader : public DriveUploaderInterface { DriveServiceInterface* drive_service_; // Not owned by this class. scoped_refptr blocking_task_runner_; + scoped_refptr current_batch_request_; // Note: This should remain the last member so it'll be destroyed and // invalidate its weak pointers before any other members are destroyed. diff --git a/chrome/browser/drive/drive_uploader_unittest.cc b/chrome/browser/drive/drive_uploader_unittest.cc index 1cbffca3cf86..41b73b27ff10 100644 --- a/chrome/browser/drive/drive_uploader_unittest.cc +++ b/chrome/browser/drive/drive_uploader_unittest.cc @@ -4,6 +4,7 @@ #include "chrome/browser/drive/drive_uploader.h" +#include #include #include @@ -51,6 +52,30 @@ const char kTestUploadExistingFileURL[] = const int64 kUploadChunkSize = 1024 * 1024 * 1024; const char kTestETag[] = "test_etag"; +CancelCallback SendMultipartUploadResult( + DriveApiErrorCode response_code, + int64 content_length, + const google_apis::FileResourceCallback& callback, + const google_apis::ProgressCallback& progress_callback) { + // Callback progress + if (!progress_callback.is_null()) { + // For the testing purpose, it always notifies the progress at the end of + // whole file uploading. + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, + base::Bind(progress_callback, content_length, content_length)); + } + + // MultipartUploadXXXFile is an asynchronous function, so don't callback + // directly. + scoped_ptr entry; + entry.reset(new FileResource); + entry->set_md5_checksum(kTestDummyMd5); + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::Bind(callback, response_code, base::Passed(&entry))); + return CancelCallback(); +} + // Mock DriveService that verifies if the uploaded content matches the preset // expectation. class MockDriveServiceWithUploadExpectation : public DummyDriveService { @@ -216,6 +241,8 @@ class MockDriveServiceWithUploadExpectation : public DummyDriveService { EXPECT_EQ(kTestDocumentTitle, title); EXPECT_EQ(expected_upload_file_, local_file_path); + received_bytes_ = content_length; + multipart_upload_call_count_++; return SendMultipartUploadResult(HTTP_CREATED, content_length, callback, progress_callback); } @@ -241,35 +268,10 @@ class MockDriveServiceWithUploadExpectation : public DummyDriveService { return CancelCallback(); } - return SendMultipartUploadResult(HTTP_SUCCESS, content_length, callback, - progress_callback); - } - - CancelCallback SendMultipartUploadResult( - DriveApiErrorCode response_code, - int64 content_length, - const google_apis::FileResourceCallback& callback, - const google_apis::ProgressCallback& progress_callback) { received_bytes_ = content_length; multipart_upload_call_count_++; - - // Callback progress - if (!progress_callback.is_null()) { - // For the testing purpose, it always notifies the progress at the end of - // whole file uploading. - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, - base::Bind(progress_callback, content_length, content_length)); - } - - // MultipartUploadXXXFile is an asynchronous function, so don't callback - // directly. - scoped_ptr entry; - entry.reset(new FileResource); - entry->set_md5_checksum(kTestDummyMd5); - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::Bind(callback, response_code, base::Passed(&entry))); - return CancelCallback(); + return SendMultipartUploadResult(HTTP_SUCCESS, content_length, callback, + progress_callback); } const base::FilePath expected_upload_file_; @@ -764,4 +766,190 @@ TEST_F(DriveUploaderTest, ResumeUpload) { upload_progress_values[0]); } +class MockDriveServiceForBatchProcessing : public DummyDriveService { + public: + struct UploadFileInfo { + enum { NEW_FILE, EXISTING_FILE } type; + std::string content_type; + uint64 content_length; + std::string parent_resource_id; + std::string resource_id; + std::string title; + base::FilePath local_file_path; + google_apis::FileResourceCallback callback; + google_apis::ProgressCallback progress_callback; + }; + + class BatchRequestConfigurator : public BatchRequestConfiguratorInterface { + public: + explicit BatchRequestConfigurator( + MockDriveServiceForBatchProcessing* service) + : service(service) {} + + CancelCallback MultipartUploadNewFile( + const std::string& content_type, + int64 content_length, + const std::string& parent_resource_id, + const std::string& title, + const base::FilePath& local_file_path, + const UploadNewFileOptions& options, + const google_apis::FileResourceCallback& callback, + const google_apis::ProgressCallback& progress_callback) override { + UploadFileInfo info; + info.type = UploadFileInfo::NEW_FILE; + info.content_type = content_type; + info.content_length = content_length; + info.parent_resource_id = parent_resource_id; + info.title = title; + info.local_file_path = local_file_path; + info.callback = callback; + info.progress_callback = progress_callback; + service->files.push_back(info); + return CancelCallback(); + } + + CancelCallback MultipartUploadExistingFile( + const std::string& content_type, + int64 content_length, + const std::string& resource_id, + const base::FilePath& local_file_path, + const UploadExistingFileOptions& options, + const google_apis::FileResourceCallback& callback, + const google_apis::ProgressCallback& progress_callback) override { + UploadFileInfo info; + info.type = UploadFileInfo::EXISTING_FILE; + info.content_type = content_type; + info.content_length = content_length; + info.resource_id = resource_id; + info.local_file_path = local_file_path; + info.callback = callback; + info.progress_callback = progress_callback; + service->files.push_back(info); + return CancelCallback(); + } + + void Commit() override { + ASSERT_FALSE(service->committed); + service->committed = true; + for (const auto& file : service->files) { + SendMultipartUploadResult(HTTP_SUCCESS, file.content_length, + file.callback, file.progress_callback); + } + } + + private: + MockDriveServiceForBatchProcessing* service; + }; + + public: + scoped_ptr StartBatchRequest() override { + committed = false; + return scoped_ptr( + new BatchRequestConfigurator(this)); + } + + std::vector files; + bool committed; +}; + +TEST_F(DriveUploaderTest, BatchProcessing) { + // Preapre test file. + const size_t kTestFileSize = 1024 * 512; + base::FilePath local_path; + std::string data; + ASSERT_TRUE(test_util::CreateFileOfSpecifiedSize( + temp_dir_.path(), kTestFileSize, &local_path, &data)); + + // Prepare test target. + MockDriveServiceForBatchProcessing service; + DriveUploader uploader(&service, base::ThreadTaskRunnerHandle::Get().get()); + + struct { + DriveApiErrorCode error; + GURL resume_url; + scoped_ptr file; + UploadCompletionCallback callback() { + return test_util::CreateCopyResultCallback(&error, &resume_url, &file); + } + } results[2]; + + uploader.StartBatchProcessing(); + uploader.UploadNewFile("parent_resource_id", local_path, "title", + kTestMimeType, UploadNewFileOptions(), + results[0].callback(), + google_apis::ProgressCallback()); + uploader.UploadExistingFile( + "resource_id", local_path, kTestMimeType, UploadExistingFileOptions(), + results[1].callback(), google_apis::ProgressCallback()); + uploader.StopBatchProcessing(); + base::RunLoop().RunUntilIdle(); + + ASSERT_EQ(2u, service.files.size()); + EXPECT_TRUE(service.committed); + + EXPECT_EQ(MockDriveServiceForBatchProcessing::UploadFileInfo::NEW_FILE, + service.files[0].type); + EXPECT_EQ(kTestMimeType, service.files[0].content_type); + EXPECT_EQ(kTestFileSize, service.files[0].content_length); + EXPECT_EQ("parent_resource_id", service.files[0].parent_resource_id); + EXPECT_EQ("", service.files[0].resource_id); + EXPECT_EQ("title", service.files[0].title); + EXPECT_EQ(local_path.value(), service.files[0].local_file_path.value()); + + EXPECT_EQ(MockDriveServiceForBatchProcessing::UploadFileInfo::EXISTING_FILE, + service.files[1].type); + EXPECT_EQ(kTestMimeType, service.files[1].content_type); + EXPECT_EQ(kTestFileSize, service.files[1].content_length); + EXPECT_EQ("", service.files[1].parent_resource_id); + EXPECT_EQ("resource_id", service.files[1].resource_id); + EXPECT_EQ("", service.files[1].title); + EXPECT_EQ(local_path.value(), service.files[1].local_file_path.value()); + + EXPECT_EQ(HTTP_SUCCESS, results[0].error); + EXPECT_TRUE(results[0].resume_url.is_empty()); + EXPECT_TRUE(results[0].file); + + EXPECT_EQ(HTTP_SUCCESS, results[1].error); + EXPECT_TRUE(results[1].resume_url.is_empty()); + EXPECT_TRUE(results[1].file); +} + +TEST_F(DriveUploaderTest, BatchProcessingWithError) { + // Prepare test target. + MockDriveServiceForBatchProcessing service; + DriveUploader uploader(&service, base::ThreadTaskRunnerHandle::Get().get()); + + struct { + DriveApiErrorCode error; + GURL resume_url; + scoped_ptr file; + UploadCompletionCallback callback() { + return test_util::CreateCopyResultCallback(&error, &resume_url, &file); + } + } results[2]; + + uploader.StartBatchProcessing(); + uploader.UploadNewFile("parent_resource_id", + base::FilePath(FILE_PATH_LITERAL("/path/non_exists")), + "title", kTestMimeType, UploadNewFileOptions(), + results[0].callback(), + google_apis::ProgressCallback()); + uploader.UploadExistingFile( + "resource_id", base::FilePath(FILE_PATH_LITERAL("/path/non_exists")), + kTestMimeType, UploadExistingFileOptions(), results[1].callback(), + google_apis::ProgressCallback()); + uploader.StopBatchProcessing(); + base::RunLoop().RunUntilIdle(); + + EXPECT_EQ(0u, service.files.size()); + EXPECT_TRUE(service.committed); + + EXPECT_EQ(HTTP_NOT_FOUND, results[0].error); + EXPECT_TRUE(results[0].resume_url.is_empty()); + EXPECT_FALSE(results[0].file); + + EXPECT_EQ(HTTP_NOT_FOUND, results[1].error); + EXPECT_TRUE(results[1].resume_url.is_empty()); + EXPECT_FALSE(results[1].file); +} } // namespace drive diff --git a/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.cc b/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.cc index a56834dee965..26dfa1349727 100644 --- a/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.cc +++ b/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.cc @@ -28,6 +28,11 @@ DriveUploaderOnWorker::DriveUploaderOnWorker( DriveUploaderOnWorker::~DriveUploaderOnWorker() {} +void DriveUploaderOnWorker::StartBatchProcessing() { +} +void DriveUploaderOnWorker::StopBatchProcessing() { +} + google_apis::CancelCallback DriveUploaderOnWorker::UploadNewFile( const std::string& parent_resource_id, const base::FilePath& local_file_path, diff --git a/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.h b/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.h index cab38f33715d..caa87e8038c3 100644 --- a/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.h +++ b/chrome/browser/sync_file_system/drive_backend/drive_uploader_on_worker.h @@ -5,6 +5,8 @@ #ifndef CHROME_BROWSER_SYNC_FILE_SYSTEM_DRIVE_BACKEND_DRIVE_UPLOADER_ON_WORKER_H_ #define CHROME_BROWSER_SYNC_FILE_SYSTEM_DRIVE_BACKEND_DRIVE_UPLOADER_ON_WORKER_H_ +#include + #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" #include "base/sequence_checker.h" @@ -32,6 +34,8 @@ class DriveUploaderOnWorker : public drive::DriveUploaderInterface { base::SequencedTaskRunner* worker_task_runner); ~DriveUploaderOnWorker() override; + void StartBatchProcessing() override; + void StopBatchProcessing() override; google_apis::CancelCallback UploadNewFile( const std::string& parent_resource_id, const base::FilePath& local_file_path, diff --git a/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.cc b/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.cc index f0d30ad42ee1..98d6d3ed4639 100644 --- a/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.cc +++ b/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.cc @@ -87,6 +87,12 @@ FakeDriveUploader::FakeDriveUploader( FakeDriveUploader::~FakeDriveUploader() {} +void FakeDriveUploader::StartBatchProcessing() { +} + +void FakeDriveUploader::StopBatchProcessing() { +} + CancelCallback FakeDriveUploader::UploadNewFile( const std::string& parent_resource_id, const base::FilePath& local_file_path, diff --git a/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.h b/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.h index fa6bb8df8e15..b14ba2b1c350 100644 --- a/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.h +++ b/chrome/browser/sync_file_system/drive_backend/fake_drive_uploader.h @@ -50,6 +50,8 @@ class FakeDriveUploader : public drive::DriveUploaderInterface { ~FakeDriveUploader() override; // DriveUploaderInterface overrides. + void StartBatchProcessing() override; + void StopBatchProcessing() override; google_apis::CancelCallback UploadNewFile( const std::string& parent_resource_id, const base::FilePath& local_file_path, diff --git a/google_apis/drive/drive_api_requests.cc b/google_apis/drive/drive_api_requests.cc index dec5adc2208b..ce07586ab3c1 100644 --- a/google_apis/drive/drive_api_requests.cc +++ b/google_apis/drive/drive_api_requests.cc @@ -1211,9 +1211,12 @@ void BatchUploadRequest::OnChildRequestPrepared(RequestID request_id, void BatchUploadRequest::Commit() { DCHECK(CalledOnValidThread()); DCHECK(!committed_); - CHECK(!child_requests_.empty()); - committed_ = true; - MayCompletePrepare(); + if (child_requests_.empty()) { + Cancel(); + } else { + committed_ = true; + MayCompletePrepare(); + } } void BatchUploadRequest::Prepare(const PrepareCallback& callback) { diff --git a/google_apis/drive/drive_api_requests_unittest.cc b/google_apis/drive/drive_api_requests_unittest.cc index f529bd551512..9f3fab9e2998 100644 --- a/google_apis/drive/drive_api_requests_unittest.cc +++ b/google_apis/drive/drive_api_requests_unittest.cc @@ -2066,9 +2066,12 @@ TEST_F(DriveApiRequestsTest, BatchUploadRequest) { } TEST_F(DriveApiRequestsTest, EmptyBatchUploadRequest) { - scoped_ptr request(new drive::BatchUploadRequest( - request_sender_.get(), *url_generator_)); - EXPECT_DEATH(request->Commit(), ""); + drive::BatchUploadRequest* const request = + new drive::BatchUploadRequest(request_sender_.get(), *url_generator_); + base::WeakPtr weak_ptr = + request->GetWeakPtrAsBatchUploadRequest(); + request->Commit(); + ASSERT_FALSE(weak_ptr.get()); } TEST_F(DriveApiRequestsTest, BatchUploadRequestWithBodyIncludingZero) { -- 2.11.4.GIT