Geolocation cleanup: corrects uses of content::AccessTokenStore* and net::URLRequestContextGetter*

chrome/browser/geolocation/access_token_store_browsertest.cc

Index: chrome/browser/geolocation/access_token_store_browsertest.cc
diff --git a/chrome/browser/geolocation/access_token_store_browsertest.cc b/chrome/browser/geolocation/access_token_store_browsertest.cc
index eb4d011e17eebfad4915b880698ad11a4fc95fb4..ecd26554f8a9052d217f1a6aa82cc39ec3773c44 100644
--- a/chrome/browser/geolocation/access_token_store_browsertest.cc
+++ b/chrome/browser/geolocation/access_token_store_browsertest.cc
@@ -25,11 +25,10 @@ const char* kRefServerUrl1 = "https://test.domain.example/foo?id=bar.bar";
 const char* kRefServerUrl2 = "http://another.domain.example/foo?id=bar.bar#2";
 const char* kOldDefaultNetworkProviderUrl = "https://www.google.com/loc/json";
 
-class GeolocationAccessTokenStoreTest
-    : public InProcessBrowserTest {
+class GeolocationAccessTokenStoreTest : public InProcessBrowserTest {
  public:
   GeolocationAccessTokenStoreTest()
-      : token_to_expect_(NULL), token_to_set_(NULL) {}
+      : token_to_expect_(nullptr), token_to_set_(nullptr) {}
 
   void DoTestStepAndWaitForResults(
       const char* ref_url, const base::string16* token_to_expect,
@@ -37,7 +36,7 @@ class GeolocationAccessTokenStoreTest
 
   void OnAccessTokenStoresLoaded(
       AccessTokenStore::AccessTokenMap access_token_map,
-      net::URLRequestContextGetter* context_getter);
+      const scoped_refptr<net::URLRequestContextGetter>& context_getter);
 
   scoped_refptr<AccessTokenStore> token_store_;
   GURL ref_url_;
@@ -46,12 +45,12 @@ class GeolocationAccessTokenStoreTest
 };
 
 void StartTestStepFromClientThread(
-    scoped_refptr<AccessTokenStore>* store,
+    scoped_refptr<AccessTokenStore> store,

[Wez, 2016/07/18 17:50:53]

Now that this isn't a ptr or reference, you're not actually writing it back into
|token_store_|.  Are you sure the test still works? :P

[mcasas, 2016/07/18 21:48:52]

Interestingly, the test still works. Reverted.

[Wez, 2016/07/18 22:49:18]

It should still say if (!*store), I think.

Looking at what AccessTokenStore does, it seems that it just stores & loads
values from the browser's pref store, which is why breaking this doesn't break
the test - it just makes it [very slightly] less efficient.

I'd suggest just creating the AccessTokenStore on token_store_ on test startup
and getting rid of this on-demand creation stuff. The store isn't thread-bound
(it's ref-counted-thread-safe in fact) so that should be fine - then you just
pass the reference to it into this callback, to actually use.

[mcasas, 2016/07/19 01:18:18]

Done.

     const AccessTokenStore::LoadAccessTokensCallback& callback) {
   ASSERT_TRUE(BrowserThread::CurrentlyOn(kExpectedClientThreadId));
-  if (store->get() == NULL)
-    (*store) = new ChromeAccessTokenStore();
-  (*store)->LoadAccessTokens(callback);
+  if (!store)
+    store = new ChromeAccessTokenStore();
+  store->LoadAccessTokens(callback);
 }
 
 struct TokenLoadClientForTest {
@@ -72,7 +71,7 @@ void GeolocationAccessTokenStoreTest::DoTestStepAndWaitForResults(
       kExpectedClientThreadId, FROM_HERE,
       base::Bind(
           &StartTestStepFromClientThread,
-          &token_store_,
+          token_store_,
           base::Bind(
               &GeolocationAccessTokenStoreTest::OnAccessTokenStoresLoaded,
               base::Unretained(this))));
@@ -81,7 +80,7 @@ void GeolocationAccessTokenStoreTest::DoTestStepAndWaitForResults(
 
 void GeolocationAccessTokenStoreTest::OnAccessTokenStoresLoaded(
     AccessTokenStore::AccessTokenMap access_token_map,
-    net::URLRequestContextGetter* context_getter) {
+    const scoped_refptr<net::URLRequestContextGetter>& context_getter) {
   ASSERT_TRUE(BrowserThread::CurrentlyOn(kExpectedClientThreadId))
       << "Callback from token factory should be from the same thread as the "
          "LoadAccessTokenStores request was made on";
@@ -109,17 +108,17 @@ IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, SetAcrossInstances) {
       base::ASCIIToUTF16("\1\2\3\4\5\6\7\10\11\12=023");
   ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  DoTestStepAndWaitForResults(kRefServerUrl1, NULL, &ref_token1);
+  DoTestStepAndWaitForResults(kRefServerUrl1, nullptr, &ref_token1);
   // Check it was set, and change to new value.
   DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, &ref_token2);
   // And change back.
   DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token2, &ref_token1);
-  DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, NULL);
+  DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, nullptr);
 
   // Set a second server URL
-  DoTestStepAndWaitForResults(kRefServerUrl2, NULL, &ref_token2);
-  DoTestStepAndWaitForResults(kRefServerUrl2, &ref_token2, NULL);
-  DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, NULL);
+  DoTestStepAndWaitForResults(kRefServerUrl2, nullptr, &ref_token2);
+  DoTestStepAndWaitForResults(kRefServerUrl2, &ref_token2, nullptr);
+  DoTestStepAndWaitForResults(kRefServerUrl1, &ref_token1, nullptr);
 }
 
 IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, OldUrlRemoval) {
@@ -128,11 +127,11 @@ IN_PROC_BROWSER_TEST_F(GeolocationAccessTokenStoreTest, OldUrlRemoval) {
 
   // Set a token for the old default network provider url.
   DoTestStepAndWaitForResults(kOldDefaultNetworkProviderUrl,
-                              NULL, &ref_token1);
+                              nullptr, &ref_token1);
   // Check that the token related to the old default network provider url
   // was deleted.
   DoTestStepAndWaitForResults(kOldDefaultNetworkProviderUrl,
-                              NULL, NULL);
+                              nullptr, nullptr);
 }
 
 }  // namespace