GaiaCookieServiceManager handles general request types

components/signin/core/browser/gaia_cookie_manager_service.cc

Index: components/signin/core/browser/gaia_cookie_manager_service.cc
diff --git a/components/signin/core/browser/gaia_cookie_manager_service.cc b/components/signin/core/browser/gaia_cookie_manager_service.cc
index 20e96f5f0adaa13c326b6fe928b4e62a0e5399e6..f2c12a96bb1ea7f678a0528627433b285d3663f3 100644
--- a/components/signin/core/browser/gaia_cookie_manager_service.cc
+++ b/components/signin/core/browser/gaia_cookie_manager_service.cc
@@ -4,6 +4,7 @@
 
 #include "components/signin/core/browser/gaia_cookie_manager_service.h"
 
+#include <queue>
 #include <vector>
 
 #include "base/json/json_reader.h"
@@ -22,6 +23,47 @@
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 
+GaiaCookieManagerService::GaiaCookieRequest::GaiaCookieRequest(
+    GaiaCookieRequestType request_type,
+    const std::string& account_id,
+    const GaiaCookieManagerService::ListAccountsCallback&
+        list_accounts_callback)
+  : request_type_(request_type),
+    account_id_(account_id),
+    list_accounts_callback_(list_accounts_callback) {}
+
+GaiaCookieManagerService::GaiaCookieRequest::~GaiaCookieRequest() {
+}
+
+// static
+GaiaCookieManagerService::GaiaCookieRequest
+GaiaCookieManagerService::GaiaCookieRequest::AddAccountRequest(
+      const std::string& account_id) {
+  return GaiaCookieManagerService::GaiaCookieRequest(
+      GaiaCookieManagerService::GaiaCookieRequestType::ADD_ACCOUNT,
+      account_id,
+      GaiaCookieManagerService::ListAccountsCallback());
+}
+
+// static
+GaiaCookieManagerService::GaiaCookieRequest
+GaiaCookieManagerService::GaiaCookieRequest::LogOutRequest() {
+  return GaiaCookieManagerService::GaiaCookieRequest(
+      GaiaCookieManagerService::GaiaCookieRequestType::LOG_OUT,
+      std::string(),
+      GaiaCookieManagerService::ListAccountsCallback());
+}
+
+GaiaCookieManagerService::GaiaCookieRequest
+GaiaCookieManagerService::GaiaCookieRequest::ListAccountsRequest(
+    const GaiaCookieManagerService::ListAccountsCallback&
+        list_accounts_callback) {
+  return GaiaCookieManagerService::GaiaCookieRequest(
+      GaiaCookieManagerService::GaiaCookieRequestType::LIST_ACCOUNTS,
+      std::string(),
+      list_accounts_callback);
+}
+
 GaiaCookieManagerService::ExternalCcResultFetcher::ExternalCcResultFetcher(
     GaiaCookieManagerService* helper)
     : helper_(helper) {
@@ -192,84 +234,97 @@ GaiaCookieManagerService::GaiaCookieManagerService(
 
 GaiaCookieManagerService::~GaiaCookieManagerService() {
   CancelAll();
-  DCHECK(accounts_.empty());
+  DCHECK(requests_.empty());
 }
 
 void GaiaCookieManagerService::AddAccountToCookie(
     const std::string& account_id) {
   DCHECK(!account_id.empty());
   VLOG(1) << "GaiaCookieManagerService::AddAccountToCookie: " << account_id;
-  accounts_.push_back(account_id);
-  if (accounts_.size() == 1)
-    StartFetching();
+  requests_.push_back(GaiaCookieRequest::AddAccountRequest(account_id));
+  if (requests_.size() == 1)
+    StartFetchingUbertoken();
 }
 
-void GaiaCookieManagerService::AddObserver(Observer* observer) {
-  observer_list_.AddObserver(observer);
-}
+void GaiaCookieManagerService::ListAccounts(
+    const ListAccountsCallback& callback) {

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:22]

|callback| is never used.  Should this function have a NOTREACHED() macro until
it's fully implemented?

[Mike Lerman, 2015/04/01 16:39:35]

Sure, will do. Good call.

+  // TODO(mlerman): Once this service listens to all GAIA cookie changes, cache
+  // the results of ListAccounts, and return them here if the GAIA cookie
+  // hasn't changed since the last call.
 
-void GaiaCookieManagerService::RemoveObserver(Observer* observer) {
-  observer_list_.RemoveObserver(observer);
-}
+  // If there's a GAIA call being executed, wait for it to complete. If it was
+  // another /ListAccounts then we'll use the results it caches.
+  if (gaia_auth_fetcher_) {
 
-void GaiaCookieManagerService::CancelAll() {
-  VLOG(1) << "GaiaCookieManagerService::CancelAll";
-  gaia_auth_fetcher_.reset();
-  uber_token_fetcher_.reset();
-  accounts_.clear();
-}
+    return;
+  }

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:22]

Don't need { }.

[Mike Lerman, 2015/04/01 16:39:35]

Done.

 
-void GaiaCookieManagerService::LogOut(
-    const std::string& account_id,
-    const std::vector<std::string>& accounts) {
-  DCHECK(!account_id.empty());
-  VLOG(1) << "GaiaCookieManagerService::LogOut: " << account_id
-          << " accounts=" << accounts.size();
-  LogOutInternal(account_id, accounts);
+

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:23]

nit: remove blank line.

[Mike Lerman, 2015/04/01 16:39:35]

Done.

+  VLOG(1) << "GaiaCookieManagerService::ListAccounts";
+  gaia_auth_fetcher_.reset(
+      new GaiaAuthFetcher(this, source_,
+                          signin_client_->GetURLRequestContext()));
+  gaia_auth_fetcher_->StartListAccounts();
 }
 
