Separate WebDataRequest functionality from WebDataService

chrome/browser/webdata/web_data_request_manager.h

Index: chrome/browser/webdata/web_data_request_manager.h
diff --git a/chrome/browser/webdata/web_data_request_manager.h b/chrome/browser/webdata/web_data_request_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..bd442406b1eab1489aea28280d89ac97e3ff2b2e
--- /dev/null
+++ b/chrome/browser/webdata/web_data_request_manager.h
@@ -0,0 +1,116 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[dhollowa, 2013/01/04 01:34:50]

nit: remove the "(c)" bit.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

+// 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 <string>

[dhollowa, 2013/01/04 01:34:50]

string and vector not used.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

+#include <vector>
+
+#include "base/callback_forward.h"

[dhollowa, 2013/01/04 01:34:50]

A good chunk of these are not required.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

+#include "base/file_path.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner_helpers.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"
+#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 "content/public/browser/browser_thread.h"
+#include "sql/init_status.h"
+
+class MessageLoop;
+class WebDataService;
+class WebDataServiceConsumer;
+
+//////////////////////////////////////////////////////////////////////////////
+//
+// 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,
+                 WebDataServiceBase::Handle handle,
+                 WebDataServiceConsumer* consumer);
+
+  virtual ~WebDataRequest();
+
+  WebDataServiceBase::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;

[dhollowa, 2013/01/04 01:34:50]

Let's take the opportunity to split out a separate accessor for the consumer. 
Mixing the two like this has always bugged me.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

+
+  // 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_;

[dhollowa, 2013/01/04 01:34:50]

Let's add some doc to these members.  Eg.

 service_ -> Used to notify service of request completion.
 message_loop_ -> Tracks loop that the request originated on.
 handle_ -> Identifier for this request.
 consumer_ -> The originator of the service request.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

+  MessageLoop* message_loop_;
+  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_;
+  WebDataServiceConsumer* consumer_;
+
+  WDTypedResult* result_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebDataRequest);
+};
+
+class WebDataRequestManager {

[dhollowa, 2013/01/04 01:34:50]

The order of the classes in this header is opposite of the order of definition
in the .cc.  Please change the .cc to match the .h.

[dhollowa, 2013/01/04 01:34:50]

Add description of this class, making note that this is an "internal" interface,
not to be used outside of webdata/.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

[Cait (Slow), 2013/01/04 22:59:29]

Done.

+ public:
+  WebDataRequestManager();
+
+  // Cancel any pending request.
+  virtual void CancelRequest(WebDataServiceBase::Handle h);
+
+  // Invoked by request implementations when a request has been processed.
+  virtual void RequestCompleted(WebDataServiceBase::Handle h);
+
+  // Register the request as a pending request.
+  virtual void RegisterRequest(WebDataRequest* request);
+
+  // Return the next request handle.
+  virtual 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__