Handling of multiple concurrent requests from different clients in OAuth2TokenService

chrome/browser/signin/oauth2_token_service_unittest.cc

Index: chrome/browser/signin/oauth2_token_service_unittest.cc
diff --git a/chrome/browser/signin/oauth2_token_service_unittest.cc b/chrome/browser/signin/oauth2_token_service_unittest.cc
index 730c30365c6d8d4a32e7501e22b1c7e89ad6e326..983d593b0576a2474df99b1cb06dc5f0afefd21f 100644
--- a/chrome/browser/signin/oauth2_token_service_unittest.cc
+++ b/chrome/browser/signin/oauth2_token_service_unittest.cc
@@ -523,3 +523,53 @@ TEST_F(OAuth2TokenServiceTest, CancelRequestsForToken) {
   EXPECT_EQ(0, consumer_.number_of_successful_tokens_);
   EXPECT_EQ(3, consumer_.number_of_errors_);
 }
+
+#if defined(OS_CHROMEOS)
+TEST_F(OAuth2TokenServiceTest, SameScopesRequestedForDifferentClients) {
+  std::string client_id_1("client1");
+  std::string client_secret_1("secret1");
+  std::string client_id_2("client2");
+  std::string client_secret_2("secret2");
+  std::set<std::string> scope_set;
+  scope_set.insert("scope1");
+  scope_set.insert("scope2");
+
+  oauth2_service_->set_refresh_token("refreshToken");
+
+  scoped_ptr<OAuth2TokenService::Request> request1(
+      oauth2_service_->StartRequestForClient(client_id_1,
+                                             client_secret_1,
+                                             scope_set,
+                                             &consumer_));
+  scoped_ptr<OAuth2TokenService::Request> request2(
+      oauth2_service_->StartRequestForClient(client_id_2,
+                                             client_secret_2,
+                                             scope_set,
+                                             &consumer_));
+  // Start a request that should be duplicate of |request1|.
+  scoped_ptr<OAuth2TokenService::Request> request3(
+      oauth2_service_->StartRequestForClient(client_id_1,
+                                             client_secret_1,
+                                             scope_set,
+                                             &consumer_));
+  base::RunLoop().RunUntilIdle();
+
+  // Check to make sure we have started exactly two fetchers.
+  net::TestURLFetcher* fetcher1 = factory_.GetFetcherByID(0);
+  ASSERT_TRUE(fetcher1);
+  net::TestURLFetcher* fetcher2 = factory_.GetFetcherByID(1);
+  ASSERT_TRUE(fetcher2);
+  EXPECT_FALSE(factory_.GetFetcherByID(2));

[(NOT FOR CODE REVIEWS), 2013/08/07 19:56:49]

Instead of this, why not expose a method like GetFetcherCountForTesting()?

Or maybe a method like:

std::vector<std::pair<ClientScopeSet, int waiting_request_count> >
GetFetchingClientScopeSetsForTesting()

Then you can really test things correctly, and you can also undo all the changes
in oauth2_access_token_fetcher.cc.

[zel, 2013/08/08 20:56:20]

The biggest problem here was that |factory_| implementation does not really
store anything else other than the last fetcher at index 0
(factory_.GetFetcherByID(0)) since fetcher's id was hardcoded to 0 in
OAuth2AccessTokenFetcher. So, the count of fetcher would always be 1 or 0...

I have exposed factory_.GetFetcherCount() to make some checks more human
readable in these unit test. PTAL.

[Roger Tawa OOO till Jul 10th, 2013/08/09 14:22:14]

I meant putting these methods on OAuth2TokenService, not the fetcher factory. 
So for example you could replace the lines 572-576 with:

   ASSERT_EQ(2U, oauth2_service->GetFetcherCount());

As for the second proposed new method, maybe a better prototype would be:

   const std::map<FetchParameters, Fetcher*>&
       OAuth2TokenService::GetPendingFetchersForTesting();

and you could then do something like:

   const std::map<FetchParameters, Fetcher*>& fetchers =
       oauth2_service->GetPendingFetchersForTesting();

   ASSERT_EQ(2U,
             fetchers[FetchParameters(std::string(),
                                      client_id_1,
                                      oauth2_service->GetRefreshToken(),
                                      scope_set)]->GetWaitingRequestCount());
   ASSERT_EQ(1U,
             fetchers[ClientScopeSet(std::string(),
                                     client_id_2,
                                     oauth2_service->GetRefreshToken(),
                                     scope_set)]->GetWaitingRequestCount());

[zel, 2013/08/15 01:26:42]

OK, check it out now... way more stuff needed to be moved around with this
approach. 

+
+  fetcher1->set_response_code(net::HTTP_OK);
+  fetcher1->SetResponseString(GetValidTokenResponse("token1", 3600));
+  fetcher1->delegate()->OnURLFetchComplete(fetcher1);
+
+  fetcher2->set_response_code(net::HTTP_OK);
+  fetcher2->SetResponseString(GetValidTokenResponse("token2", 3600));
+  fetcher2->delegate()->OnURLFetchComplete(fetcher2);
+
+  EXPECT_EQ(3, consumer_.number_of_successful_tokens_);
+  EXPECT_EQ(0, consumer_.number_of_errors_);
+}
+#endif