Commit 4b48896a authored by Tanmoy Mollik's avatar Tanmoy Mollik Committed by Chromium LUCI CQ

Convert components/signin/c/b/* code to OnPrimaryAccountChanged()

Methods OnPrimaryAccountSet/Cleared and
OnUnconsentedPrimaryAccountChanged() are deprecated. So this cl changes
these calls to OnPrimaryAccountChanged().

There is a behavior change introduced with this cl for
AboutSigninInternals. Observers are now notified when the unconsented
primary account changes as well as when sync account changes.

Bug: 1158855
Change-Id: I2c7f2f70dd1b6e79f4b12033306fecc160329edb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611276
Commit-Queue: Tanmoy Mollik <triploblastic@chromium.org>
Reviewed-by: default avatarMihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842004}
parent 940101c4
...@@ -462,13 +462,8 @@ void AboutSigninInternals::OnUnblockReconcile() { ...@@ -462,13 +462,8 @@ void AboutSigninInternals::OnUnblockReconcile() {
NotifyObservers(); NotifyObservers();
} }
void AboutSigninInternals::OnPrimaryAccountSet( void AboutSigninInternals::OnPrimaryAccountChanged(
const CoreAccountInfo& primary_account_info) { const signin::PrimaryAccountChangeEvent& event) {
NotifyObservers();
}
void AboutSigninInternals::OnPrimaryAccountCleared(
const CoreAccountInfo& primary_account_info) {
NotifyObservers(); NotifyObservers();
} }
......
...@@ -207,10 +207,8 @@ class AboutSigninInternals : public KeyedService, ...@@ -207,10 +207,8 @@ class AboutSigninInternals : public KeyedService,
// IdentityManager::Observer implementations. // IdentityManager::Observer implementations.
void OnRefreshTokensLoaded() override; void OnRefreshTokensLoaded() override;
void OnEndBatchOfRefreshTokenStateChanges() override; void OnEndBatchOfRefreshTokenStateChanges() override;
void OnPrimaryAccountSet( void OnPrimaryAccountChanged(
const CoreAccountInfo& primary_account_info) override; const signin::PrimaryAccountChangeEvent& event) override;
void OnPrimaryAccountCleared(
const CoreAccountInfo& primary_account_info) override;
void NotifyTimedSigninFieldValueChanged( void NotifyTimedSigninFieldValueChanged(
const signin_internals_util::TimedSigninStatusField& field, const signin_internals_util::TimedSigninStatusField& field,
......
...@@ -77,25 +77,8 @@ MirrorAccountReconcilorDelegate::GetChromeAccountsForReconcile( ...@@ -77,25 +77,8 @@ MirrorAccountReconcilorDelegate::GetChromeAccountsForReconcile(
gaia_accounts); gaia_accounts);
} }
// TODO(https://crbug.com/1046746): Replace separate IdentityManager::Observer void MirrorAccountReconcilorDelegate::OnPrimaryAccountChanged(
// method overrides below with a single OnPrimaryAccountChanged method and const PrimaryAccountChangeEvent& event) {
// inline UpdateReconcilorStatus.
void MirrorAccountReconcilorDelegate::OnPrimaryAccountSet(
const CoreAccountInfo& primary_account_info) {
UpdateReconcilorStatus();
}
void MirrorAccountReconcilorDelegate::OnPrimaryAccountCleared(
const CoreAccountInfo& previous_primary_account_info) {
UpdateReconcilorStatus();
}
void MirrorAccountReconcilorDelegate::OnUnconsentedPrimaryAccountChanged(
const CoreAccountInfo& unconsented_primary_account_info) {
UpdateReconcilorStatus();
}
void MirrorAccountReconcilorDelegate::UpdateReconcilorStatus() {
// Have to check whether the state has actually changed, as calling // Have to check whether the state has actually changed, as calling
// DisableReconcile logs out all accounts even if it was already disabled. // DisableReconcile logs out all accounts even if it was already disabled.
bool should_enable_reconcile = bool should_enable_reconcile =
......
...@@ -50,12 +50,7 @@ class MirrorAccountReconcilorDelegate : public AccountReconcilorDelegate, ...@@ -50,12 +50,7 @@ class MirrorAccountReconcilorDelegate : public AccountReconcilorDelegate,
const gaia::MultiloginMode mode) const override; const gaia::MultiloginMode mode) const override;
// IdentityManager::Observer: // IdentityManager::Observer:
void OnPrimaryAccountSet( void OnPrimaryAccountChanged(const PrimaryAccountChangeEvent& event) override;
const CoreAccountInfo& primary_account_info) override;
void OnPrimaryAccountCleared(
const CoreAccountInfo& previous_primary_account_info) override;
void OnUnconsentedPrimaryAccountChanged(
const CoreAccountInfo& unconsented_primary_account_info) override;
void UpdateReconcilorStatus(); void UpdateReconcilorStatus();
......
...@@ -149,17 +149,12 @@ void SigninErrorController::OnErrorStateOfRefreshTokenUpdatedForAccount( ...@@ -149,17 +149,12 @@ void SigninErrorController::OnErrorStateOfRefreshTokenUpdatedForAccount(
Update(); Update();
} }
void SigninErrorController::OnPrimaryAccountSet( void SigninErrorController::OnPrimaryAccountChanged(
const CoreAccountInfo& primary_account_info) { const signin::PrimaryAccountChangeEvent& event) {
// Ignore updates to the primary account if not in PRIMARY_ACCOUNT mode. if (event.GetEventTypeFor(signin::ConsentLevel::kSync) ==
if (account_mode_ != AccountMode::PRIMARY_ACCOUNT) signin::PrimaryAccountChangeEvent::Type::kNone) {
return; return;
}
Update();
}
void SigninErrorController::OnPrimaryAccountCleared(
const CoreAccountInfo& previous_primary_account_info) {
// Ignore updates to the primary account if not in PRIMARY_ACCOUNT mode. // Ignore updates to the primary account if not in PRIMARY_ACCOUNT mode.
if (account_mode_ != AccountMode::PRIMARY_ACCOUNT) if (account_mode_ != AccountMode::PRIMARY_ACCOUNT)
return; return;
......
...@@ -78,10 +78,8 @@ class SigninErrorController : public KeyedService, ...@@ -78,10 +78,8 @@ class SigninErrorController : public KeyedService,
void OnErrorStateOfRefreshTokenUpdatedForAccount( void OnErrorStateOfRefreshTokenUpdatedForAccount(
const CoreAccountInfo& account_info, const CoreAccountInfo& account_info,
const GoogleServiceAuthError& error) override; const GoogleServiceAuthError& error) override;
void OnPrimaryAccountSet( void OnPrimaryAccountChanged(
const CoreAccountInfo& primary_account_info) override; const signin::PrimaryAccountChangeEvent& event) override;
void OnPrimaryAccountCleared(
const CoreAccountInfo& previous_primary_account_info) override;
const AccountMode account_mode_; const AccountMode account_mode_;
signin::IdentityManager* identity_manager_; signin::IdentityManager* identity_manager_;
......
...@@ -72,26 +72,30 @@ void SigninStatusMetricsProvider::OnIdentityManagerShutdown( ...@@ -72,26 +72,30 @@ void SigninStatusMetricsProvider::OnIdentityManagerShutdown(
scoped_observations_.RemoveObservation(identity_manager); scoped_observations_.RemoveObservation(identity_manager);
} }
void SigninStatusMetricsProvider::OnPrimaryAccountSet( void SigninStatusMetricsProvider::OnPrimaryAccountChanged(
const CoreAccountInfo& account_info) { const signin::PrimaryAccountChangeEvent& event) {
SigninStatus recorded_signin_status = signin_status(); if (event.GetEventTypeFor(signin::ConsentLevel::kSync) ==
if (recorded_signin_status == ALL_PROFILES_NOT_SIGNED_IN) { signin::PrimaryAccountChangeEvent::Type::kNone) {
UpdateSigninStatus(MIXED_SIGNIN_STATUS); return;
} else if (recorded_signin_status == UNKNOWN_SIGNIN_STATUS) {
// There should have at least one browser opened if the user can sign in, so
// signin_status_ value should not be unknown.
UpdateSigninStatus(ERROR_GETTING_SIGNIN_STATUS);
} }
}
void SigninStatusMetricsProvider::OnPrimaryAccountCleared(
const CoreAccountInfo& account_info) {
SigninStatus recorded_signin_status = signin_status(); SigninStatus recorded_signin_status = signin_status();
if (recorded_signin_status == ALL_PROFILES_SIGNED_IN) { switch (event.GetEventTypeFor(signin::ConsentLevel::kSync)) {
case signin::PrimaryAccountChangeEvent::Type::kSet:
if (recorded_signin_status == ALL_PROFILES_NOT_SIGNED_IN)
UpdateSigninStatus(MIXED_SIGNIN_STATUS);
break;
case signin::PrimaryAccountChangeEvent::Type::kCleared:
if (recorded_signin_status == ALL_PROFILES_SIGNED_IN)
UpdateSigninStatus(MIXED_SIGNIN_STATUS); UpdateSigninStatus(MIXED_SIGNIN_STATUS);
} else if (recorded_signin_status == UNKNOWN_SIGNIN_STATUS) { break;
// There should have at least one browser opened if the user can sign out, case signin::PrimaryAccountChangeEvent::Type::kNone:
// so signin_status_ value should not be unknown. NOTREACHED() << "See return statement above";
break;
}
if (recorded_signin_status == UNKNOWN_SIGNIN_STATUS) {
// There should have at least one browser opened if the user can sign in or
// out, so signin_status_ value should not be unknown.
UpdateSigninStatus(ERROR_GETTING_SIGNIN_STATUS); UpdateSigninStatus(ERROR_GETTING_SIGNIN_STATUS);
} }
} }
......
...@@ -70,8 +70,8 @@ class SigninStatusMetricsProvider : public SigninStatusMetricsProviderBase, ...@@ -70,8 +70,8 @@ class SigninStatusMetricsProvider : public SigninStatusMetricsProviderBase,
bool is_test); bool is_test);
// IdentityManager::Observer: // IdentityManager::Observer:
void OnPrimaryAccountSet(const CoreAccountInfo& account_info) override; void OnPrimaryAccountChanged(
void OnPrimaryAccountCleared(const CoreAccountInfo& account_info) override; const signin::PrimaryAccountChangeEvent& event) override;
// Obtain sign-in status and add observers. // Obtain sign-in status and add observers.
void Initialize(); void Initialize();
......
...@@ -3,11 +3,22 @@ ...@@ -3,11 +3,22 @@
// found in the LICENSE file. // found in the LICENSE file.
#include "components/signin/core/browser/signin_status_metrics_provider.h" #include "components/signin/core/browser/signin_status_metrics_provider.h"
#include "components/signin/public/identity_manager/identity_test_utils.h"
#include <string> #include <string>
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
using signin::PrimaryAccountChangeEvent;
CoreAccountInfo TestAccount() {
CoreAccountInfo account;
account.email = "test@gmail.com";
account.gaia = signin::GetTestGaiaIdForEmail(account.email);
account.account_id = CoreAccountId::FromEmail(account.email);
return account;
}
TEST(SigninStatusMetricsProviderTest, UpdateInitialSigninStatus) { TEST(SigninStatusMetricsProviderTest, UpdateInitialSigninStatus) {
SigninStatusMetricsProvider metrics_provider(nullptr, true); SigninStatusMetricsProvider metrics_provider(nullptr, true);
...@@ -24,32 +35,46 @@ TEST(SigninStatusMetricsProviderTest, UpdateInitialSigninStatus) { ...@@ -24,32 +35,46 @@ TEST(SigninStatusMetricsProviderTest, UpdateInitialSigninStatus) {
TEST(SigninStatusMetricsProviderTest, OnPrimaryAccountSet) { TEST(SigninStatusMetricsProviderTest, OnPrimaryAccountSet) {
SigninStatusMetricsProvider metrics_provider(nullptr, true); SigninStatusMetricsProvider metrics_provider(nullptr, true);
CoreAccountInfo account_info = TestAccount();
// Initial status is all signed out and then one of the profiles is signed in. // Initial status is all signed out and then one of the profiles is signed in.
metrics_provider.UpdateInitialSigninStatus(2, 0); metrics_provider.UpdateInitialSigninStatus(2, 0);
metrics_provider.OnPrimaryAccountSet(AccountInfo()); metrics_provider.OnPrimaryAccountChanged(PrimaryAccountChangeEvent(
PrimaryAccountChangeEvent::State(),
PrimaryAccountChangeEvent::State(account_info,
signin::ConsentLevel::kSync)));
EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS, EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS,
metrics_provider.GetSigninStatusForTesting()); metrics_provider.GetSigninStatusForTesting());
// Initial status is mixed and then one of the profiles is signed in. // Initial status is mixed and then one of the profiles is signed in.
metrics_provider.UpdateInitialSigninStatus(2, 1); metrics_provider.UpdateInitialSigninStatus(2, 1);
metrics_provider.OnPrimaryAccountSet(AccountInfo()); metrics_provider.OnPrimaryAccountChanged(PrimaryAccountChangeEvent(
PrimaryAccountChangeEvent::State(),
PrimaryAccountChangeEvent::State(account_info,
signin::ConsentLevel::kSync)));
EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS, EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS,
metrics_provider.GetSigninStatusForTesting()); metrics_provider.GetSigninStatusForTesting());
} }
TEST(SigninStatusMetricsProviderTest, OnPrimaryAccountCleared) { TEST(SigninStatusMetricsProviderTest, OnPrimaryAccountCleared) {
SigninStatusMetricsProvider metrics_provider(nullptr, true); SigninStatusMetricsProvider metrics_provider(nullptr, true);
CoreAccountInfo account_info = TestAccount();
// Initial status is all signed in and then one of the profiles is signed out. // Initial status is all signed in and then one of the profiles is signed out.
metrics_provider.UpdateInitialSigninStatus(2, 2); metrics_provider.UpdateInitialSigninStatus(2, 2);
metrics_provider.OnPrimaryAccountCleared(AccountInfo()); metrics_provider.OnPrimaryAccountChanged(
PrimaryAccountChangeEvent(PrimaryAccountChangeEvent::State(
account_info, signin::ConsentLevel::kSync),
PrimaryAccountChangeEvent::State()));
EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS, EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS,
metrics_provider.GetSigninStatusForTesting()); metrics_provider.GetSigninStatusForTesting());
// Initial status is mixed and then one of the profiles is signed out. // Initial status is mixed and then one of the profiles is signed out.
metrics_provider.UpdateInitialSigninStatus(2, 1); metrics_provider.UpdateInitialSigninStatus(2, 1);
metrics_provider.OnPrimaryAccountCleared(AccountInfo()); metrics_provider.OnPrimaryAccountChanged(
PrimaryAccountChangeEvent(PrimaryAccountChangeEvent::State(
account_info, signin::ConsentLevel::kSync),
PrimaryAccountChangeEvent::State()));
EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS, EXPECT_EQ(SigninStatusMetricsProviderBase::MIXED_SIGNIN_STATUS,
metrics_provider.GetSigninStatusForTesting()); metrics_provider.GetSigninStatusForTesting());
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment