Adding a step to the check-in process that ensures the correct account information is present before device

check-in happen, in order to maintain a relationship
between signed in accounts and the device.

Behavior of the check-in is not symmetric:
* Adding an account converges slowly - newly added
account will be associated to device with next periodic
check, to avoid checking in too often.
* Removing account triggers check-in immediately to ensure
users privacy.


BUG=374969

Review URL: https://codereview.chromium.org/378643002

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

4 files changed, 71 insertions, 33 deletions

diff --git a/chrome/browser/services/gcm/gcm_account_tracker.cc b/chrome/browser/services/gcm/gcm_account_tracker.cc
index 00bb150..d2636ad 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker.cc
+++ b/chrome/browser/services/gcm/gcm_account_tracker.cc
@@ -13,8 +13,7 @@
 namespace gcm {
 
 namespace {
-const char kGCMGroupServerScope[] =
-    "oauth2:https://www.googleapis.com/auth/gcm";
+const char kGCMGroupServerScope[] = "https://www.googleapis.com/auth/gcm";
 const char kGCMAccountTrackerName[] = "gcm_account_tracker";
 }  // namespace
 
@@ -75,10 +74,12 @@ void GCMAccountTracker::Stop() {
 }
 
 void GCMAccountTracker::OnAccountAdded(const gaia::AccountIds& ids) {
+  DVLOG(1) << "Account added: " << ids.email;
   // We listen for the account signing in, which happens after account is added.
 }
 
 void GCMAccountTracker::OnAccountRemoved(const gaia::AccountIds& ids) {
+  DVLOG(1) << "Account removed: " << ids.email;
   // We listen for the account signing out, which happens before account is
   // removed.
 }
@@ -97,6 +98,7 @@ void GCMAccountTracker::OnGetTokenSuccess(
     const base::Time& expiration_time) {
   DCHECK(request);
   DCHECK(!request->GetAccountId().empty());
+  DVLOG(1) << "Get token success: " << request->GetAccountId();
 
   AccountInfos::iterator iter = account_infos_.find(request->GetAccountId());
   DCHECK(iter != account_infos_.end());
@@ -120,6 +122,7 @@ void GCMAccountTracker::OnGetTokenFailure(
     const GoogleServiceAuthError& error) {
   DCHECK(request);
   DCHECK(!request->GetAccountId().empty());
+  DVLOG(1) << "Get token failure: " << request->GetAccountId();
 
   AccountInfos::iterator iter = account_infos_.find(request->GetAccountId());
   DCHECK(iter != account_infos_.end());
@@ -181,7 +184,13 @@ void GCMAccountTracker::CompleteCollectingTokens() {
     }
   }
 
-  callback_.Run(account_tokens, account_removed);
+  // Make sure that there is something to report, otherwise bail out.
+  if (!account_tokens.empty() || account_removed) {
+    DVLOG(1) << "Calling callback: " << account_tokens.size();
+    callback_.Run(account_tokens);
+  } else {
+    DVLOG(1) << "No tokens and nothing removed. Skipping callback.";
+  }
 }
 
 void GCMAccountTracker::DeleteTokenRequest(
@@ -215,6 +224,7 @@ void GCMAccountTracker::GetToken(AccountInfos::iterator& account_iter) {
 }
 
 void GCMAccountTracker::OnAccountSignedIn(const gaia::AccountIds& ids) {
+  DVLOG(1) << "Account signed in: " << ids.email;
   AccountInfos::iterator iter = account_infos_.find(ids.account_key);
   if (iter == account_infos_.end()) {
     DCHECK(!ids.email.empty());
@@ -228,6 +238,7 @@ void GCMAccountTracker::OnAccountSignedIn(const gaia::AccountIds& ids) {
 }
 
 void GCMAccountTracker::OnAccountSignedOut(const gaia::AccountIds& ids) {
+  DVLOG(1) << "Account signed out: " << ids.email;
   AccountInfos::iterator iter = account_infos_.find(ids.account_key);
   if (iter == account_infos_.end())
     return;
diff --git a/chrome/browser/services/gcm/gcm_account_tracker.h b/chrome/browser/services/gcm/gcm_account_tracker.h
index 9093ba7..9da40a6 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker.h
+++ b/chrome/browser/services/gcm/gcm_account_tracker.h
@@ -50,11 +50,9 @@ class GCMAccountTracker : public gaia::AccountTracker::Observer,
   };
 
   // Callback for the GetAccountsForCheckin call. |account_tokens| maps email
-  // addresses to OAuth2 access tokens. |account_removed| indicates whether an
-  // account has been removed since the last time the callback was called.
-  typedef base::Callback<
-      void(const std::map<std::string, std::string>& account_tokens,
-           bool account_removed)> UpdateAccountsCallback;
+  // addresses to OAuth2 access tokens.
+  typedef base::Callback<void(const std::map<std::string, std::string>&
+                                  account_tokens)> UpdateAccountsCallback;
 
   // Creates an instance of GCMAccountTracker. |account_tracker| is used to
   // deliver information about the account, while |callback| will be called
diff --git a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
index 24c3ab3..cd9deb5 100644
--- a/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
+++ b/chrome/browser/services/gcm/gcm_account_tracker_unittest.cc
@@ -45,8 +45,7 @@ class GCMAccountTrackerTest : public testing::Test {
   virtual ~GCMAccountTrackerTest();
 
   // Callback for the account tracker.
-  void UpdateAccounts(const std::map<std::string, std::string>& accounts,
-                      bool account_removed);
+  void UpdateAccounts(const std::map<std::string, std::string>& accounts);
 
   // Helpers to pass fake events to the tracker. Tests should have either a pair
   // of Start/FinishAccountSignIn or SignInAccount per account. Don't mix.
@@ -63,7 +62,6 @@ class GCMAccountTrackerTest : public testing::Test {
   // Test results and helpers.
   void ResetResults();
   bool update_accounts_called() const { return update_accounts_called_; }
-  bool account_removed() const { return account_removed_; }
   const std::map<std::string, std::string>& accounts() const {
     return accounts_;
   }
@@ -74,7 +72,6 @@ class GCMAccountTrackerTest : public testing::Test {
  private:
   std::map<std::string, std::string> accounts_;
   bool update_accounts_called_;
-  bool account_removed_;
 
   base::MessageLoop message_loop_;
   net::TestURLFetcherFactory test_fetcher_factory_;
@@ -84,7 +81,7 @@ class GCMAccountTrackerTest : public testing::Test {
 };
 
 GCMAccountTrackerTest::GCMAccountTrackerTest()
-    : update_accounts_called_(false), account_removed_(false) {
+    : update_accounts_called_(false) {
   fake_token_service_.reset(new FakeOAuth2TokenService());
 
   fake_identity_provider_.reset(
@@ -107,17 +104,14 @@ GCMAccountTrackerTest::~GCMAccountTrackerTest() {
 }
 
 void GCMAccountTrackerTest::UpdateAccounts(
-    const std::map<std::string, std::string>& accounts,
-    bool account_removed) {
+    const std::map<std::string, std::string>& accounts) {
   update_accounts_called_ = true;
   accounts_ = accounts;
-  account_removed_ = account_removed;
 }
 
 void GCMAccountTrackerTest::ResetResults() {
   accounts_.clear();
   update_accounts_called_ = false;
-  account_removed_ = false;
 }
 
 void GCMAccountTrackerTest::StartAccountSignIn(const std::string& account_key) {
@@ -159,10 +153,9 @@ void GCMAccountTrackerTest::IssueError(const std::string& account_key) {
 
 TEST_F(GCMAccountTrackerTest, NoAccounts) {
   EXPECT_FALSE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
   tracker()->Start();
-  EXPECT_TRUE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
+  // Callback should not be called if there where no accounts provided.
+  EXPECT_FALSE(update_accounts_called());
   EXPECT_TRUE(accounts().empty());
   tracker()->Stop();
 }
@@ -183,7 +176,6 @@ TEST_F(GCMAccountTrackerTest, SingleAccount) {
   IssueAccessToken(kAccountId1);
 
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
@@ -201,12 +193,10 @@ TEST_F(GCMAccountTrackerTest, MultipleAccounts) {
   FinishAccountSignIn(kAccountId1);
   IssueAccessToken(kAccountId1);
   EXPECT_FALSE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
 
   FinishAccountSignIn(kAccountId2);
   IssueAccessToken(kAccountId2);
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
@@ -225,7 +215,6 @@ TEST_F(GCMAccountTrackerTest, AccountAdded) {
 
   IssueAccessToken(kAccountId1);
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
@@ -248,7 +237,6 @@ TEST_F(GCMAccountTrackerTest, AccountRemoved) {
 
   SignOutAccount(kAccountId2);
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_TRUE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
@@ -267,7 +255,6 @@ TEST_F(GCMAccountTrackerTest, GetTokenFailed) {
 
   IssueError(kAccountId2);
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_FALSE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
@@ -287,7 +274,6 @@ TEST_F(GCMAccountTrackerTest, GetTokenFailedAccountRemoved) {
   ResetResults();
   SignOutAccount(kAccountId2);
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_TRUE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
@@ -307,7 +293,6 @@ TEST_F(GCMAccountTrackerTest, AccountRemovedWhileRequestsPending) {
   SignOutAccount(kAccountId2);
   IssueAccessToken(kAccountId2);
   EXPECT_TRUE(update_accounts_called());
-  EXPECT_TRUE(account_removed());
 
   std::map<std::string, std::string> expected_accounts;
   expected_accounts[kAccountId1] = MakeAccessToken(kAccountId1);
diff --git a/chrome/browser/services/gcm/gcm_profile_service.cc b/chrome/browser/services/gcm/gcm_profile_service.cc
index 3e549a7..4b90897 100644
--- a/chrome/browser/services/gcm/gcm_profile_service.cc
+++ b/chrome/browser/services/gcm/gcm_profile_service.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/services/gcm/gcm_profile_service.h"
 
+#include <map>
+
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -13,7 +15,10 @@
 #if defined(OS_ANDROID)
 #include "components/gcm_driver/gcm_driver_android.h"
 #else
+#include "base/bind.h"
 #include "base/files/file_path.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/services/gcm/gcm_account_tracker.h"
 #include "chrome/browser/services/gcm/gcm_desktop_utils.h"
 #include "chrome/browser/signin/profile_identity_provider.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
@@ -21,7 +26,9 @@
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/chrome_constants.h"
 #include "components/gcm_driver/gcm_client_factory.h"
+#include "components/gcm_driver/gcm_driver_desktop.h"
 #include "components/signin/core/browser/signin_manager.h"
+#include "google_apis/gaia/account_tracker.h"
 #include "google_apis/gaia/identity_provider.h"
 #include "net/url_request/url_request_context_getter.h"
 #endif
@@ -29,9 +36,11 @@
 namespace gcm {
 
 #if !defined(OS_ANDROID)
+// Identity observer only has actual work to do when the user is actually signed
+// in. It ensures that account tracker is taking
 class GCMProfileService::IdentityObserver : public IdentityProvider::Observer {
  public:
-  IdentityObserver(Profile* profile, GCMDriver* driver);
+  IdentityObserver(Profile* profile, GCMDriverDesktop* driver);
   virtual ~IdentityObserver();
 
   // IdentityProvider::Observer:
@@ -40,20 +49,29 @@ class GCMProfileService::IdentityObserver : public IdentityProvider::Observer {
 
   std::string SignedInUserName() const;
 
+  // Called to inform IdentityObserver that a list of accounts was updated.
+  // |account_tokens| maps email addresses to OAuth2 access tokens.
+  void AccountsUpdated(
+      const std::map<std::string, std::string>& account_tokens);
+
  private:
-  GCMDriver* driver_;
+  Profile* profile_;
+  GCMDriverDesktop* driver_;
   scoped_ptr<IdentityProvider> identity_provider_;
+  scoped_ptr<GCMAccountTracker> gcm_account_tracker_;
 
   // The account ID that this service is responsible for. Empty when the service
   // is not running.
   std::string account_id_;
 
+  base::WeakPtrFactory<GCMProfileService::IdentityObserver> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(IdentityObserver);
 };
 
 GCMProfileService::IdentityObserver::IdentityObserver(Profile* profile,
-                                                      GCMDriver* driver)
-    : driver_(driver) {
+                                                      GCMDriverDesktop* driver)
+    : profile_(profile), driver_(driver), weak_ptr_factory_(this) {
   identity_provider_.reset(new ProfileIdentityProvider(
       SigninManagerFactory::GetForProfile(profile),
       ProfileOAuth2TokenServiceFactory::GetForProfile(profile),
@@ -64,6 +82,8 @@ GCMProfileService::IdentityObserver::IdentityObserver(Profile* profile,
 }
 
 GCMProfileService::IdentityObserver::~IdentityObserver() {
+  if (gcm_account_tracker_)
+    gcm_account_tracker_->Shutdown();
   identity_provider_->RemoveObserver(this);
 }
 
@@ -75,15 +95,38 @@ void GCMProfileService::IdentityObserver::OnActiveAccountLogin() {
   account_id_ = account_id;
 
   driver_->OnSignedIn();
+
+  if (!gcm_account_tracker_) {
+    scoped_ptr<gaia::AccountTracker> gaia_account_tracker(
+        new gaia::AccountTracker(identity_provider_.get(),
+                                 profile_->GetRequestContext()));
+
+    gcm_account_tracker_.reset(new GCMAccountTracker(
+        gaia_account_tracker.Pass(),
+        base::Bind(&GCMProfileService::IdentityObserver::AccountsUpdated,
+                   weak_ptr_factory_.GetWeakPtr())));
+  }
+
+  gcm_account_tracker_->Start();
 }
 
 void GCMProfileService::IdentityObserver::OnActiveAccountLogout() {
+  // Check is necessary to not crash browser_tests.
+  if (gcm_account_tracker_)
+    gcm_account_tracker_->Stop();
+  // TODO(fgorski): If we purge here, what should happen when we get
+  // OnActiveAccountLogin() right after that?
   driver_->Purge();
 }
 
 std::string GCMProfileService::IdentityObserver::SignedInUserName() const {
   return driver_->IsStarted() ? account_id_ : std::string();
 }
+
+void GCMProfileService::IdentityObserver::AccountsUpdated(
+    const std::map<std::string, std::string>& account_tokens) {
+  driver_->SetAccountsForCheckin(account_tokens);
+}
 #endif  // !defined(OS_ANDROID)
 
 // static
@@ -122,7 +165,8 @@ GCMProfileService::GCMProfileService(
       profile_->GetPath().Append(chrome::kGCMStoreDirname),
       profile_->GetRequestContext());
 
-  identity_observer_.reset(new IdentityObserver(profile, driver_.get()));
+  identity_observer_.reset(new IdentityObserver(
+      profile, static_cast<gcm::GCMDriverDesktop*>(driver_.get())));
 }
 #endif  // defined(OS_ANDROID)