Revert 114992 - Switch to PostTaskAndReply for AccessTokenStore.

This removes the only usage of the CancelableRequest class in
content/, and is in preparation for moving the class back to
chrome/browser.

The ability to cancel requests to the AccessTokenStore was never used in production, so the interface now changes to a simpler non-cancelable one, and the browsertest GeolocationAccessTokenStoreTest.CancelRequest which tested the functionality is removed.

BUG=98716

Review URL: http://codereview.chromium.org/8996006

TBR=joi@chromium.org
Review URL: http://codereview.chromium.org/8992018

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

12 files changed, 138 insertions, 92 deletions

diff --git a/chrome/browser/geolocation/access_token_store_browsertest.cc b/chrome/browser/geolocation/access_token_store_browsertest.cc
index b9ede22..22729d5 100644
--- a/chrome/browser/geolocation/access_token_store_browsertest.cc
+++ b/chrome/browser/geolocation/access_token_store_browsertest.cc
@@ -38,6 +38,7 @@ class GeolocationAccessTokenStoreTest
       net::URLRequestContextGetter* context_getter);
 
   scoped_refptr<AccessTokenStore> token_store_;
+  CancelableRequestConsumer request_consumer_;
   GURL ref_url_;
   const string16* token_to_expect_;
   const string16* token_to_set_;
@@ -45,11 +46,12 @@ class GeolocationAccessTokenStoreTest
 
 void StartTestStepFromClientThread(
     scoped_refptr<AccessTokenStore>* store,
+    CancelableRequestConsumerBase* consumer,
     const AccessTokenStore::LoadAccessTokensCallbackType& callback) {
   ASSERT_TRUE(BrowserThread::CurrentlyOn(kExpectedClientThreadId));
   if (*store == NULL)
     (*store) = new ChromeAccessTokenStore();
-  (*store)->LoadAccessTokens(callback);
+  (*store)->LoadAccessTokens(consumer, callback);
 }
 
 struct TokenLoadClientForTest {
@@ -59,6 +61,43 @@ struct TokenLoadClientForTest {
   }
 };
 
+void RunCancelTestInClientTread() {
+  ASSERT_TRUE(BrowserThread::CurrentlyOn(kExpectedClientThreadId));
+  scoped_refptr<AccessTokenStore> store(new ChromeAccessTokenStore());
+  CancelableRequestConsumer consumer;
+  TokenLoadClientForTest load_client;
+
+  // Single request, canceled explicitly
+  CancelableRequestProvider::Handle first_handle =
+      store->LoadAccessTokens(
+          &consumer,
+          base::Bind(&TokenLoadClientForTest::NotReachedCallback,
+                     base::Unretained(&load_client)));
+  EXPECT_TRUE(consumer.HasPendingRequests());
+  // Test this handle is valid.
+  consumer.GetClientData(store.get(), first_handle);
+  store->CancelRequest(first_handle);
+  EXPECT_FALSE(consumer.HasPendingRequests());
+
+  // 2 requests, canceled globally.
+  store->LoadAccessTokens(
+      &consumer,
+      base::Bind(&TokenLoadClientForTest::NotReachedCallback,
+                 base::Unretained(&load_client)));
+  store->LoadAccessTokens(
+      &consumer,
+      base::Bind(&TokenLoadClientForTest::NotReachedCallback,
+                 base::Unretained(&load_client)));
+  EXPECT_TRUE(consumer.HasPendingRequests());
+  EXPECT_EQ(2u, consumer.PendingRequestCount());
+  consumer.CancelAllRequests();
+  EXPECT_FALSE(consumer.HasPendingRequests());
+  EXPECT_EQ(0u, consumer.PendingRequestCount());
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE, MessageLoop::QuitClosure());
+}
+
 void GeolocationAccessTokenStoreTest::DoTestStepAndWaitForResults(
     const char* ref_url, const string16* token_to_expect,
     const string16* token_to_set) {
@@ -68,12 +107,10 @@ void GeolocationAccessTokenStoreTest::DoTestStepAndWaitForResults(
 
   BrowserThread::PostTask(
       kExpectedClientThreadId, FROM_HERE,
-      base::Bind(
-          &StartTestStepFromClientThread,
-          &token_store_,
-          base::Bind(
-              &GeolocationAccessTokenStoreTest::OnAccessTokenStoresLoaded,
-              base::Unretained(this))));
+      base::Bind(&StartTestStepFromClientThread, &token_store_,
+                 &request_consumer_,
+                 base::Bind(&GeolocationAccessTokenStoreTest::
+                     OnAccessTokenStoresLoaded, base::Unretained(this))));
   ui_test_utils::RunMessageLoop();
 }
 
