Skip to content

Commit

Permalink
Revert "[UPM] Report sync status to the CredentialManager for signed …
Browse files Browse the repository at this point in the history
…out users"

This reverts commit 48876b4.

Reason for revert: Possibly causes crash go/paste/6166598166511616

Original change's description:
> [UPM] Report sync status to the CredentialManager for signed out users
>
> This CL adds calls to CredentialManager setCurrentAutofillAccount()
> for non signed-in users with UPM flag set to enabled. For these users
> sync service did not reported status changes after PasswordManager
> initialisation which resulted in not informing CredentialManager about
> the initial sync status. After this CL, CredentialManager will always
> be informed on Chrome startup. Sync status should not change after the
> sync service is initialised, so for signed in users there is still
> only one call but it's performed earlier.
>
> This should not change any user-visible behaviour and only affects
> metrics collected on the GMS ChromeSync side.
>
> (cherry picked from commit 0671c87)
>
> Bug: 1402964
> Change-Id: Ia3618cf007eb9987ea431b4bbd236036cf59800b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4120437
> Reviewed-by: Maria Kazinova <kazinova@google.com>
> Commit-Queue: Maxim Anufriev <maxan@google.com>
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Original-Commit-Position: refs/heads/main@{#1086367}
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4122349
> Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
> Auto-Submit: Maxim Anufriev <maxan@google.com>
> Cr-Commit-Position: refs/branch-heads/5481@{#56}
> Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

Bug: 1402964
Change-Id: Idf8b13f6b02c55e2784db55b46a2b8ad5e8da518
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4122226
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#60}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Maxim Anufriev authored and Chromium LUCI CQ committed Dec 24, 2022
1 parent 2e7bcd2 commit 7d79977
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 65 deletions.
Expand Up @@ -55,8 +55,7 @@ void PasswordSyncControllerDelegateAndroid::OnSyncServiceInitialized(
syncer::SyncService* sync_service) {
sync_service_ = sync_service;
sync_observation_.Observe(sync_service);
is_sync_enabled_ = IsSyncEnabled(IsPasswordSyncEnabled(sync_service));
UpdateCredentialManagerSyncStatus(is_sync_enabled_.value());
is_sync_enabled_ = IsSyncEnabled(IsPasswordSyncEnabled(sync_service_));
}

void PasswordSyncControllerDelegateAndroid::OnSyncStarting(
Expand Down Expand Up @@ -131,7 +130,20 @@ void PasswordSyncControllerDelegateAndroid::

void PasswordSyncControllerDelegateAndroid::OnStateChanged(
syncer::SyncService* sync) {
UpdateCredentialManagerSyncStatus(IsSyncEnabled(IsPasswordSyncEnabled(sync)));
// Notify credential manager about current account on startup or if
// password sync setting has changed.
if (sync_util::IsPasswordSyncEnabled(sync) &&
(!credential_manager_sync_setting_.has_value() ||
credential_manager_sync_setting_ == IsSyncEnabled(false))) {
bridge_->NotifyCredentialManagerWhenSyncing();
credential_manager_sync_setting_ = IsSyncEnabled(true);
}
if (!sync_util::IsPasswordSyncEnabled(sync) &&
(!credential_manager_sync_setting_.has_value() ||
credential_manager_sync_setting_ == IsSyncEnabled(true))) {
bridge_->NotifyCredentialManagerWhenNotSyncing();
credential_manager_sync_setting_ = IsSyncEnabled(false);
}
}

void PasswordSyncControllerDelegateAndroid::OnSyncShutdown(
Expand Down Expand Up @@ -161,21 +173,6 @@ void PasswordSyncControllerDelegateAndroid::OnCredentialManagerError(
}
}

void PasswordSyncControllerDelegateAndroid::UpdateCredentialManagerSyncStatus(
IsSyncEnabled is_enabled) {
if (credential_manager_sync_setting_.has_value() &&
credential_manager_sync_setting_ == is_enabled) {
return;
}

credential_manager_sync_setting_ = is_enabled;
if (is_enabled) {
bridge_->NotifyCredentialManagerWhenSyncing();
} else {
bridge_->NotifyCredentialManagerWhenNotSyncing();
}
}

base::WeakPtr<syncer::ModelTypeControllerDelegate>
PasswordSyncControllerDelegateAndroid::GetWeakPtrToBaseClass() {
return weak_ptr_factory_.GetWeakPtr();
Expand Down
Expand Up @@ -72,10 +72,6 @@ class PasswordSyncControllerDelegateAndroid
private:
using IsSyncEnabled = base::StrongAlias<struct IsSyncEnabledTag, bool>;

// Notify credential manager about current account on startup or if
// password sync setting has changed.
void UpdateCredentialManagerSyncStatus(IsSyncEnabled is_enabled);

base::WeakPtr<syncer::ModelTypeControllerDelegate> GetWeakPtrToBaseClass();

const std::unique_ptr<PasswordSyncControllerDelegateBridge> bridge_;
Expand Down
Expand Up @@ -45,7 +45,7 @@ class PasswordSyncControllerDelegateAndroidTest : public testing::Test {
void RunUntilIdle() { task_environment_.RunUntilIdle(); }

MockPasswordSyncControllerDelegateBridge* bridge() { return bridge_; }
syncer::TestSyncService* sync_service() { return &sync_service_; }
syncer::SyncService* sync_service() { return &sync_service_; }
PasswordSyncControllerDelegateAndroid* sync_controller_delegate() {
return sync_controller_delegate_.get();
}
Expand All @@ -69,55 +69,16 @@ class PasswordSyncControllerDelegateAndroidTest : public testing::Test {
raw_ptr<StrictMock<MockPasswordSyncControllerDelegateBridge>> bridge_;
};

TEST_F(PasswordSyncControllerDelegateAndroidTest,
OnSyncStatusEnabledOnStartup) {
EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenSyncing);
sync_controller_delegate()->OnSyncServiceInitialized(sync_service());
testing::Mock::VerifyAndClearExpectations(bridge());

// Check that observing the same event again will not trigger another
// notification.
sync_controller_delegate()->OnStateChanged(sync_service());
}

TEST_F(PasswordSyncControllerDelegateAndroidTest,
OnSyncStatusEnabledWithoutPasswordsOnStartup) {
sync_service()->GetUserSettings()->SetSelectedTypes(/*sync_everything=*/false,
/*types=*/{});

EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenNotSyncing);
sync_controller_delegate()->OnStateChanged(sync_service());
testing::Mock::VerifyAndClearExpectations(bridge());

// Check that observing the same event again will not trigger another
// notification.
sync_controller_delegate()->OnStateChanged(sync_service());
}

TEST_F(PasswordSyncControllerDelegateAndroidTest,
OnSyncStatusDisabledOnStartup) {
sync_service()->SetDisableReasons(
syncer::SyncService::DISABLE_REASON_NOT_SIGNED_IN);

EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenNotSyncing);
sync_controller_delegate()->OnStateChanged(sync_service());
testing::Mock::VerifyAndClearExpectations(bridge());

// Check that observing the same event again will not trigger another
// notification.
sync_controller_delegate()->OnStateChanged(sync_service());
}

TEST_F(PasswordSyncControllerDelegateAndroidTest,
OnSyncStatusChangedToEnabledAfterStartup) {
syncer::TestSyncService sync_service;

EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenSyncing);
sync_controller_delegate()->OnStateChanged(&sync_service);
testing::Mock::VerifyAndClearExpectations(bridge());

// Check that observing the same event again will not trigger another
// notification.
EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenSyncing).Times(0);
sync_controller_delegate()->OnStateChanged(&sync_service);
}

