Get rid of RequestRegistry (part 1): remove the notion of suspended requests.

This is the first part of the series of RequestRegistry removal.
Now the class is doing nothing; it is just deleting Requests.
Everything can now be deleted, but to cleanly decouple the
dependency of other parts to it, I'll remove the class step by step.

What this patch does is:

* Remove unittest.
* Suspend => Finish.
* Resume => Start.

Previously "resume" searched existing suspended request and took over it,
but we don't need such complexity anymore.

BUG=164098

Review URL: https://chromiumcodereview.appspot.com/17066007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@206970 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 11 insertions, 298 deletions

diff --git a/chrome/browser/google_apis/base_requests.cc b/chrome/browser/google_apis/base_requests.cc
index 12d419e..0f3dfce 100644
--- a/chrome/browser/google_apis/base_requests.cc
+++ b/chrome/browser/google_apis/base_requests.cc
@@ -205,7 +205,7 @@ void UrlFetchRequestBase::Start(const std::string& access_token,
   }
 
   // Register to request registry.
-  NotifyStartToRequestRegistry();
+  NotifyStart();
 
   url_fetcher_->Start();
   started_ = true;
@@ -252,10 +252,7 @@ GDataErrorCode UrlFetchRequestBase::GetErrorCode(const URLFetcher* source) {
 }
 
 void UrlFetchRequestBase::OnProcessURLFetchResultsComplete(bool result) {
-  if (result)
-    NotifySuccessToRequestRegistry();
-  else
-    NotifyFinish(REQUEST_FAILED);
+  NotifyFinish(result ? REQUEST_COMPLETED : REQUEST_FAILED);
 }
 
 void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
@@ -279,14 +276,6 @@ void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
   ProcessURLFetchResults(source);
 }
 
