Move WebDataRequest management code into a separate class

(which can be reused when WebDataService is broken up).

TBR=ben@chromium.org (for gyp changes)
TEST=No visible change
BUG=166488



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

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

chrome/browser/api/webdata/web_data_results.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/api/webdata/web_data_results.h"
+
+// TODO(caitkp): This should not live in chrome/browser/api.
+
+WDTypedResult::WDTypedResult(WDResultType type)
+    : type_(type),
+    callback_(DestroyCallback()) {
+}
+
+WDTypedResult::WDTypedResult(WDResultType type, const DestroyCallback& callback)
+    : type_(type),
+    callback_(callback) {
+}
+
+WDTypedResult::~WDTypedResult() {}

chrome/browser/api/webdata/web_data_results.h

@@ -6,6 +6,9 @@
 #define CHROME_BROWSER_API_WEBDATA_WEB_DATA_RESULTS_H_
 
 #include "base/basictypes.h"
+#include "base/callback.h"
+
+class WDTypedResult;
 
 //
 // Result types for WebDataService.
@@ -29,32 +32,47 @@ typedef enum {
   WEB_INTENTS_DEFAULTS_RESULT, // WDResult<std::vector<DefaultWebIntentService>>
 } WDResultType;
 
+
+typedef base::Callback<void(const WDTypedResult*)> DestroyCallback;
+
 //
 // The top level class for a result.
 //
 class WDTypedResult {
  public:
-  virtual ~WDTypedResult() {}
+  virtual ~WDTypedResult();
 
   // Return the result type.
   WDResultType GetType() const {
     return type_;
   }
 
- protected:
-  explicit WDTypedResult(WDResultType type) : type_(type) {
+  void Destroy() const {
+    if (!callback_.is_null()) {
+      callback_.Run(this);
+    }
   }
 
+ protected:
+  explicit WDTypedResult(WDResultType type);
+
+  WDTypedResult(WDResultType type, const DestroyCallback& callback);
+
  private:
   WDResultType type_;
+  DestroyCallback callback_;
   DISALLOW_COPY_AND_ASSIGN(WDTypedResult);
 };
 
 // A result containing one specific pointer or literal value.
 template <class T> class WDResult : public WDTypedResult {
  public:
+  WDResult(WDResultType type, const T& v)
+      : WDTypedResult(type), value_(v) {
+  }
 
-  WDResult(WDResultType type, const T& v) : WDTypedResult(type), value_(v) {
+  WDResult(WDResultType type, const DestroyCallback& callback, const T& v)
+      : WDTypedResult(type, callback), value_(v) {
   }
 
   virtual ~WDResult() {
@@ -73,7 +91,12 @@ template <class T> class WDResult : public WDTypedResult {
 
 template <class T> class WDObjectResult : public WDTypedResult {
  public:
-  explicit WDObjectResult(WDResultType type) : WDTypedResult(type) {
+  explicit WDObjectResult(WDResultType type)
+    : WDTypedResult(type) {
+  }
+
+  WDObjectResult(WDResultType type, const DestroyCallback& callback)
+    : WDTypedResult(type, callback) {
   }
 
   T* GetValue() const {

chrome/browser/webdata/web_data_request_manager.cc

+// Copyright 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/webdata/web_data_request_manager.h"
+
+#include "base/bind.h"
+#include "base/message_loop.h"
+#include "base/stl_util.h"
+#include "chrome/browser/autofill/autofill_profile.h"
+#include "chrome/browser/autofill/credit_card.h"
+#include "chrome/browser/webdata/web_data_service.h"
+
+////////////////////////////////////////////////////////////////////////////////
+//
+// WebDataRequest implementation.
+//
+////////////////////////////////////////////////////////////////////////////////
+
+WebDataRequest::WebDataRequest(WebDataService* service,
+                               WebDataServiceConsumer* consumer,
+                               WebDataRequestManager* manager)
+    : service_(service),
+      cancelled_(false),
+      consumer_(consumer),
+      result_(NULL) {
+  handle_ = manager->GetNextRequestHandle();
+  message_loop_ = MessageLoop::current();
+  manager->RegisterRequest(this);
+}
+
+WebDataRequest::~WebDataRequest() {
+  delete result_;
+}
+
+WebDataService::Handle WebDataRequest::GetHandle() const {
+  return handle_;
+}
+
+WebDataServiceConsumer* WebDataRequest::GetConsumer() const {
+  return consumer_;
+}
+
+bool WebDataRequest::IsCancelled() const {
+  base::AutoLock l(cancel_lock_);
+  return cancelled_;
+}
+
+void WebDataRequest::Cancel() {
+  base::AutoLock l(cancel_lock_);
+  cancelled_ = true;
+  consumer_ = NULL;
+}
+
+void WebDataRequest::SetResult(WDTypedResult* r) {
+  result_ = r;
+}
+
+const WDTypedResult* WebDataRequest::GetResult() const {
+  return result_;
+}
+
+void WebDataRequest::RequestComplete() {
+  message_loop_->PostTask(FROM_HERE, Bind(&WebDataService::RequestCompleted,
+                                          service_.get(), handle_));
+}
+
+////////////////////////////////////////////////////////////////////////////////
+//
+// WebDataRequestManager implementation.
+//
+////////////////////////////////////////////////////////////////////////////////
+
+WebDataRequestManager::WebDataRequestManager()
+    : next_request_handle_(1) {
+}
+
+WebDataRequestManager::~WebDataRequestManager() {
+}
+
+void WebDataRequestManager::RegisterRequest(WebDataRequest* request) {
+  base::AutoLock l(pending_lock_);
+  pending_requests_[request->GetHandle()] = request;
+}
+
+int WebDataRequestManager::GetNextRequestHandle() {
+  base::AutoLock l(pending_lock_);
+  return ++next_request_handle_;
+}
+
+void WebDataRequestManager::CancelRequest(WebDataServiceBase::Handle h) {
+  base::AutoLock l(pending_lock_);
+  RequestMap::iterator i = pending_requests_.find(h);
+  if (i == pending_requests_.end()) {
+    NOTREACHED() << "Canceling a nonexistent web data service request";
+    return;
+  }
+  i->second->Cancel();
+}
+
+void WebDataRequestManager::RequestCompleted(WebDataServiceBase::Handle h) {
+  pending_lock_.Acquire();
+  RequestMap::iterator i = pending_requests_.find(h);
+  if (i == pending_requests_.end()) {
+    NOTREACHED() << "Request completed called for an unknown request";
+    pending_lock_.Release();
+    return;
+  }
+
+  // Take ownership of the request object and remove it from the map.
+  scoped_ptr<WebDataRequest> request(i->second);
+  pending_requests_.erase(i);
+  pending_lock_.Release();
+
+  // Notify the consumer if needed.
+  WebDataServiceConsumer* consumer = request->GetConsumer();
+  if (!request->IsCancelled() && consumer) {
+    consumer->OnWebDataServiceRequestDone(request->GetHandle(),
+                                          request->GetResult());
+  } else {
+    // Nobody is taken ownership of the result, either because it is cancelled
+    // or there is no consumer. Destroy results that require special handling.
+    WDTypedResult const *result = request->GetResult();
+    if (result) {
+      result->Destroy();
+    }
+  }
+}

chrome/browser/webdata/web_data_request_manager.h

+// Copyright 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.
+
+// Chromium settings and storage represent user-selected preferences and
+// information and MUST not be extracted, overwritten or modified except
+// through Chromium defined APIs.
+
+#ifndef CHROME_BROWSER_WEBDATA_WEB_DATA_REQUEST_MANAGER_H__
+#define CHROME_BROWSER_WEBDATA_WEB_DATA_REQUEST_MANAGER_H__
+
+#include <map>
+
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "chrome/browser/api/webdata/web_data_results.h"
+#include "chrome/browser/api/webdata/web_data_service_base.h"
+#include "chrome/browser/api/webdata/web_data_service_consumer.h"
+
+class MessageLoop;
+class WebDataService;
+class WebDataServiceConsumer;
+class WebDataRequestManager;
+
+//////////////////////////////////////////////////////////////////////////////
+//
+// Webdata requests
+//
+// Every request is processed using a request object. The object contains
+// both the request parameters and the results.
+//////////////////////////////////////////////////////////////////////////////
+class WebDataRequest {
+ public:
+  WebDataRequest(WebDataService* service,
+                 WebDataServiceConsumer* consumer,
+                 WebDataRequestManager* manager);
+
+  virtual ~WebDataRequest();
+
+  WebDataServiceBase::Handle GetHandle() const;
+
+  // Retrieves the |consumer_| set in the constructor.
+  WebDataServiceConsumer* GetConsumer() const;
+
+  // Returns |true| if the request was cancelled via the |Cancel()| method.
+  bool IsCancelled() const;
+
+  // This can be invoked from any thread. From this point we assume that
+  // our consumer_ reference is invalid.
+  void Cancel();
+
+  // Invoked by the service when this request has been completed.
+  // This will notify the service in whatever thread was used to create this
+  // request.
+  void RequestComplete();
+
+  // The result is owned by the request.
+  void SetResult(WDTypedResult* r);
+  const WDTypedResult* GetResult() const;
+
+ private:
+  // Used to notify service of request completion.
+  scoped_refptr<WebDataService> service_;
+
+  // Tracks loop that the request originated on.
+  MessageLoop* message_loop_;
+
+  // Identifier for this request.
+  WebDataServiceBase::Handle handle_;
+
+  // A lock to protect against simultaneous cancellations of the request.
+  // Cancellation affects both the |cancelled_| flag and |consumer_|.
+  mutable base::Lock cancel_lock_;
+  bool cancelled_;
+
+  // The originator of the service request.
+  WebDataServiceConsumer* consumer_;
+
+  WDTypedResult* result_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebDataRequest);
+};
+
+//////////////////////////////////////////////////////////////////////////////
+//
+// Webdata request templates
+//
+// Internally we use instances of the following template to represent
+// requests.
+//////////////////////////////////////////////////////////////////////////////
+
+template <class T>
+class GenericRequest : public WebDataRequest {
+ public:
+  GenericRequest(WebDataService* service,
+                 WebDataServiceConsumer* consumer,
+                 WebDataRequestManager* manager,
+                 const T& arg)
+      : WebDataRequest(service, consumer, manager),
+        arg_(arg) {
+  }
+
+  virtual ~GenericRequest() {
+  }
+
+  const T& arg() const { return arg_; }
+
+ private:
+  T arg_;
+};
+
+template <class T, class U>
+class GenericRequest2 : public WebDataRequest {
+ public:
+  GenericRequest2(WebDataService* service,
+                  WebDataServiceConsumer* consumer,
+                  WebDataRequestManager* manager,
+                  const T& arg1,
+                  const U& arg2)
+      : WebDataRequest(service, consumer, manager),
+        arg1_(arg1),
+        arg2_(arg2) {
+  }
+
+  virtual ~GenericRequest2() { }
+
+  const T& arg1() const { return arg1_; }
+
+  const U& arg2() const { return arg2_; }
+
+ private:
+  T arg1_;
+  U arg2_;
+};
+
+//////////////////////////////////////////////////////////////////////////////
+//
+// Webdata Request Manager
+//
+// Tracks all WebDataRequests for a WebDataService.
+//
+// Note: This is an internal interface, not to be used outside of webdata/
+//////////////////////////////////////////////////////////////////////////////
+class WebDataRequestManager {
+ public:
+  WebDataRequestManager();
+
+  ~WebDataRequestManager();
+
+  // Cancel any pending request.
+  void CancelRequest(WebDataServiceBase::Handle h);
+
+  // Invoked by request implementations when a request has been processed.
+  void RequestCompleted(WebDataServiceBase::Handle h);
+
+  // Register the request as a pending request.
+  void RegisterRequest(WebDataRequest* request);
+
+  // Return the next request handle.
+  int GetNextRequestHandle();
+
+ private:
+  // A lock to protect pending requests and next request handle.
+  base::Lock pending_lock_;
+
+  // Next handle to be used for requests. Incremented for each use.
+  WebDataServiceBase::Handle next_request_handle_;
+
+  typedef std::map<WebDataServiceBase::Handle, WebDataRequest*> RequestMap;
+  RequestMap pending_requests_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebDataRequestManager);
+};
+
+#endif  // CHROME_BROWSER_WEBDATA_WEB_DATA_REQUEST_MANAGER_H__

chrome/browser/webdata/web_data_service.h

@@ -27,6 +27,7 @@
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_id.h"
 #include "chrome/browser/webdata/keyword_table.h"
+#include "chrome/browser/webdata/web_data_request_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "sql/init_status.h"
 
@@ -100,106 +101,6 @@ class WebDataService
       public AutofillWebData,
       public RefcountedProfileKeyedService {
  public:
-  //////////////////////////////////////////////////////////////////////////////
-  //
-  // Internal requests
-  //
-  // Every request is processed using a request object. The object contains
-  // both the request parameters and the results.
-  //////////////////////////////////////////////////////////////////////////////
-  class WebDataRequest {
-   public:
-    WebDataRequest(WebDataService* service,
-                   Handle handle,
-                   WebDataServiceConsumer* consumer);
-
-    virtual ~WebDataRequest();
-
-    Handle GetHandle() const;
-
-    // Retrieves the |consumer_| set in the constructor.  If the request was
-    // cancelled via the |Cancel()| method then |true| is returned and
-    // |*consumer| is set to NULL.  The |consumer| parameter may be set to NULL
-    // if only the return value is desired.
-    bool IsCancelled(WebDataServiceConsumer** consumer) const;
-
-    // This can be invoked from any thread. From this point we assume that
-    // our consumer_ reference is invalid.
-    void Cancel();
-
-    // Invoked by the service when this request has been completed.
-    // This will notify the service in whatever thread was used to create this
-    // request.
-    void RequestComplete();
-
-    // The result is owned by the request.
-    void SetResult(WDTypedResult* r);
-    const WDTypedResult* GetResult() const;
-
-   private:
-    scoped_refptr<WebDataService> service_;
-    MessageLoop* message_loop_;
-    Handle handle_;
-
-    // A lock to protect against simultaneous cancellations of the request.
-    // Cancellation affects both the |cancelled_| flag and |consumer_|.
-    mutable base::Lock cancel_lock_;
-    bool cancelled_;
-    WebDataServiceConsumer* consumer_;
-
-    WDTypedResult* result_;
-
-    DISALLOW_COPY_AND_ASSIGN(WebDataRequest);
-  };
-
-  //
-  // Internally we use instances of the following template to represent
-  // requests.
-  //
-  template <class T>
-  class GenericRequest : public WebDataRequest {
-   public:
-    GenericRequest(WebDataService* service,
-                   Handle handle,
-                   WebDataServiceConsumer* consumer,
-                   const T& arg)
-        : WebDataRequest(service, handle, consumer),
-          arg_(arg) {
-    }
-
-    virtual ~GenericRequest() {
-    }
-
-    const T& arg() const { return arg_; }
-
-   private:
-    T arg_;
-  };
-
-  template <class T, class U>
-  class GenericRequest2 : public WebDataRequest {
-   public:
-    GenericRequest2(WebDataService* service,
-                    Handle handle,
-                    WebDataServiceConsumer* consumer,
-                    const T& arg1,
-                    const U& arg2)
-        : WebDataRequest(service, handle, consumer),
-          arg1_(arg1),
-          arg2_(arg2) {
-    }
-
-    virtual ~GenericRequest2() { }
-
-    const T& arg1() const { return arg1_; }
-
-    const U& arg2() const { return arg2_; }
-
-   private:
-    T arg1_;
-    U arg2_;
-  };
-
   WebDataService();
 
   // WebDataServiceBase implementation.
@@ -427,9 +328,6 @@ class WebDataService
   // Invoked by request implementations when a request has been processed.
   void RequestCompleted(Handle h);
 
-  // Register the request as a pending request.
-  void RegisterRequest(WebDataRequest* request);
-
   //////////////////////////////////////////////////////////////////////////////
   //
   // The following methods are only invoked in the web data service thread.
@@ -471,9 +369,6 @@ class WebDataService
   // Schedule a commit if one is not already pending.
   void ScheduleCommit();
 
-  // Return the next request handle.
-  int GetNextRequestHandle();
-
   //////////////////////////////////////////////////////////////////////////////
   //
   // Keywords.
@@ -565,6 +460,11 @@ class WebDataService
   void RemoveAutofillProfilesAndCreditCardsModifiedBetweenImpl(
       GenericRequest2<base::Time, base::Time>* request);
 
+  // Callbacks to ensure that sensitive info is destroyed if request is
+  // cancelled.
+  void DestroyAutofillProfileResult(const WDTypedResult* result);
+  void DestroyAutofillCreditCardResult(const WDTypedResult* result);
+
   // True once initialization has started.
   bool is_running_;
 
@@ -575,6 +475,9 @@ class WebDataService
   // |db_| lifetime must be managed on the database thread.
   WebDatabase* db_;
 
+  // Keeps track of all pending requests made to the db.
+  WebDataRequestManager request_manager_;
+
   // Syncable services for the database data.  We own the services, but don't
   // use |scoped_ptr|s because the lifetimes must be managed on the database
   // thread.
@@ -590,15 +493,6 @@ class WebDataService
   // Whether we should commit the database.
   bool should_commit_;
 
-  // A lock to protect pending requests and next request handle.
-  base::Lock pending_lock_;
-
-  // Next handle to be used for requests. Incremented for each use.
-  Handle next_request_handle_;
-
-  typedef std::map<Handle, WebDataRequest*> RequestMap;
-  RequestMap pending_requests_;
-
   // MessageLoop the WebDataService is created on.
   MessageLoop* main_loop_;
 

chrome/browser/webdata/web_data_service_win.cc

@@ -13,18 +13,14 @@ using base::Bind;
 
 void WebDataService::AddIE7Login(const IE7PasswordInfo& info) {
   GenericRequest<IE7PasswordInfo>* request =
-      new GenericRequest<IE7PasswordInfo>(this, GetNextRequestHandle(), NULL,
-                                          info);
-  RegisterRequest(request);
+      new GenericRequest<IE7PasswordInfo>(this, NULL, &request_manager_, info);
   ScheduleTask(FROM_HERE,
                Bind(&WebDataService::AddIE7LoginImpl, this, request));
 }
 
 void WebDataService::RemoveIE7Login(const IE7PasswordInfo& info) {
   GenericRequest<IE7PasswordInfo>* request =
-      new GenericRequest<IE7PasswordInfo>(this, GetNextRequestHandle(), NULL,
-                                          info);
-  RegisterRequest(request);
+      new GenericRequest<IE7PasswordInfo>(this, NULL, &request_manager_, info);
   ScheduleTask(FROM_HERE,
                Bind(&WebDataService::RemoveIE7LoginImpl, this, request));
 }
@@ -33,16 +29,15 @@ WebDataService::Handle WebDataService::GetIE7Login(
     const IE7PasswordInfo& info,
     WebDataServiceConsumer* consumer) {
   GenericRequest<IE7PasswordInfo>* request =
-      new GenericRequest<IE7PasswordInfo>(this, GetNextRequestHandle(),
-                                          consumer, info);
-  RegisterRequest(request);
+      new GenericRequest<IE7PasswordInfo>(this, consumer, &request_manager_,
+                                          info);
   ScheduleTask(FROM_HERE,
                Bind(&WebDataService::GetIE7LoginImpl, this, request));
   return request->GetHandle();
 }
 
 void WebDataService::AddIE7LoginImpl(GenericRequest<IE7PasswordInfo>* request) {
-  if (db_ && !request->IsCancelled(NULL)) {
+  if (db_ && !request->IsCancelled()) {
     if (db_->GetLoginsTable()->AddIE7Login(request->arg()))
       ScheduleCommit();
   }
@@ -51,7 +46,7 @@ void WebDataService::AddIE7LoginImpl(GenericRequest<IE7PasswordInfo>* request) {
 
 void WebDataService::RemoveIE7LoginImpl(
     GenericRequest<IE7PasswordInfo>* request) {
-  if (db_ && !request->IsCancelled(NULL)) {
+  if (db_ && !request->IsCancelled()) {
     if (db_->GetLoginsTable()->RemoveIE7Login(request->arg()))
       ScheduleCommit();
   }
@@ -60,7 +55,7 @@ void WebDataService::RemoveIE7LoginImpl(
 
 void WebDataService::GetIE7LoginImpl(
     GenericRequest<IE7PasswordInfo>* request) {
-  if (db_ && !request->IsCancelled(NULL)) {
+  if (db_ && !request->IsCancelled()) {
     IE7PasswordInfo result;
     db_->GetLoginsTable()->GetIE7Login(request->arg(), &result);
     request->SetResult(

chrome/chrome_browser.gypi

@@ -116,6 +116,7 @@
         'browser/api/sync/profile_sync_service_observer.h',
         'browser/api/webdata/autofill_web_data.h',
         'browser/api/webdata/autofill_web_data_service.h',
+        'browser/api/webdata/web_data_results.cc',
         'browser/api/webdata/web_data_results.h',
         'browser/api/webdata/web_data_service_base.h',
         'browser/api/webdata/web_data_service_consumer.h',
@@ -2208,6 +2209,8 @@
         'browser/webdata/token_service_table.h',
         'browser/webdata/web_apps_table.cc',
         'browser/webdata/web_apps_table.h',
+        'browser/webdata/web_data_request_manager.cc',
+        'browser/webdata/web_data_request_manager.h',
         'browser/webdata/web_data_service.cc',
         'browser/webdata/web_data_service.h',
         'browser/webdata/web_data_service_factory.cc',