Skip to content

Commit

Permalink
[ios] Delete SyncSetupService
Browse files Browse the repository at this point in the history
If there are any behavior changes, they should only happen in code
paths where kReplaceSyncPromosWithSignInPromos is disabled, and
that code should be deleted soon anyway (the feature is launched).

There were 2 kinds of methods left in this service.
1) Wrappers around SyncSetupInProgressHandle: CommitSyncChanges(),
HasUncommittedChanges(), PrepareForFirstSyncSetup().
2) A wrapper around
SyncUserSettings::SetInitialSyncFeatureSetupComplete().

Calls to (1) can be further divided in 2 kinds:
a) Holding a SyncSetupInProgressHandle before doing
   SetSyncFeatureRequested() & SetInitialSyncFeatureSetupComplete() [1]
   -> Calls deleted in this CL. This might cause the sync engine to
   start a bit earlier but is believed to be harmless.
b) Holding a SyncSetupInProgressHandle while showing the data type
   toggles to syncing users -> Replaced with direct use of
   SyncSetupInProgressHandle.

Calls to (2) are replaced with direct use of SyncUserSettings. The
original wrapper had some extra guarding by CanSyncFeatureStart() [2].
I think that predicate is guaranteed to be true in all current callers
so I just removed the check. But as mentioned before, even if that's
not true, these code paths will be deleted soon anyway.

[1] Note: GrantSyncConsent() calls PrepareForFirstSyncSetup(), which
holds a SyncSetupInProgressHandle. CommitSyncChanges() releases the
handle.
https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_mediator.mm;l=271-277;drc=0dcaea514c649315fd66232a792c3ae74217ab2d
https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/ui/authentication/tangible_sync/tangible_sync_mediator.mm;l=251-260;drc=0dcaea514c649315fd66232a792c3ae74217ab2d

[2]
```
if (sync_service_->CanSyncFeatureStart()) {
    sync_service_->GetUserSettings()
                 ->SetInitialSyncFeatureSetupComplete(source);
}
```

Fixed: 40064196
Change-Id: I75bc2b680bfb8dba251a5a242e32387119133913
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5374774
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Victor Vianna <victorvianna@google.com>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1276801}
  • Loading branch information
