Get rid of RequestRegistry (part 6): get rid of RequestRegistry.

Now all the thing that the class does is to delete request objects.
The lifetime management duty is moved to RequestSender.
* NotifyStart => no op.
* NotifyFinish => sender_->RequestFinished.

BUG=164098

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

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

chrome/browser/google_apis/base_requests.cc

@@ -93,12 +93,11 @@ void ParseJson(const std::string& json, const ParseJsonCallback& callback) {
 //============================ UrlFetchRequestBase ===========================
 
 UrlFetchRequestBase::UrlFetchRequestBase(
-    RequestSender* runner,
+    RequestSender* sender,
     net::URLRequestContextGetter* url_request_context_getter)
-    : RequestRegistry::Request(runner->request_registry()),
-      url_request_context_getter_(url_request_context_getter),
+    : url_request_context_getter_(url_request_context_getter),
       re_authenticate_count_(0),
-      started_(false),
+      sender_(sender),
       save_temp_file_(false),
       weak_ptr_factory_(this) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -191,11 +190,7 @@ void UrlFetchRequestBase::Start(const std::string& access_token,
     }
   }
 
-  // Register to request registry.
-  NotifyStart();
-
   url_fetcher_->Start();
-  started_ = true;
 }
 
 URLFetcher::RequestType UrlFetchRequestBase::GetRequestType() const {
@@ -221,7 +216,7 @@ bool UrlFetchRequestBase::GetContentFile(base::FilePath* local_file_path,
 void UrlFetchRequestBase::Cancel() {
   url_fetcher_.reset(NULL);
   RunCallbackOnPrematureFailure(GDATA_CANCELLED);
-  NotifyFinish();
+  sender_->RequestFinished(this);
 }
 
 // static
@@ -240,7 +235,7 @@ GDataErrorCode UrlFetchRequestBase::GetErrorCode(const URLFetcher* source) {
 }
 
 void UrlFetchRequestBase::OnProcessURLFetchResultsComplete(bool result) {
-  NotifyFinish();
+  sender_->RequestFinished(this);
 }
 
 void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
@@ -266,16 +261,7 @@ void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
 
 void UrlFetchRequestBase::OnAuthFailed(GDataErrorCode code) {
   RunCallbackOnPrematureFailure(code);
-
-  // Check if this failed before we even started fetching. If so, register
-  // for start so we can properly unregister with finish.
-  if (!started_)
-    NotifyStart();
-
-  // Note: NotifyFinish() must be invoked at the end, after all other callbacks
-  // and notifications. Once NotifyFinish() is called, the current instance of
-  // request will be deleted from the RequestRegistry and become invalid.
-  NotifyFinish();
+  sender_->RequestFinished(this);
 }
 
 base::WeakPtr<AuthenticatedRequestInterface>

chrome/browser/google_apis/base_requests.h

@@ -15,7 +15,6 @@
 #include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/google_apis/gdata_errorcode.h"
-#include "chrome/browser/google_apis/request_registry.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
@@ -75,7 +74,7 @@ class AuthenticatedRequestInterface {
   // deleted when it is canceled by user action, for posting asynchronous tasks
   // on the authentication request object, weak pointers have to be used.
   // TODO(kinaba): crbug.com/134814 use more clean life time management than
-  // using weak pointers, while deprecating RequestRegistry.
+  // using weak pointers.
   virtual base::WeakPtr<AuthenticatedRequestInterface> GetWeakPtr() = 0;
 
   // Cancels the request. It will invoke the callback object passed in
@@ -87,7 +86,6 @@ class AuthenticatedRequestInterface {
 
 // Base class for requests that are fetching URLs.
 class UrlFetchRequestBase : public AuthenticatedRequestInterface,
-                            public RequestRegistry::Request,
                             public net::URLFetcherDelegate {
  public:
   // AuthenticatedRequestInterface overrides.
@@ -98,7 +96,7 @@ class UrlFetchRequestBase : public AuthenticatedRequestInterface,
   virtual void Cancel() OVERRIDE;
 
  protected:
-  UrlFetchRequestBase(RequestSender* runner,
+  UrlFetchRequestBase(RequestSender* sender,
                       net::URLRequestContextGetter* url_request_context_getter);
   virtual ~UrlFetchRequestBase();
 
@@ -170,7 +168,7 @@ class UrlFetchRequestBase : public AuthenticatedRequestInterface,
   ReAuthenticateCallback re_authenticate_callback_;
   int re_authenticate_count_;
   scoped_ptr<net::URLFetcher> url_fetcher_;
-  bool started_;
+  RequestSender* sender_;
 
   bool save_temp_file_;
   base::FilePath output_file_path_;

chrome/browser/google_apis/base_requests_unittest.cc

@@ -30,8 +30,6 @@ class FakeGetDataRequest : public GetDataRequest {
   virtual ~FakeGetDataRequest() {
   }
 
-  using RequestRegistry::Request::NotifyStart;
-
  protected:
   virtual GURL GetURL() const OVERRIDE {
     NOTREACHED();  // This method is not called in tests.
@@ -119,7 +117,6 @@ TEST_F(BaseRequestsTest, GetDataRequestParseValidResponse) {
           runner_.get(),
           base::Bind(&BaseRequestsTest::GetDataCallback,
                      base::Unretained(this)));
-  get_data_request->NotifyStart();
 
   get_data_request->ParseResponse(HTTP_SUCCESS, kValidJsonString);
   // Should wait for a blocking pool task, as the JSON parsing is done in the
@@ -137,7 +134,6 @@ TEST_F(BaseRequestsTest, GetDataRequestParseInvalidResponse) {
           runner_.get(),
           base::Bind(&BaseRequestsTest::GetDataCallback,
                      base::Unretained(this)));
-  get_data_request->NotifyStart();
 
   get_data_request->ParseResponse(HTTP_SUCCESS, kInvalidJsonString);
   // Should wait for a blocking pool task, as the JSON parsing is done in the

chrome/browser/google_apis/request_registry.cc

-// 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"
-
-namespace google_apis {
-
-RequestRegistry::Request::Request(RequestRegistry* registry)
-    : registry_(registry), id_(-1) {
-}
-
-RequestRegistry::Request::~Request() {
-}
-
-void RequestRegistry::Request::NotifyStart() {
-  registry_->OnRequestStart(this, &id_);
-}
-
-void RequestRegistry::Request::NotifyFinish() {
-  registry_->OnRequestFinish(id_);
-}
-
-RequestRegistry::RequestRegistry() {
-  in_flight_requests_.set_check_on_null_data(true);
-}
-
-RequestRegistry::~RequestRegistry() {
-}
-
-void RequestRegistry::OnRequestStart(
-    RequestRegistry::Request* request,
-    RequestID* id) {
-  *id = in_flight_requests_.Add(request);
-}
-
-void RequestRegistry::OnRequestFinish(RequestID id) {
-  if (in_flight_requests_.Lookup(id))
-    in_flight_requests_.Remove(id);
-}
-
-}  // namespace google_apis

chrome/browser/google_apis/request_registry.h

-// 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.
-
-#ifndef CHROME_BROWSER_GOOGLE_APIS_REQUEST_REGISTRY_H_
-#define CHROME_BROWSER_GOOGLE_APIS_REQUEST_REGISTRY_H_
-
-#include "base/basictypes.h"
-#include "base/id_map.h"
-
-namespace google_apis {
-
-// Unique ID to identify each request.
-typedef int32 RequestID;
-
-// This class tracks all the in-flight Google API requests and manage
-// their lifetime.
-class RequestRegistry {
- public:
-  RequestRegistry();
-  ~RequestRegistry();
-
-  // Base class for requests that this registry class can maintain.
-  // NotifyStart() passes the ownership of the Request object to the registry.
-  // In particular, calling NotifyFinish() causes the registry to delete the
-  // Request object itself.
-  class Request {
-   public:
-    explicit Request(RequestRegistry* registry);
-    virtual ~Request();
-
-   protected:
-    // Notifies the registry about current status.
-    void NotifyStart();
-    void NotifyFinish();
-
-   private:
-    RequestRegistry* const registry_;
-    RequestID id_;
-  };
-
- private:
-  // Handlers for notifications from Requests.
-  friend class Request;
-  // Notifies that an request has started. This method passes the ownership of
-  // the request to the registry. A fresh request ID is returned to *id.
-  void OnRequestStart(Request* request, RequestID* id);
-  void OnRequestFinish(RequestID request_id);
-
-  typedef IDMap<Request, IDMapOwnPointer> RequestIDMap;
-  RequestIDMap in_flight_requests_;
-
-  DISALLOW_COPY_AND_ASSIGN(RequestRegistry);
-};
-
-}  // namespace google_apis
-
-#endif  // CHROME_BROWSER_GOOGLE_APIS_REQUEST_REGISTRY_H_

chrome/browser/google_apis/request_sender.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/google_apis/request_sender.h"
 
 #include "base/bind.h"
+#include "base/stl_util.h"
 #include "chrome/browser/google_apis/auth_service.h"
 #include "chrome/browser/google_apis/base_requests.h"
 
@@ -17,7 +18,6 @@ RequestSender::RequestSender(
     const std::string& custom_user_agent)
     : profile_(profile),
       auth_service_(new AuthService(url_request_context_getter, scopes)),
-      request_registry_(new RequestRegistry()),
       custom_user_agent_(custom_user_agent),
       weak_ptr_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -25,6 +25,8 @@ RequestSender::RequestSender(
 
 RequestSender::~RequestSender() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  STLDeleteContainerPointers(in_flight_requests_.begin(),
+                             in_flight_requests_.end());
 }
 
 void RequestSender::Initialize() {
@@ -36,6 +38,8 @@ base::Closure RequestSender::StartRequestWithRetry(
     AuthenticatedRequestInterface* request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  in_flight_requests_.insert(request);
+
   // TODO(kinaba): Stop relying on weak pointers. Move lifetime management
   // of the requests to request sender.
   base::Closure cancel_closure =
@@ -96,4 +100,9 @@ void RequestSender::CancelRequest(
   request->Cancel();
 }
 
+void RequestSender::RequestFinished(AuthenticatedRequestInterface* request) {
+  in_flight_requests_.erase(request);
+  delete request;
+}
+
 }  // namespace google_apis

chrome/browser/google_apis/request_sender.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_GOOGLE_APIS_REQUEST_SENDER_H_
 #define CHROME_BROWSER_GOOGLE_APIS_REQUEST_SENDER_H_
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -25,7 +26,6 @@ namespace google_apis {
 
 class AuthenticatedRequestInterface;
 class AuthService;
-class RequestRegistry;
 
 // Helper class that sends requests implementing
 // AuthenticatedRequestInterface and handles retries and authentication.
@@ -45,21 +45,24 @@ class RequestSender {
   virtual ~RequestSender();
 
   AuthService* auth_service() { return auth_service_.get(); }
-  RequestRegistry* request_registry() {
-    return request_registry_.get();
-  }
 
   // Prepares the object for use.
   virtual void Initialize();
 
   // Starts a request implementing the AuthenticatedRequestInterface
   // interface, and makes the request retry upon authentication failures by
-  // calling back to RetryRequest.
+  // calling back to RetryRequest. The |request| object is owned by this
+  // RequestSender. It will be deleted in RequestSender's destructor or
+  // in RequestFinished().
   //
   // Returns a closure to cancel the request. The closure cancels the request
   // if it is in-flight, and does nothing if it is already terminated.
   base::Closure StartRequestWithRetry(AuthenticatedRequestInterface* request);
 
+  // Notifies to this RequestSender that |request| has finished.
+  // TODO(kinaba): refactor the life time management and make this at private.
+  void RequestFinished(AuthenticatedRequestInterface* request);
+
  private:
   // Called when the access token is fetched.
   void OnAccessTokenFetched(
@@ -79,7 +82,7 @@ class RequestSender {
   Profile* profile_;  // Not owned.
 
   scoped_ptr<AuthService> auth_service_;
-  scoped_ptr<RequestRegistry> request_registry_;
+  std::set<AuthenticatedRequestInterface*> in_flight_requests_;
   const std::string custom_user_agent_;
 
   base::ThreadChecker thread_checker_;

chrome/browser/sync_file_system/drive/api_util.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/memory/weak_ptr.h"
+#include "base/threading/non_thread_safe.h"
 #include "chrome/browser/drive/drive_service_interface.h"
 #include "chrome/browser/google_apis/drive_api_url_generator.h"
 #include "chrome/browser/google_apis/gdata_wapi_url_generator.h"

chrome/chrome_browser.gypi

@@ -658,8 +658,6 @@
         'browser/google_apis/gdata_wapi_parser.h',
         'browser/google_apis/gdata_wapi_url_generator.cc',
         'browser/google_apis/gdata_wapi_url_generator.h',
-        'browser/google_apis/request_registry.cc',
-        'browser/google_apis/request_registry.h',
         'browser/google_apis/request_sender.cc',
         'browser/google_apis/request_sender.h',
         'browser/google_apis/request_util.cc',