Expand All @@ -127,6 +88,7 @@ TEST_F(PasswordSyncControllerDelegateAndroidTest,
sync_service.GetUserSettings()->SetSelectedTypes(/*sync_everything=*/false,
/*types=*/{});

EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenSyncing).Times(0);
EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenNotSyncing);
sync_controller_delegate()->OnStateChanged(&sync_service);
}
Expand Down Expand Up @@ -155,10 +117,10 @@ TEST_F(PasswordSyncControllerDelegateAndroidTest,

EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenNotSyncing);
sync_controller_delegate()->OnStateChanged(&sync_service);
testing::Mock::VerifyAndClearExpectations(bridge());

// Check that observing the same event again will not trigger another
// notification.
EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenNotSyncing).Times(0);
sync_controller_delegate()->OnStateChanged(&sync_service);
}

Expand Down Expand Up @@ -242,7 +204,6 @@ TEST_F(PasswordSyncControllerDelegateAndroidTest,

TEST_F(PasswordSyncControllerDelegateAndroidTest,
AttachesObserverOnSyncServiceInitialized) {
EXPECT_CALL(*bridge(), NotifyCredentialManagerWhenSyncing);
sync_controller_delegate()->OnSyncServiceInitialized(sync_service());
EXPECT_TRUE(sync_service()->HasObserver(sync_controller_delegate()));
}
Expand Down

0 comments on commit 7d79977

Please sign in to comment.