-void GaiaCookieManagerService::LogOutInternal(
-    const std::string& account_id,
-    const std::vector<std::string>& accounts) {
-  bool pending = !accounts_.empty();
-
-  if (pending) {
-    for (std::deque<std::string>::const_iterator it = accounts_.begin() + 1;
-         it != accounts_.end(); it++) {
-      if (!it->empty() &&
-          (std::find(accounts.begin(), accounts.end(), *it) == accounts.end() ||
-           *it == account_id)) {
+void GaiaCookieManagerService::LogOutAllAccounts() {
+  VLOG(1) << "GaiaCookieManagerService::LogOutAllAccounts";
+
+  bool log_out_queued = false;
+  if (!requests_.empty()) {
+    // Track requests to keep; all other unstarted requests will be removed.
+    std::vector<GaiaCookieRequest> requests_to_keep;
+
+    // Check all pending, non-executing requests.
+    for (auto it = requests_.begin() + 1;
+        it != requests_.end(); it++) {

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:23]

use ++it

Should fit one line?

[Mike Lerman, 2015/04/01 16:39:35]

Ah yes. I didn't used to have auto, and it had needed 2 lines.
Thanks.

+      if (it->request_type() == GaiaCookieRequestType::ADD_ACCOUNT) {
         // We have a pending log in request for an account followed by
         // a signout.
         GoogleServiceAuthError error(GoogleServiceAuthError::REQUEST_CANCELED);
-        SignalComplete(*it, error);
+        SignalComplete(it->account_id(), error);
       }
-    }
 
-    // Remove every thing in the work list besides the one that is running.
-    accounts_.resize(1);
-  }
+      // ADD_ACCOUNT requests should be removed.

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:23]

Comment does not seem to match the code, but I think it's a moot point, see
comment at line 305.

[Mike Lerman, 2015/04/01 16:39:35]

Done.

+      if (it->request_type() != GaiaCookieRequestType::ADD_ACCOUNT)
+        requests_to_keep.push_back(*it);
+
+      // Verify a LOG_OUT isn't already queued.
+      if (it->request_type() == GaiaCookieRequestType::LOG_OUT)
+        log_out_queued = true;
+    }
 
-  // Signal a logout to be the next thing to do unless the pending
-  // action is already a logout.
-  if (!pending || !accounts_.front().empty())
-    accounts_.push_back("");
+    // Verify a LOG_OUT isn't currently being processed.
+    if (requests_.front().request_type() == GaiaCookieRequestType::LOG_OUT)
+      log_out_queued = true;
 
-  for (std::vector<std::string>::const_iterator it = accounts.begin();
-       it != accounts.end(); it++) {
-    if (*it != account_id) {
-      DCHECK(!it->empty());
-      accounts_.push_back(*it);
+    if (requests_.size() > 1) {
+      requests_.erase(requests_.begin() + 1, requests_.end());
+      requests_.insert(
+          requests_.end(), requests_to_keep.begin(), requests_to_keep.end());
     }

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:23]

I don't understand this if block.  The new code for LogOutAllAccounts() should
behave like the old code for the case LogOutInternal("",
std::vector<std::string>()).  This means that:

1/ all pending ADD_ACCOUNTs should never be re-added

2/ if we get to line 301, only want to keep the currently pending request, so
lines 301-305 should be replaced with |requests_.resize(1)|.

You don't need the local var |requests_to_keep|.

Please keep the comment at old line 249 and put right above line 301.

[Mike Lerman, 2015/04/01 16:39:35]

We discussed offline.

   }
 
-  if (!pending)
+  if (!log_out_queued)
+    requests_.push_back(GaiaCookieRequest::LogOutRequest());
+
+  if (requests_.size() == 1)
     StartLogOutUrlFetch();

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:23]

If a logout was already pending, then you get here and with a |request| size of
1.  This will start a new fetch when not needed.  You need to move line 312 into
the if block at line 309.  No need to check |request.size()|.

[Mike Lerman, 2015/04/01 16:39:35]

Good point. Done.

 }