@@ -132,4 +169,10 @@ IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, OldUrlRemoval) {
                               NULL, NULL);
 }
 
+IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, CancelRequest) {
+  BrowserThread::PostTask(kExpectedClientThreadId, FROM_HERE,
+                          base::Bind(&RunCancelTestInClientTread));
+  ui_test_utils::RunMessageLoop();
+}
+
 }  // namespace
diff --git a/chrome/browser/geolocation/chrome_access_token_store.cc b/chrome/browser/geolocation/chrome_access_token_store.cc
index fb0c281..d0fcd39 100644
--- a/chrome/browser/geolocation/chrome_access_token_store.cc
+++ b/chrome/browser/geolocation/chrome_access_token_store.cc
@@ -18,67 +18,9 @@
 using content::BrowserThread;
 
 namespace {
-
 // This was the default location service url for Chrome versions 14 and earlier
 // but is no longer supported.
 const char* kOldDefaultNetworkProviderUrl = "https://www.google.com/loc/json";
-
-// Loads access tokens and other necessary data on the UI thread, and
-// calls back to the originator on the originating threaad.
-class TokenLoadingJob : public base::RefCountedThreadSafe<TokenLoadingJob> {
- public:
-  TokenLoadingJob(
-      const AccessTokenStore::LoadAccessTokensCallbackType& callback)
-      : callback_(callback) {
-  }
-
-  void Run() {
-    BrowserThread::PostTaskAndReply(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&TokenLoadingJob::PerformWorkOnUIThread, this),
-        base::Bind(&TokenLoadingJob::RespondOnOriginatingThread, this));
-  }
-
- private:
-  void PerformWorkOnUIThread() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    DictionaryPrefUpdate update(g_browser_process->local_state(),
-                                prefs::kGeolocationAccessToken);
-    DictionaryValue* token_dictionary = update.Get();
-
-    bool has_old_network_provider_url = false;
-    // The dictionary value could be NULL if the pref has never been set.
-    if (token_dictionary != NULL) {
-      for (DictionaryValue::key_iterator it = token_dictionary->begin_keys();
-           it != token_dictionary->end_keys(); ++it) {
-        GURL url(*it);
-        if (!url.is_valid())
-          continue;
-        if (url.spec() == kOldDefaultNetworkProviderUrl) {
-          has_old_network_provider_url = true;
-          continue;
-        }
-        token_dictionary->GetStringWithoutPathExpansion(
-            *it, &access_token_set_[url]);
-      }
-      if (has_old_network_provider_url)
-        token_dictionary->RemoveWithoutPathExpansion(
-            kOldDefaultNetworkProviderUrl, NULL);
-    }
-
-    system_request_context_ = g_browser_process->system_request_context();
-  }
-
-  void RespondOnOriginatingThread() {
-    callback_.Run(access_token_set_, system_request_context_);
-  }
-
-  AccessTokenStore::LoadAccessTokensCallbackType callback_;
-  AccessTokenStore::AccessTokenSet access_token_set_;
-  net::URLRequestContextGetter* system_request_context_;
-};
-
 }  // namespace
 
 void ChromeAccessTokenStore::RegisterPrefs(PrefService* prefs) {
@@ -88,13 +30,45 @@ void ChromeAccessTokenStore::RegisterPrefs(PrefService* prefs) {
 ChromeAccessTokenStore::ChromeAccessTokenStore() {
 }
 
-ChromeAccessTokenStore::~ChromeAccessTokenStore() {
+void ChromeAccessTokenStore::LoadDictionaryStoreInUIThread(
+      scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (request->canceled())
+    return;
+  DictionaryPrefUpdate update(g_browser_process->local_state(),
+                              prefs::kGeolocationAccessToken);
+  DictionaryValue* token_dictionary = update.Get();
+
+  AccessTokenStore::AccessTokenSet access_token_set;
+  bool has_old_network_provider_url = false;
+  // The dictionary value could be NULL if the pref has never been set.
+  if (token_dictionary != NULL) {
+    for (DictionaryValue::key_iterator it = token_dictionary->begin_keys();
+        it != token_dictionary->end_keys(); ++it) {
+      GURL url(*it);
+      if (!url.is_valid())
+        continue;
+      if (url.spec() == kOldDefaultNetworkProviderUrl) {
+        has_old_network_provider_url = true;
+        continue;
+      }
+      token_dictionary->GetStringWithoutPathExpansion(*it,
+                                                      &access_token_set[url]);
+    }
+    if (has_old_network_provider_url)
+      token_dictionary->RemoveWithoutPathExpansion(
+          kOldDefaultNetworkProviderUrl, NULL);
+  }
+  request->ForwardResultAsync(access_token_set,
+                              g_browser_process->system_request_context());
 }
 
-void ChromeAccessTokenStore::LoadAccessTokens(
-    const LoadAccessTokensCallbackType& callback) {
-  scoped_refptr<TokenLoadingJob> job(new TokenLoadingJob(callback));
-  job->Run();
+void ChromeAccessTokenStore::DoLoadAccessTokens(
+    scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ChromeAccessTokenStore::LoadDictionaryStoreInUIThread, this,
+                 request));
 }
 
 void SetAccessTokenOnUIThread(const GURL& server_url, const string16& token) {
diff --git a/chrome/browser/geolocation/chrome_access_token_store.h b/chrome/browser/geolocation/chrome_access_token_store.h
index 49c143e..f61d3976 100644
--- a/chrome/browser/geolocation/chrome_access_token_store.h
+++ b/chrome/browser/geolocation/chrome_access_token_store.h
@@ -17,13 +17,15 @@ class ChromeAccessTokenStore : public AccessTokenStore {
   static void RegisterPrefs(PrefService* prefs);
 
   ChromeAccessTokenStore();
-  virtual ~ChromeAccessTokenStore();
-
-  virtual void LoadAccessTokens(
-      const LoadAccessTokensCallbackType& request) OVERRIDE;
 
  private:
+  void LoadDictionaryStoreInUIThread(
+      scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request);
+
   // AccessTokenStore
+  virtual void DoLoadAccessTokens(
+      scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request)
+          OVERRIDE;
   virtual void SaveAccessToken(
       const GURL& server_url, const string16& access_token) OVERRIDE;
 
diff --git a/content/browser/geolocation/access_token_store.cc b/content/browser/geolocation/access_token_store.cc
index 5345661..45d0203 100644
--- a/content/browser/geolocation/access_token_store.cc
+++ b/content/browser/geolocation/access_token_store.cc
@@ -9,3 +9,15 @@ AccessTokenStore::AccessTokenStore() {
 
 AccessTokenStore::~AccessTokenStore() {
 }
+
+AccessTokenStore::Handle AccessTokenStore::LoadAccessTokens(
+    CancelableRequestConsumerBase* consumer,
+    const LoadAccessTokensCallbackType& callback) {
+  scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request(
+      new CancelableRequest<LoadAccessTokensCallbackType>(callback));
+  AddRequest(request, consumer);
+  DCHECK(request->handle());
+
+  DoLoadAccessTokens(request);
+  return request->handle();
+}
diff --git a/content/browser/geolocation/access_token_store.h b/content/browser/geolocation/access_token_store.h
index 5fc2c57..2089dfb 100644
--- a/content/browser/geolocation/access_token_store.h
+++ b/content/browser/geolocation/access_token_store.h
@@ -19,6 +19,7 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/string16.h"
+#include "content/browser/cancelable_request.h"
 #include "content/common/content_export.h"
 #include "googleurl/src/gurl.h"
 
@@ -29,8 +30,10 @@ class URLRequestContextGetter;
 }
 
 // Provides storage for the access token used in the network request.
-class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore> {
+class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore>,
+                         public CancelableRequestProvider {
  public:
+
   // Map of server URLs to associated access token.
   typedef std::map<GURL, string16> AccessTokenSet;
   typedef base::Callback<void(AccessTokenSet, net::URLRequestContextGetter*)>
@@ -42,8 +45,10 @@ class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore> {
   // in Chrome the call to obtain this must also be performed on the UI thread
   // so it is efficient to piggyback it onto this request.
   // Takes ownership of |callback|.
-  CONTENT_EXPORT virtual void LoadAccessTokens(
-      const LoadAccessTokensCallbackType& callback) = 0;
+  // Returns a handle which can subsequently be used with CancelRequest().
+  CONTENT_EXPORT Handle LoadAccessTokens(
+      CancelableRequestConsumerBase* consumer,
+      const LoadAccessTokensCallbackType& callback);
 
   virtual void SaveAccessToken(
       const GURL& server_url, const string16& access_token) = 0;
@@ -53,6 +58,9 @@ class AccessTokenStore : public base::RefCountedThreadSafe<AccessTokenStore> {
   CONTENT_EXPORT AccessTokenStore();
   CONTENT_EXPORT virtual ~AccessTokenStore();
 
+  virtual void DoLoadAccessTokens(
+      scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > req) = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AccessTokenStore);
 };
diff --git a/content/browser/geolocation/arbitrator_dependency_factory.cc b/content/browser/geolocation/arbitrator_dependency_factory.cc
index 88fccfc..25f3954 100644
--- a/content/browser/geolocation/arbitrator_dependency_factory.cc
+++ b/content/browser/geolocation/arbitrator_dependency_factory.cc
@@ -4,7 +4,6 @@
 
 #include "content/browser/geolocation/arbitrator_dependency_factory.h"
 
-#include "base/time.h"
 #include "content/browser/geolocation/access_token_store.h"
 #include "content/browser/geolocation/location_provider.h"
 #include "content/public/browser/content_browser_client.h"
diff --git a/content/browser/geolocation/fake_access_token_store.cc b/content/browser/geolocation/fake_access_token_store.cc
index dacacab..4b2dc53 100644
--- a/content/browser/geolocation/fake_access_token_store.cc
+++ b/content/browser/geolocation/fake_access_token_store.cc
@@ -4,28 +4,30 @@
 
 #include "content/browser/geolocation/fake_access_token_store.h"
 
-#include "base/logging.h"
-
 using testing::_;
 using testing::Invoke;
 
 FakeAccessTokenStore::FakeAccessTokenStore() {
-  ON_CALL(*this, LoadAccessTokens(_))
+  ON_CALL(*this, DoLoadAccessTokens(_))
       .WillByDefault(Invoke(this,
-                            &FakeAccessTokenStore::DefaultLoadAccessTokens));
+                            &FakeAccessTokenStore::DefaultDoLoadAccessTokens));
   ON_CALL(*this, SaveAccessToken(_, _))
       .WillByDefault(Invoke(this,
                             &FakeAccessTokenStore::DefaultSaveAccessToken));
 }
 
 void FakeAccessTokenStore::NotifyDelegateTokensLoaded() {
+  CHECK(request_ != NULL);
   net::URLRequestContextGetter* context_getter = NULL;
-  callback_.Run(access_token_set_, context_getter);
+  request_->ForwardResult(access_token_set_, context_getter);
+  request_ = NULL;
 }
 
-void FakeAccessTokenStore::DefaultLoadAccessTokens(
-    const LoadAccessTokensCallbackType& callback) {
-  callback_ = callback;
+void FakeAccessTokenStore::DefaultDoLoadAccessTokens(
+    scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request) {
+  DCHECK(request_ == NULL)
+      << "Fake token store currently only allows one request at a time";
+  request_ = request;
 }
 
 void FakeAccessTokenStore::DefaultSaveAccessToken(
diff --git a/content/browser/geolocation/fake_access_token_store.h b/content/browser/geolocation/fake_access_token_store.h
index 3276159..a3ee08a 100644
--- a/content/browser/geolocation/fake_access_token_store.h
+++ b/content/browser/geolocation/fake_access_token_store.h
@@ -18,18 +18,20 @@ class FakeAccessTokenStore : public AccessTokenStore {
   void NotifyDelegateTokensLoaded();
 
   // AccessTokenStore
-  MOCK_METHOD1(LoadAccessTokens,
-               void(const LoadAccessTokensCallbackType& callback));
+  MOCK_METHOD1(DoLoadAccessTokens,
+               void(scoped_refptr<CancelableRequest
+                    <LoadAccessTokensCallbackType> > request));
   MOCK_METHOD2(SaveAccessToken,
                void(const GURL& server_url, const string16& access_token));
 
-  void DefaultLoadAccessTokens(const LoadAccessTokensCallbackType& callback);
+  void DefaultDoLoadAccessTokens(
+      scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request);
 
   void DefaultSaveAccessToken(const GURL& server_url,
                               const string16& access_token);
 
   AccessTokenSet access_token_set_;
-  LoadAccessTokensCallbackType callback_;
+  scoped_refptr<CancelableRequest<LoadAccessTokensCallbackType> > request_;
 
  private:
   virtual ~FakeAccessTokenStore();
diff --git a/content/browser/geolocation/geolocation_provider_unittest.cc b/content/browser/geolocation/geolocation_provider_unittest.cc
index 089de94..2b899c1 100644
--- a/content/browser/geolocation/geolocation_provider_unittest.cc
+++ b/content/browser/geolocation/geolocation_provider_unittest.cc
@@ -141,10 +141,10 @@ TEST_F(GeolocationProviderTest, StartStop) {
       new MockDependencyFactory(&message_loop_, fake_access_token_store.get());
   base::WaitableEvent event(false, false);
 
-  EXPECT_CALL(*(fake_access_token_store.get()), LoadAccessTokens(_))
+  EXPECT_CALL(*(fake_access_token_store.get()), DoLoadAccessTokens(_))
       .Times(1)
       .WillOnce(DoAll(Invoke(fake_access_token_store.get(),
-                             &FakeAccessTokenStore::DefaultLoadAccessTokens),
+                             &FakeAccessTokenStore::DefaultDoLoadAccessTokens),
                       InvokeWithoutArgs(&event, &base::WaitableEvent::Signal)));
 
   GeolocationArbitrator::SetDependencyFactoryForTest(dependency_factory.get());
diff --git a/content/browser/geolocation/location_arbitrator.cc b/content/browser/geolocation/location_arbitrator.cc
index 1c06244..2765cdc 100644
--- a/content/browser/geolocation/location_arbitrator.cc
+++ b/content/browser/geolocation/location_arbitrator.cc
@@ -64,6 +64,7 @@ void GeolocationArbitrator::StartProviders(
   if (providers_.empty()) {
     DCHECK(GURL(kDefaultNetworkProviderUrl).is_valid());
     access_token_store_->LoadAccessTokens(
+        &request_consumer_,
         base::Bind(&GeolocationArbitrator::OnAccessTokenStoresLoaded,
                    base::Unretained(this)));
   } else {
diff --git a/content/browser/geolocation/location_arbitrator.h b/content/browser/geolocation/location_arbitrator.h
index 68d6e11..b020191 100644
--- a/content/browser/geolocation/location_arbitrator.h
+++ b/content/browser/geolocation/location_arbitrator.h
@@ -96,6 +96,7 @@ class CONTENT_EXPORT GeolocationArbitrator
   // The provider which supplied the current |position_|
   const LocationProviderBase* position_provider_;
   GURL most_recent_authorized_frame_;
+  CancelableRequestConsumer request_consumer_;
   // The current best estimate of our position.
   Geoposition position_;
 
diff --git a/content/browser/geolocation/location_arbitrator_unittest.cc b/content/browser/geolocation/location_arbitrator_unittest.cc
index f730dc5..b87b600 100644
--- a/content/browser/geolocation/location_arbitrator_unittest.cc
+++ b/content/browser/geolocation/location_arbitrator_unittest.cc
@@ -167,7 +167,9 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   arbitrator_->StartProviders(GeolocationObserverOptions(false));
 
   EXPECT_TRUE(access_token_store_->access_token_set_.empty());
+  EXPECT_TRUE(access_token_store_->request_);
   EXPECT_TRUE(access_token_store_->access_token_set_.empty());
+  ASSERT_TRUE(access_token_store_->request_);
 
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());