Victor Hugo Vianna Silva authored and Chromium LUCI CQ committed Mar 22, 2024
1 parent 49a2e3b commit ba627ce
Show file tree
Hide file tree
Showing 31 changed files with 24 additions and 387 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@
#import "ios/chrome/browser/sync/model/ios_user_event_service_factory.h"
#import "ios/chrome/browser/sync/model/model_type_store_service_factory.h"
#import "ios/chrome/browser/sync/model/sync_service_factory.h"
#import "ios/chrome/browser/sync/model/sync_setup_service_factory.h"
#import "ios/chrome/browser/tabs_search/model/tabs_search_service_factory.h"
#import "ios/chrome/browser/text_selection/model/text_classifier_model_service_factory.h"
#import "ios/chrome/browser/translate/model/translate_ranker_factory.h"
Expand Down Expand Up @@ -212,7 +211,6 @@ void EnsureBrowserStateKeyedServiceFactoriesBuilt() {
SupervisedUserServiceFactory::GetInstance();
SupervisedUserSettingsServiceFactory::GetInstance();
SyncServiceFactory::GetInstance();
SyncSetupServiceFactory::GetInstance();
TabsSearchServiceFactory::GetInstance();
TextClassifierModelServiceFactory::GetInstance();
TextToSpeechPlaybackControllerFactory::GetInstance();
Expand Down
3 changes: 0 additions & 3 deletions ios/chrome/browser/signin/model/authentication_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class AuthenticationServiceDelegate;
class AuthenticationServiceObserver;
class FakeAuthenticationService;
class PrefService;
class SyncSetupService;
@protocol RefreshAccessTokenError;
@protocol SystemIdentity;

Expand All @@ -53,7 +52,6 @@ class AuthenticationService : public KeyedService,

// Initializes the service.
AuthenticationService(PrefService* pref_service,
SyncSetupService* sync_setup_service,
ChromeAccountManagerService* account_manager_service,
signin::IdentityManager* identity_manager,
syncer::SyncService* sync_service);
Expand Down Expand Up @@ -228,7 +226,6 @@ class AuthenticationService : public KeyedService,

// Pointer to the KeyedServices used by AuthenticationService.
raw_ptr<PrefService> pref_service_ = nullptr;
raw_ptr<SyncSetupService> sync_setup_service_ = nullptr;
raw_ptr<ChromeAccountManagerService> account_manager_service_ = nullptr;
raw_ptr<signin::IdentityManager> identity_manager_ = nullptr;
raw_ptr<syncer::SyncService> sync_service_ = nullptr;
Expand Down
10 changes: 0 additions & 10 deletions ios/chrome/browser/signin/model/authentication_service.mm
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
#import "ios/chrome/browser/signin/model/signin_util.h"
#import "ios/chrome/browser/signin/model/system_identity.h"
#import "ios/chrome/browser/signin/model/system_identity_manager.h"
#import "ios/chrome/browser/sync/model/sync_setup_service.h"
#import "ios/chrome/browser/ui/authentication/signin/signin_utils.h"

namespace {
Expand Down Expand Up @@ -67,18 +66,15 @@ CoreAccountId SystemIdentityToAccountID(

AuthenticationService::AuthenticationService(
PrefService* pref_service,
SyncSetupService* sync_setup_service,
ChromeAccountManagerService* account_manager_service,
signin::IdentityManager* identity_manager,
syncer::SyncService* sync_service)
: pref_service_(pref_service),
sync_setup_service_(sync_setup_service),
account_manager_service_(account_manager_service),
identity_manager_(identity_manager),
sync_service_(sync_service),
weak_pointer_factory_(this) {
DCHECK(pref_service_);
DCHECK(sync_setup_service_);
DCHECK(identity_manager_);
DCHECK(sync_service_);
}
Expand Down Expand Up @@ -379,12 +375,6 @@ CoreAccountId SystemIdentityToAccountID(
CHECK_EQ(account_id,
identity_manager_->GetPrimaryAccountId(signin::ConsentLevel::kSync));

// Sets the Sync setup handle to prepare for configuring the Sync data types
// before Sync-the-feature actually starts.
// TODO(crbug.com/1206680): Add EarlGrey tests to ensure that the Sync feature
// only starts after GrantSyncConsent is called.
sync_setup_service_->PrepareForFirstSyncSetup();

// Kick-off sync: The authentication error UI (sign in infobar and warning
// badge in settings screen) check the sync auth error state. Sync
// needs to be kicked off so that it resets the auth error quickly once
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#import "ios/chrome/browser/signin/model/chrome_account_manager_service_factory.h"
#import "ios/chrome/browser/signin/model/identity_manager_factory.h"
#import "ios/chrome/browser/sync/model/sync_service_factory.h"
#import "ios/chrome/browser/sync/model/sync_setup_service_factory.h"

namespace {

Expand All @@ -25,7 +24,6 @@
ChromeBrowserState::FromBrowserState(context);
return std::make_unique<AuthenticationService>(
browser_state->GetPrefs(),
SyncSetupServiceFactory::GetForBrowserState(browser_state),
ChromeAccountManagerServiceFactory::GetForBrowserState(browser_state),
IdentityManagerFactory::GetForBrowserState(browser_state),
SyncServiceFactory::GetForBrowserState(browser_state));
Expand Down Expand Up @@ -70,7 +68,6 @@
BrowserStateDependencyManager::GetInstance()) {
DependsOn(ChromeAccountManagerServiceFactory::GetInstance());
DependsOn(IdentityManagerFactory::GetInstance());
DependsOn(SyncSetupServiceFactory::GetInstance());
DependsOn(SyncServiceFactory::GetInstance());
}

Expand Down
37 changes: 0 additions & 37 deletions ios/chrome/browser/signin/model/authentication_service_unittest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@
#import "ios/chrome/browser/signin/model/system_identity.h"
#import "ios/chrome/browser/sync/model/mock_sync_service_utils.h"
#import "ios/chrome/browser/sync/model/sync_service_factory.h"
#import "ios/chrome/browser/sync/model/sync_setup_service_factory.h"
#import "ios/chrome/browser/sync/model/sync_setup_service_mock.h"
#import "ios/chrome/test/testing_application_context.h"
#import "ios/web/public/test/web_task_environment.h"
#import "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -106,9 +104,6 @@ int GetOnServiceStatusChangedCounter() {
builder.SetPrefService(CreatePrefService());
builder.AddTestingFactory(SyncServiceFactory::GetInstance(),
base::BindRepeating(&CreateMockSyncService));
builder.AddTestingFactory(
SyncSetupServiceFactory::GetInstance(),
base::BindRepeating(&SyncSetupServiceMock::CreateKeyedService));
builder.AddTestingFactory(
AuthenticationServiceFactory::GetInstance(),
AuthenticationServiceFactory::GetDefaultFactory());
Expand All @@ -133,14 +128,6 @@ int GetOnServiceStatusChangedCounter() {
return prefs;
}

void SetExpectationsForSignIn() {
EXPECT_CALL(*sync_setup_service_mock(), PrepareForFirstSyncSetup).Times(0);
}

void SetExpectationsForSignInAndSync() {
EXPECT_CALL(*sync_setup_service_mock(), PrepareForFirstSyncSetup).Times(1);
}

void VerifyLastSigninTimestamp() {
EXPECT_EQ(
browser_state_.get()->GetPrefs()->GetTime(prefs::kLastSigninTimestamp),
Expand Down Expand Up @@ -225,11 +212,6 @@ int ClearBrowsingDataCount() {
SyncServiceFactory::GetForBrowserState(browser_state_.get()));
}

SyncSetupServiceMock* sync_setup_service_mock() {
return static_cast<SyncSetupServiceMock*>(
SyncSetupServiceFactory::GetForBrowserState(browser_state_.get()));
}

id<SystemIdentity> identity(NSUInteger index) {
return [account_manager_->GetAllIdentities() objectAtIndex:index];
}
Expand Down Expand Up @@ -262,7 +244,6 @@ void SetPattern(const std::string& pattern) {

TEST_F(AuthenticationServiceTest, TestSignInAndGetPrimaryIdentity) {
// Sign in.
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_SIGNIN_PROMO);
VerifyLastSigninTimestamp();
Expand Down Expand Up @@ -298,7 +279,6 @@ void SetPattern(const std::string& pattern) {
// Tests that reauth prompt is not set when the user signs out.
TEST_F(AuthenticationServiceTest, TestHandleForgottenIdentityNoPromptSignIn) {
// Sign in.
SetExpectationsForSignInAndSync();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
authentication_service()->GrantSyncConsent(
Expand All @@ -323,7 +303,6 @@ void SetPattern(const std::string& pattern) {
// an other app when the user was signed and syncing.
TEST_F(AuthenticationServiceTest, TestHandleForgottenIdentityPromptSignIn) {
// Sign in.
SetExpectationsForSignInAndSync();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
authentication_service()->GrantSyncConsent(
Expand Down Expand Up @@ -352,7 +331,6 @@ void SetPattern(const std::string& pattern) {
syncer::kReplaceSyncPromosWithSignInPromos);

// Sign in.
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
VerifyLastSigninTimestamp();
Expand Down Expand Up @@ -381,7 +359,6 @@ void SetPattern(const std::string& pattern) {
syncer::kReplaceSyncPromosWithSignInPromos);

// Sign in.
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
VerifyLastSigninTimestamp();
Expand All @@ -401,7 +378,6 @@ void SetPattern(const std::string& pattern) {
TEST_F(AuthenticationServiceTest,
OnApplicationEnterForegroundReloadCredentials) {
// Sign in.
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
VerifyLastSigninTimestamp();
Expand Down Expand Up @@ -436,7 +412,6 @@ void SetPattern(const std::string& pattern) {

TEST_F(AuthenticationServiceTest, HasPrimaryIdentityBackground) {
// Sign in.
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_TRUE(authentication_service()->HasPrimaryIdentity(
Expand All @@ -456,7 +431,6 @@ void SetPattern(const std::string& pattern) {
// Tests that MDM errors are correctly cleared on foregrounding, sending
// notifications that the state of error has changed.
TEST_F(AuthenticationServiceTest, MDMErrorsClearedOnForeground) {
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 2UL);
Expand Down Expand Up @@ -498,7 +472,6 @@ GoogleServiceAuthError error(

// Tests that MDM errors are correctly cleared when signing out.
TEST_F(AuthenticationServiceTest, MDMErrorsClearedOnSignout) {
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 2UL);
Expand All @@ -517,7 +490,6 @@ GoogleServiceAuthError error(
// browsing data.
TEST_F(AuthenticationServiceTest,
MDMErrorsClearedOnSignoutAndClearBrowsingData) {
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 2UL);
Expand All @@ -537,7 +509,6 @@ GoogleServiceAuthError error(
TEST_F(AuthenticationServiceTest, SignedInManagedAccountSignOut) {
fake_system_identity_manager()->AddManagedIdentities(@[ @"foo3" ]);

SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(2), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 3UL);
Expand All @@ -558,7 +529,6 @@ GoogleServiceAuthError error(
//
// Regression test for root cause of crbug/1482236
TEST_F(AuthenticationServiceTest, MDMErrorsDontSeedEmptyAccountIds) {
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 2UL);
Expand Down Expand Up @@ -588,7 +558,6 @@ GoogleServiceAuthError error(
TEST_F(AuthenticationServiceTest, ManagedAccountSignOut) {
fake_system_identity_manager()->AddManagedIdentities(@[ @"foo3" ]);

SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(2), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
EXPECT_EQ(identity_manager()->GetAccountsWithRefreshTokens().size(), 3UL);
Expand All @@ -613,7 +582,6 @@ GoogleServiceAuthError error(
TEST_F(AuthenticationServiceTest, ManagedAccountSignOutAndClearBrowsingData) {
fake_system_identity_manager()->AddManagedIdentities(@[ @"foo3" ]);

SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(2), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
VerifyLastSigninTimestamp();
Expand All @@ -635,7 +603,6 @@ GoogleServiceAuthError error(
// Tests that potential MDM notifications are correctly handled and dispatched
// to MDM service when necessary.
TEST_F(AuthenticationServiceTest, HandleMDMNotification) {
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
VerifyLastSigninTimestamp();
Expand Down Expand Up @@ -675,7 +642,6 @@ GoogleServiceAuthError error(
// Tests that MDM blocked notifications are correctly signing out the user if
// the primary account is blocked.
TEST_F(AuthenticationServiceTest, HandleMDMBlockedNotification) {
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
GoogleServiceAuthError error(
Expand Down Expand Up @@ -747,7 +713,6 @@ GoogleServiceAuthError error(
// phase 3. See ConsentLevel::kSync documentation for details.
TEST_F(AuthenticationServiceTest, SigninAndSyncDecoupled) {
// Sign in.
SetExpectationsForSignIn();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
VerifyLastSigninTimestamp();
Expand All @@ -762,7 +727,6 @@ GoogleServiceAuthError error(
signin::ConsentLevel::kSignin));

// Grant Sync consent.
EXPECT_CALL(*sync_setup_service_mock(), PrepareForFirstSyncSetup).Times(1);
EXPECT_CALL(*mock_sync_service(), SetSyncFeatureRequested());
authentication_service()->GrantSyncConsent(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
Expand Down Expand Up @@ -792,7 +756,6 @@ GoogleServiceAuthError error(
AuthenticationServiceObserverTest observer_test;
authentication_service()->AddObserver(&observer_test);
// Sign in.
SetExpectationsForSignInAndSync();
authentication_service()->SignIn(
identity(0), signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN);
authentication_service()->GrantSyncConsent(
Expand Down
6 changes: 0 additions & 6 deletions ios/chrome/browser/sync/model/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ source_set("model") {
"sync_observer_bridge.mm",
"sync_service_factory.h",
"sync_service_factory.mm",
"sync_setup_service.cc",
"sync_setup_service.h",
"sync_setup_service_factory.cc",
"sync_setup_service_factory.h",
]
deps = [
":model_type_store_service_factory",
Expand Down Expand Up @@ -142,8 +138,6 @@ source_set("test_support") {
sources = [
"mock_sync_service_utils.cc",
"mock_sync_service_utils.h",
"sync_setup_service_mock.cc",
"sync_setup_service_mock.h",
]
deps = [
":model",
Expand Down
47 changes: 0 additions & 47 deletions ios/chrome/browser/sync/model/sync_setup_service.cc

This file was deleted.

0 comments on commit ba627ce

Please sign in to comment.