-void UrlFetchRequestBase::NotifySuccessToRequestRegistry() {
-  NotifyFinish(REQUEST_COMPLETED);
-}
-
-void UrlFetchRequestBase::NotifyStartToRequestRegistry() {
-  NotifyStart();
-}
-
 void UrlFetchRequestBase::OnAuthFailed(GDataErrorCode code) {
   RunCallbackOnPrematureFailure(code);
 
@@ -449,10 +438,6 @@ void InitiateUploadRequestBase::ProcessURLFetchResults(
   OnProcessURLFetchResultsComplete(code == HTTP_SUCCESS);
 }
 
-void InitiateUploadRequestBase::NotifySuccessToRequestRegistry() {
-  NotifySuspend();
-}
-
 void InitiateUploadRequestBase::RunCallbackOnPrematureFailure(
     GDataErrorCode code) {
   callback_.Run(code, GURL());
@@ -498,7 +483,6 @@ UploadRangeRequestBase::UploadRangeRequestBase(
                           drive_file_path),
       drive_file_path_(drive_file_path),
       upload_url_(upload_url),
-      last_chunk_completed_(false),
       weak_ptr_factory_(this) {
 }
 
@@ -575,20 +559,9 @@ void UploadRangeRequestBase::OnDataParsed(GDataErrorCode code,
                                           scoped_ptr<base::Value> value) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // For a new file, HTTP_CREATED is returned.
-  // For an existing file, HTTP_SUCCESS is returned.
-  if (code == HTTP_CREATED || code == HTTP_SUCCESS)
-    last_chunk_completed_ = true;
-
   OnRangeRequestComplete(UploadRangeResponse(code, -1, -1), value.Pass());
-  OnProcessURLFetchResultsComplete(last_chunk_completed_);
-}
-
-void UploadRangeRequestBase::NotifySuccessToRequestRegistry() {
-  if (last_chunk_completed_)
-    NotifyFinish(REQUEST_COMPLETED);
-  else
-    NotifySuspend();
+  OnProcessURLFetchResultsComplete(
+      code == HTTP_CREATED || code == HTTP_SUCCESS);
 }
 
 void UploadRangeRequestBase::RunCallbackOnPrematureFailure(
@@ -667,10 +640,6 @@ bool ResumeUploadRequestBase::GetContentFile(
   return true;
 }
 
-void ResumeUploadRequestBase::NotifyStartToRequestRegistry() {
-  NotifyResume();
-}
-
 //======================== GetUploadStatusRequestBase ========================
 
 GetUploadStatusRequestBase::GetUploadStatusRequestBase(
diff --git a/chrome/browser/google_apis/base_requests.h b/chrome/browser/google_apis/base_requests.h
index 7a87760..f187b15 100644
--- a/chrome/browser/google_apis/base_requests.h
+++ b/chrome/browser/google_apis/base_requests.h
@@ -142,12 +142,6 @@ class UrlFetchRequestBase : public AuthenticatedRequestInterface,
   // authentication error. Must be implemented by a derived class.
   virtual void ProcessURLFetchResults(const net::URLFetcher* source) = 0;
 
-  // Invoked when it needs to notify the status. Chunked requests that
-  // constructs a logically single request from multiple physical requests
-  // should notify resume/suspend instead of start/finish.
-  virtual void NotifyStartToRequestRegistry();
-  virtual void NotifySuccessToRequestRegistry();
-
   // Invoked by this base class upon an authentication error or cancel by
   // a user request. Must be implemented by a derived class.
   virtual void RunCallbackOnPrematureFailure(GDataErrorCode code) = 0;
@@ -308,7 +302,6 @@ class InitiateUploadRequestBase : public UrlFetchRequestBase {
 
   // UrlFetchRequestBase overrides.
   virtual void ProcessURLFetchResults(const net::URLFetcher* source) OVERRIDE;
-  virtual void NotifySuccessToRequestRegistry() OVERRIDE;
   virtual void RunCallbackOnPrematureFailure(GDataErrorCode code) OVERRIDE;
   virtual std::vector<std::string> GetExtraRequestHeaders() const OVERRIDE;
 
@@ -362,7 +355,6 @@ class UploadRangeRequestBase : public UrlFetchRequestBase {
   virtual GURL GetURL() const OVERRIDE;
   virtual net::URLFetcher::RequestType GetRequestType() const OVERRIDE;
   virtual void ProcessURLFetchResults(const net::URLFetcher* source) OVERRIDE;
-  virtual void NotifySuccessToRequestRegistry() OVERRIDE;
   virtual void RunCallbackOnPrematureFailure(GDataErrorCode code) OVERRIDE;
 
   // This method will be called when the request is done, regardless of
@@ -391,8 +383,6 @@ class UploadRangeRequestBase : public UrlFetchRequestBase {
   const base::FilePath drive_file_path_;
   const GURL upload_url_;
 
-  bool last_chunk_completed_;
-
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<UploadRangeRequestBase> weak_ptr_factory_;
@@ -437,7 +427,6 @@ class ResumeUploadRequestBase : public UploadRangeRequestBase {
                               int64* range_offset,
                               int64* range_length,
                               std::string* upload_content_type) OVERRIDE;
-  virtual void NotifyStartToRequestRegistry() OVERRIDE;
 
  private:
   // The parameters for the request. See ResumeUploadParams for the details.
diff --git a/chrome/browser/google_apis/base_requests_unittest.cc b/chrome/browser/google_apis/base_requests_unittest.cc
index aaca989..a0a3f36 100644
--- a/chrome/browser/google_apis/base_requests_unittest.cc
+++ b/chrome/browser/google_apis/base_requests_unittest.cc
@@ -30,9 +30,7 @@ class FakeGetDataRequest : public GetDataRequest {
   virtual ~FakeGetDataRequest() {
   }
 
-  void NotifyStart() {
-    NotifyStartToRequestRegistry();
-  }
+  using RequestRegistry::Request::NotifyStart;
 
  protected:
   virtual GURL GetURL() const OVERRIDE {
@@ -64,9 +62,9 @@ class BaseRequestsTest : public testing::Test {
   virtual void SetUp() OVERRIDE {
     profile_.reset(new TestingProfile);
     runner_.reset(new RequestSender(profile_.get(),
-                                      NULL /* url_request_context_getter */,
-                                      std::vector<std::string>() /* scopes */,
-                                      std::string() /* custom user agent */));
+                                    NULL /* url_request_context_getter */,
+                                    std::vector<std::string>() /* scopes */,
+                                    std::string() /* custom user agent */));
     runner_->Initialize();
     LOG(ERROR) << "Initialized.";
   }
diff --git a/chrome/browser/google_apis/drive_uploader.cc b/chrome/browser/google_apis/drive_uploader.cc
index 5716798..26c3969 100644
--- a/chrome/browser/google_apis/drive_uploader.cc
+++ b/chrome/browser/google_apis/drive_uploader.cc
@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
-#include "base/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
@@ -17,8 +16,6 @@
 #include "chrome/browser/google_apis/gdata_wapi_parser.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/power_save_blocker.h"
-#include "net/base/file_stream.h"
-#include "net/base/net_errors.h"
 
 using content::BrowserThread;
 
@@ -289,17 +286,7 @@ void DriveUploader::OnUploadLocationReceived(
   }
 
   upload_file_info->upload_location = upload_location;
-
-  // Start the upload from the beginning of the file.
-  // PostTask is necessary because we have to finish
-  // InitiateUpload's callback before calling ResumeUpload, due to the
-  // implementation of RequestRegistry. (http://crbug.com/134814)
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&DriveUploader::UploadNextChunk,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Passed(&upload_file_info),
-                 0));  // Upload from the beginning of the file.
+  UploadNextChunk(upload_file_info.Pass(), 0);  // start_position
 }
 
 void DriveUploader::StartGetUploadStatus(
@@ -325,9 +312,6 @@ void DriveUploader::UploadNextChunk(
   DCHECK_GE(start_position, 0);
   DCHECK_LE(start_position, upload_file_info->content_length);
 
-  // TODO(kinaba) this will become not needed if the redundant workaround
-  // base::MessageLoop::current()->PostTask for calling UploadNextChunk is
-  // removed.
   if (upload_file_info->cancelled) {
     UploadFailed(upload_file_info.Pass(), GDATA_CANCELLED);
     return;
@@ -402,17 +386,7 @@ void DriveUploader::OnUploadRangeResponseReceived(
            << "-" << response.end_position_received
            << " for [" << upload_file_info->drive_path.value() << "]";
 
-  // Continue uploading the remaining chunk. The start position of the
-  // remaining data is |response.end_position_received|.
-  // PostTask is necessary because we have to finish previous ResumeUpload's
-  // callback before calling ResumeUpload again, due to the implementation of
-  // RequestRegistry. (http://crbug.com/134814)
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&DriveUploader::UploadNextChunk,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Passed(&upload_file_info),
-                 response.end_position_received));
+  UploadNextChunk(upload_file_info.Pass(), response.end_position_received);
 }
 
 void DriveUploader::OnUploadProgress(const ProgressCallback& callback,
diff --git a/chrome/browser/google_apis/request_registry.cc b/chrome/browser/google_apis/request_registry.cc
index f2f773c..e7b8f74 100644
--- a/chrome/browser/google_apis/request_registry.cc
+++ b/chrome/browser/google_apis/request_registry.cc
@@ -29,7 +29,6 @@ RequestRegistry::Request::Request(RequestRegistry* registry,
 
 RequestRegistry::Request::~Request() {
   DCHECK(progress_status_.transfer_state == REQUEST_COMPLETED ||
-         progress_status_.transfer_state == REQUEST_SUSPENDED ||
          progress_status_.transfer_state == REQUEST_FAILED);
 }
 
@@ -56,21 +55,6 @@ void RequestRegistry::Request::NotifyFinish(
   registry_->OnRequestFinish(progress_status().request_id);
 }
 
-void RequestRegistry::Request::NotifySuspend() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(progress_status_.transfer_state >= REQUEST_STARTED);
-  progress_status_.transfer_state = REQUEST_SUSPENDED;
-  registry_->OnRequestSuspend(progress_status().request_id);
-}
-
-void RequestRegistry::Request::NotifyResume() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (progress_status_.transfer_state == REQUEST_NOT_STARTED) {
-    progress_status_.transfer_state = REQUEST_IN_PROGRESS;
-    registry_->OnRequestResume(this, &progress_status_);
-  }
-}
-
 RequestRegistry::RequestRegistry() {
   in_flight_requests_.set_check_on_null_data(true);
 }
@@ -109,16 +93,7 @@ bool RequestRegistry::CancelForFilePath(const base::FilePath& file_path) {
 }
 
 void RequestRegistry::CancelRequest(Request* request) {
-  if (request->progress_status().transfer_state == REQUEST_SUSPENDED) {
-    // SUSPENDED request already completed its job (like calling back to
-    // its client code). Invoking request->Cancel() again on it is a kind of
-    // 'double deletion'. So here we directly call OnRequestFinish and just
-    // unregister the request from the registry.
-    // TODO(kinaba): http://crbug.com/164098 Get rid of the hack.
-    OnRequestFinish(request->progress_status().request_id);
-  } else {
-    request->Cancel();
-  }
+  request->Cancel();
 }
 
 void RequestRegistry::OnRequestStart(
@@ -140,54 +115,4 @@ void RequestRegistry::OnRequestFinish(RequestID id) {
   in_flight_requests_.Remove(id);
 }
 
-void RequestRegistry::OnRequestResume(
-    RequestRegistry::Request* request,
-    RequestProgressStatus* new_status) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Find the corresponding suspended task.
-  Request* suspended = NULL;
-  for (RequestIDMap::iterator iter(&in_flight_requests_);
-       !iter.IsAtEnd();
-       iter.Advance()) {
-    Request* in_flight_request = iter.GetCurrentValue();
-    const RequestProgressStatus& status =
-        in_flight_request->progress_status();
-    if (status.transfer_state == REQUEST_SUSPENDED &&
-        status.file_path == request->progress_status().file_path) {
-      suspended = in_flight_request;
-      break;
-    }
-  }
-
-  if (!suspended) {
-    // Preceding suspended requests was not found. Assume it was canceled.
-    //
-    // request->Cancel() needs to be called to properly shut down the
-    // current request, but request->Cancel() tries to unregister itself
-    // from the registry. So, as a hack, temporarily assign it an ID.
-    // TODO(kinaba): http://crbug.com/164098 Get rid of it.
-    new_status->request_id = in_flight_requests_.Add(request);
-    CancelRequest(request);
-    return;
-  }
-
-  // Remove the old one and initiate the new request.
-  const RequestProgressStatus& old_status = suspended->progress_status();
-  RequestID old_id = old_status.request_id;
-  in_flight_requests_.Remove(old_id);
-  new_status->request_id = in_flight_requests_.Add(request);
-  DVLOG(1) << "Request[" << old_id << " -> " <<
-           new_status->request_id << "] resumed.";
-}
-
-void RequestRegistry::OnRequestSuspend(RequestID id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  Request* request = in_flight_requests_.Lookup(id);
-  DCHECK(request);
-
-  DVLOG(1) << "Request[" << id << "] suspended.";
-}
-
 }  // namespace google_apis
diff --git a/chrome/browser/google_apis/request_registry.h b/chrome/browser/google_apis/request_registry.h
index ff1c678..571fbfc 100644
--- a/chrome/browser/google_apis/request_registry.h
+++ b/chrome/browser/google_apis/request_registry.h
@@ -22,7 +22,6 @@ enum RequestTransferState {
   REQUEST_IN_PROGRESS,
   REQUEST_COMPLETED,
   REQUEST_FAILED,
-  REQUEST_SUSPENDED,
 };
 
 // Returns string representations of the request state.
@@ -70,15 +69,6 @@ class RequestRegistry {
     // Notifies the registry about current status.
     void NotifyStart();
     void NotifyFinish(RequestTransferState status);
-    // Notifies suspend/resume, used for chunked upload requests.
-    // The initial upload request should issue "start" "progress"* "suspend".
-    // The subsequent requests will call "resume" "progress"* "suspend",
-    // and the last one will do "resume" "progress"* "finish".
-    // In other words, "suspend" is similar to "finish" except it lasts to live
-    // until the next "resume" comes. "Resume" is similar to "start", except
-    // that it removes the existing "suspend" request.
-    void NotifySuspend();
-    void NotifyResume();
 
    private:
     // Does the cancellation.
@@ -105,9 +95,6 @@ class RequestRegistry {
   // the request to the registry. A fresh request ID is returned to *id.
   void OnRequestStart(Request* request, RequestID* id);
   void OnRequestFinish(RequestID request_id);
-  void OnRequestSuspend(RequestID request_id);
-  void OnRequestResume(Request* request,
-                       RequestProgressStatus* new_status);
 
   typedef IDMap<Request, IDMapOwnPointer> RequestIDMap;
   RequestIDMap in_flight_requests_;
diff --git a/chrome/browser/google_apis/request_registry_unittest.cc b/chrome/browser/google_apis/request_registry_unittest.cc
deleted file mode 100644
index d2f7b34..0000000
--- a/chrome/browser/google_apis/request_registry_unittest.cc
+++ /dev/null
@@ -1,128 +0,0 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/google_apis/request_registry.h"
-
-#include "base/compiler_specific.h"
-#include "base/memory/weak_ptr.h"
-#include "chrome/browser/google_apis/request_registry.h"
-#include "content/public/test/test_browser_thread_bundle.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace google_apis {
-
-namespace {
-
-class MockRequest : public RequestRegistry::Request,
-                    public base::SupportsWeakPtr<MockRequest> {
- public:
-  explicit MockRequest(RequestRegistry* registry)
-      : RequestRegistry::Request(
-          registry,
-          base::FilePath(FILE_PATH_LITERAL("/dummy/download"))) {
-  }
-
-  MOCK_METHOD0(DoCancel, void());
-
-  // Make them public so that they can be called from test cases.
-  using RequestRegistry::Request::NotifyStart;
-  using RequestRegistry::Request::NotifyFinish;
-  using RequestRegistry::Request::NotifySuspend;
-  using RequestRegistry::Request::NotifyResume;
-};
-
-}  // namespace
-
-class RequestRegistryTest : public testing::Test {
- protected:
-  content::TestBrowserThreadBundle thread_bundle_;
-};
-
-TEST_F(RequestRegistryTest, OneSuccess) {
-  RequestRegistry registry;
-
-  base::WeakPtr<MockRequest> op1 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  EXPECT_CALL(*op1, DoCancel()).Times(0);
-
-  op1->NotifyStart();
-  op1->NotifyFinish(REQUEST_COMPLETED);
-  EXPECT_EQ(NULL, op1.get());  // deleted
-}
-
-TEST_F(RequestRegistryTest, OneCancel) {
-  RequestRegistry registry;
-
-  base::WeakPtr<MockRequest> op1 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  EXPECT_CALL(*op1, DoCancel());
-
-  op1->NotifyStart();
-  registry.CancelAll();
-  EXPECT_EQ(NULL, op1.get());  // deleted
-}
-
-TEST_F(RequestRegistryTest, TwoSuccess) {
-  RequestRegistry registry;
-
-  base::WeakPtr<MockRequest> op1 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  base::WeakPtr<MockRequest> op2 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  EXPECT_CALL(*op1, DoCancel()).Times(0);
-  EXPECT_CALL(*op2, DoCancel()).Times(0);
-
-  op1->NotifyStart();
-  op2->NotifyStart();
-  op1->NotifyFinish(REQUEST_COMPLETED);
-  op2->NotifyFinish(REQUEST_COMPLETED);
-  EXPECT_EQ(NULL, op1.get());  // deleted
-  EXPECT_EQ(NULL, op2.get());  // deleted
-}
-
-TEST_F(RequestRegistryTest, RestartRequest) {
-  RequestRegistry registry;
-
-  base::WeakPtr<MockRequest> op1 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  EXPECT_CALL(*op1, DoCancel()).Times(0);
-
-  op1->NotifyStart();
-  op1->NotifyStart();  // restart
-  op1->NotifyFinish(REQUEST_COMPLETED);
-  EXPECT_EQ(NULL, op1.get());  // deleted
-}
-
-TEST_F(RequestRegistryTest, SuspendCancel) {
-  RequestRegistry registry;
-
-  // Suspend-then-resume is a hack in RequestRegistry to tie physically
-  // split but logically single request (= chunked uploading split into
-  // multiple HTTP requests). When the "logically-single" request is
-  // canceled between the two physical requests,
-  //    |----op1----| CANCEL! |----op2----|
-  // the cancellation is notified to the callback function associated with
-  // op2, not op1. This is because, op1's callback is already invoked at this
-  // point to notify the completion of the physical request. Completion
-  // callback must not be called more than once.
-
-  base::WeakPtr<MockRequest> op1 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  EXPECT_CALL(*op1, DoCancel()).Times(0);
-
-  op1->NotifyStart();
-  op1->NotifySuspend();
-  registry.CancelAll();
-  EXPECT_EQ(NULL, op1.get());  // deleted
-
-  base::WeakPtr<MockRequest> op2 =
-      (new MockRequest(&registry))->AsWeakPtr();
-  EXPECT_CALL(*op2, DoCancel()).Times(1);
-
-  op2->NotifyResume();
-  EXPECT_EQ(NULL, op2.get());  // deleted
-}
-
-}  // namespace google_apis
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 4bf8ce8..063b719 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -863,7 +863,6 @@
         'browser/google_apis/fake_drive_service_unittest.cc',
         'browser/google_apis/mock_drive_service.cc',
         'browser/google_apis/mock_drive_service.h',
-        'browser/google_apis/request_registry_unittest.cc',
         'browser/google_apis/request_util_unittest.cc',
         'browser/google_apis/time_util_unittest.cc',
         'browser/history/android/android_cache_database_unittest.cc',