[Roger Tawa OOO till Jul 10th, 2015/03/30 18:57:23]

I'm not sure this passes the unit tests, did you run them?

I think you should consider rewriting LogOutAllAccounts() by starting with the
existing LogOutInternal().  Make the assumption that |account_id| and |accounts|
are always empty and remove dead code.  Then adapt it from a list of strings to
a list of GaiaCookieRequests.

[Mike Lerman, 2015/04/01 16:39:35]

I did ;) And I'll write more as you suggested.

 
-void GaiaCookieManagerService::LogOutAllAccounts() {
-  VLOG(1) << "GaiaCookieManagerService::LogOutAllAccounts";
-  LogOutInternal("", std::vector<std::string>());
+void GaiaCookieManagerService::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void GaiaCookieManagerService::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void GaiaCookieManagerService::CancelAll() {
+  VLOG(1) << "GaiaCookieManagerService::CancelAll";
+  gaia_auth_fetcher_.reset();
+  uber_token_fetcher_.reset();
+  requests_.clear();
 }
 
 void GaiaCookieManagerService::SignalComplete(
@@ -288,7 +343,7 @@ void GaiaCookieManagerService::StartFetchingExternalCcResult() {
 }
 
 void GaiaCookieManagerService::StartLogOutUrlFetch() {
-  DCHECK(accounts_.front().empty());
+  DCHECK(requests_.front().request_type() == GaiaCookieRequestType::LOG_OUT);
   VLOG(1) << "GaiaCookieManagerService::StartLogOutUrlFetch";
   GURL logout_url(GaiaUrls::GetInstance()->service_logout_url().Resolve(
       base::StringPrintf("?source=%s", source_.c_str())));
@@ -301,7 +356,7 @@ void GaiaCookieManagerService::StartLogOutUrlFetch() {
 void GaiaCookieManagerService::OnUbertokenSuccess(
     const std::string& uber_token) {
   VLOG(1) << "GaiaCookieManagerService::OnUbertokenSuccess"
-          << " account=" << accounts_.front();
+          << " account=" << requests_.front().account_id();
   gaia_auth_fetcher_.reset(
       new GaiaAuthFetcher(this, source_,
                           signin_client_->GetURLRequestContext()));
@@ -315,56 +370,64 @@ void GaiaCookieManagerService::OnUbertokenSuccess(
 void GaiaCookieManagerService::OnUbertokenFailure(
     const GoogleServiceAuthError& error) {
   VLOG(1) << "Failed to retrieve ubertoken"
-          << " account=" << accounts_.front() << " error=" << error.ToString();
-  const std::string account_id = accounts_.front();
-  HandleNextAccount();
+          << " account=" << requests_.front().account_id()
+          << " error=" << error.ToString();
+  const std::string account_id = requests_.front().account_id();
+  HandleNextRequest();
   SignalComplete(account_id, error);
 }
 
 void GaiaCookieManagerService::OnMergeSessionSuccess(const std::string& data) {
-  VLOG(1) << "MergeSession successful account=" << accounts_.front();
-  const std::string account_id = accounts_.front();
-  HandleNextAccount();
+  VLOG(1) << "MergeSession successful account="
+          << requests_.front().account_id();
+  const std::string account_id = requests_.front().account_id();
+  HandleNextRequest();
   SignalComplete(account_id, GoogleServiceAuthError::AuthErrorNone());
 }
 
 void GaiaCookieManagerService::OnMergeSessionFailure(
     const GoogleServiceAuthError& error) {
   VLOG(1) << "Failed MergeSession"
-          << " account=" << accounts_.front() << " error=" << error.ToString();
-  const std::string account_id = accounts_.front();
-  HandleNextAccount();
+          << " account=" << requests_.front().account_id()
+          << " error=" << error.ToString();
+  const std::string account_id = requests_.front().account_id();
+  HandleNextRequest();
   SignalComplete(account_id, error);
 }
 
-void GaiaCookieManagerService::StartFetching() {
+void GaiaCookieManagerService::StartFetchingUbertoken() {
   VLOG(1) << "GaiaCookieManagerService::StartFetching account_id="
-          << accounts_.front();
+          << requests_.front().account_id();
   uber_token_fetcher_.reset(
       new UbertokenFetcher(token_service_, this, source_,
                            signin_client_->GetURLRequestContext()));
-  uber_token_fetcher_->StartFetchingToken(accounts_.front());
+  uber_token_fetcher_->StartFetchingToken(requests_.front().account_id());
 }
 
 void GaiaCookieManagerService::OnURLFetchComplete(
     const net::URLFetcher* source) {
-  DCHECK(accounts_.front().empty());
+  DCHECK(requests_.front().request_type() == GaiaCookieRequestType::LOG_OUT);
   VLOG(1) << "GaiaCookieManagerService::OnURLFetchComplete";
-  HandleNextAccount();
+  HandleNextRequest();
 }
 
-void GaiaCookieManagerService::HandleNextAccount() {
-  VLOG(1) << "GaiaCookieManagerService::HandleNextAccount";
-  accounts_.pop_front();
+void GaiaCookieManagerService::HandleNextRequest() {
+  VLOG(1) << "GaiaCookieManagerService::HandleNextRequest";
+  requests_.pop_front();
   gaia_auth_fetcher_.reset();
-  if (accounts_.empty()) {
-    VLOG(1) << "GaiaCookieManagerService::HandleNextAccount: no more";
+  if (requests_.empty()) {
+    VLOG(1) << "GaiaCookieManagerService::HandleNextRequest: no more";
     uber_token_fetcher_.reset();
   } else {
-    if (accounts_.front().empty()) {
-      StartLogOutUrlFetch();
-    } else {
-      StartFetching();
-    }
+    switch (requests_.front().request_type()) {
+      case GaiaCookieRequestType::ADD_ACCOUNT:
+        StartFetchingUbertoken();
+        break;
+      case GaiaCookieRequestType::LOG_OUT:
+        StartLogOutUrlFetch();
+        break;
+      case GaiaCookieRequestType::LIST_ACCOUNTS:
+        break;
+    };
   }
 }