Reland "[Uno-d] Disable account storage for addresses when in auth error"
This reverts commit 04c57c4843b05dee8781365e0ba8269399d5f8bb.
Reason for revert: initial version had compilation errors, caused by
a conflict with https://crrev.com/c/5509512.
Original change's description:
> Revert "[Uno-d] Disable account storage for addresses when in auth error"
>
> This reverts commit 22dfcb968327f5d7c480c88a3b6ef3255210b502.
>
> Reason for revert: Breaks compilation, for example https://ci.chromium.org/ui/p/chromium/builders/ci/ios-simulator-noncq/44391/overview
>
> Original change's description:
> > [Uno-d] Disable account storage for addresses when in auth error
> >
> > Account storage was enabled while in pending state in the CL:
> > https://crrev.com/c/5432217
> > This CL conceptually reverts to the previous behavior, where
> > account storage is disabled when in signin pending state.
> >
> > The reason for the change is driven by product decisions, as there is
> > a requirement to have valid credentials in order for Chrome to keep
> > using the account storage.
> >
> > This may be explored again or revisited in the future.
> >
> > Bug: b/331601440
> > Change-Id: I551cd51864a2112f0ff0bfcb9c057a8f46a5dff7
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5499309
> > Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
> > Reviewed-by: Ryan Sultanem <rsult@google.com>
> > Commit-Queue: David Roger <droger@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1295514}
>
> Bug: b/331601440
> Change-Id: I57d66808d79dc0ef10accdbf61c63c9ce875974f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5507218
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Owners-Override: Nicolas MacBeth <nicolasmacbeth@google.com>
> Auto-Submit: Nicolas MacBeth <nicolasmacbeth@google.com>
> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1295516}
Bug: b/331601440
Change-Id: Id4ec537f59889b8d4c11dd4152ef68a37926d0f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5511162
Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1295615}
diff --git a/chrome/browser/signin/dice_browsertest.cc b/chrome/browser/signin/dice_browsertest.cc
index 90ba9114..c292d1c4 100644
--- a/chrome/browser/signin/dice_browsertest.cc
+++ b/chrome/browser/signin/dice_browsertest.cc
@@ -1346,7 +1346,6 @@
prefs::kExplicitBrowserSignin));
// Account storage was not enabled yet.
AccountStorageStatus account_storage_status = GetAccountStorageStatus();
- EXPECT_FALSE(account_storage_status.autofill_sync_toggle_available);
EXPECT_FALSE(account_storage_status.user_selectable_type_set.HasAny(
{syncer::UserSelectableType::kAutofill,
syncer::UserSelectableType::kPasswords}));
@@ -1358,7 +1357,6 @@
// Account storage is now enabled.
account_storage_status = GetAccountStorageStatus();
- EXPECT_TRUE(account_storage_status.autofill_sync_toggle_available);
EXPECT_TRUE(account_storage_status.user_selectable_type_set.HasAll(
{syncer::UserSelectableType::kAutofill,
syncer::UserSelectableType::kPasswords}));
diff --git a/chrome/browser/sync/test/integration/single_client_contact_info_sync_test.cc b/chrome/browser/sync/test/integration/single_client_contact_info_sync_test.cc
index 47169caa..97795dea 100644
--- a/chrome/browser/sync/test/integration/single_client_contact_info_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_contact_info_sync_test.cc
@@ -301,7 +301,7 @@
}
#if !BUILDFLAG(IS_ANDROID)
-// Account storage is still enabled when the user is in auth error.
+// Account storage is not enabled when the user is in auth error.
IN_PROC_BROWSER_TEST_F(SingleClientContactInfoTransportSyncTest,
AuthErrorState) {
// Setup transport mode.
@@ -325,31 +325,28 @@
/*expect_active=*/false)
.Wait());
- // Save a profile, it is not uploaded.
+ EXPECT_FALSE(GetPersonalDataManager()
+ ->address_data_manager()
+ .IsEligibleForAddressAccountStorage());
+ EXPECT_FALSE(GetPersonalDataManager()
+ ->address_data_manager()
+ .IsAutofillSyncToggleAvailable());
+ GetPersonalDataManager()->address_data_manager().AddProfile(kProfile);
+
+ // Fix the authentication error, sync is available again.
+ signin::UpdatePersistentErrorOfRefreshTokenForAccount(
+ identity_manager,
+ identity_manager->GetPrimaryAccountId(signin::ConsentLevel::kSignin),
+ GoogleServiceAuthError::AuthErrorNone());
+ EXPECT_TRUE(ContactInfoActiveChecker(GetSyncService(0),
+ /*expect_active=*/true)
+ .Wait());
EXPECT_TRUE(GetPersonalDataManager()
->address_data_manager()
.IsEligibleForAddressAccountStorage());
EXPECT_TRUE(GetPersonalDataManager()
->address_data_manager()
.IsAutofillSyncToggleAvailable());
- GetPersonalDataManager()->address_data_manager().AddProfile(kProfile);
- EXPECT_TRUE(AddressDataManagerProfileChecker(
- &GetPersonalDataManager()->address_data_manager(),
- UnorderedElementsAre(kProfile))
- .Wait());
- EXPECT_TRUE(GetFakeServer()
- ->GetSyncEntitiesByModelType(syncer::CONTACT_INFO)
- .empty());
-
- // Fix the authentication error, data is uploaded.
- signin::UpdatePersistentErrorOfRefreshTokenForAccount(
- identity_manager,
- identity_manager->GetPrimaryAccountId(signin::ConsentLevel::kSignin),
- GoogleServiceAuthError::AuthErrorNone());
- EXPECT_TRUE(FakeServerSpecificsChecker(
- UnorderedElementsAre(
- AsContactInfoSpecifics(kProfile).SerializeAsString()))
- .Wait());
}
IN_PROC_BROWSER_TEST_F(SingleClientContactInfoSyncTest,
diff --git a/components/autofill/core/browser/address_data_manager.cc b/components/autofill/core/browser/address_data_manager.cc
index 2128c019..04f389f 100644
--- a/components/autofill/core/browser/address_data_manager.cc
+++ b/components/autofill/core/browser/address_data_manager.cc
@@ -20,7 +20,6 @@
#include "components/autofill/core/browser/metrics/profile_deduplication_metrics.h"
#include "components/autofill/core/browser/metrics/profile_token_quality_metrics.h"
#include "components/autofill/core/browser/metrics/stored_profile_metrics.h"
-#include "components/autofill/core/browser/webdata/addresses/contact_info_precondition_checker.h"
#include "components/autofill/core/common/autofill_clock.h"
#include "components/autofill/core/common/autofill_prefs.h"
#include "components/prefs/pref_service.h"
@@ -88,13 +87,6 @@
webdata_service_observer_.Observe(webdata_service_.get());
}
- if (sync_service_ && identity_manager_) {
- contact_info_precondition_checker_ =
- std::make_unique<ContactInfoPreconditionChecker>(
- sync_service_, identity_manager_,
- /*on_precondition_changed=*/base::DoNothing());
- }
-
SetPrefService(pref_service);
SetStrikeDatabase(strike_database);
// `IsAutofillProfileEnabled()` relies on the `pref_service_`, which is only
@@ -115,7 +107,6 @@
void AddressDataManager::Shutdown() {
// These classes' sync observers needs to be unregistered.
address_data_cleaner_.reset();
- contact_info_precondition_checker_.reset();
}
void AddressDataManager::AddObserver(AddressDataManager::Observer* obs) {
@@ -305,15 +296,6 @@
return false;
}
- if (::switches::IsExplicitBrowserSigninUIOnDesktopEnabled()) {
- return contact_info_precondition_checker_ &&
- contact_info_precondition_checker_->GetPreconditionState() ==
- syncer::ModelTypeController::PreconditionState::
- kPreconditionsMet &&
- sync_service_->GetUserSettings()->GetSelectedTypes().Has(
- syncer::UserSelectableType::kAutofill);
- }
-
// The CONTACT_INFO data type is only running for eligible users. See
// ContactInfoModelTypeController.
return sync_service_->GetActiveDataTypes().Has(syncer::CONTACT_INFO);
@@ -599,14 +581,10 @@
}
bool AddressDataManager::IsAutofillSyncToggleAvailable() const {
- return sync_service_ && !sync_service_->GetAccountInfo().IsEmpty() &&
+ return IsEligibleForAddressAccountStorage() && sync_service_ &&
!sync_service_->HasSyncConsent() &&
!sync_service_->GetUserSettings()->IsTypeManagedByPolicy(
syncer::UserSelectableType::kAutofill) &&
- contact_info_precondition_checker_ &&
- contact_info_precondition_checker_->GetPreconditionState() ==
- syncer::ModelTypeController::PreconditionState::
- kPreconditionsMet &&
base::FeatureList::IsEnabled(
syncer::kSyncEnableContactInfoDataTypeInTransportMode) &&
pref_service_->GetBoolean(::prefs::kExplicitBrowserSignin);
diff --git a/components/autofill/core/browser/address_data_manager.h b/components/autofill/core/browser/address_data_manager.h
index 044a787b..5446e4d 100644
--- a/components/autofill/core/browser/address_data_manager.h
+++ b/components/autofill/core/browser/address_data_manager.h
@@ -42,7 +42,6 @@
class AddressDataCleaner;
class AlternativeStateNameMapUpdater;
-class ContactInfoPreconditionChecker;
// Contains all address-related logic of the `PersonalDataManager`. See comment
// above the `PersonalDataManager` first. In the `AddressDataManager` (ADM),
@@ -405,9 +404,6 @@
// has finished.
void LogStoredDataMetrics() const;
- std::unique_ptr<ContactInfoPreconditionChecker>
- contact_info_precondition_checker_;
-
// A copy of the profiles stored in `AddressAutofillTable`. They come from
// two sources:
// - kLocalOrSyncable: Stored in `synced_local_profiles_`.
diff --git a/components/autofill/core/browser/address_data_manager_unittest.cc b/components/autofill/core/browser/address_data_manager_unittest.cc
index a28520c79..135acc1 100644
--- a/components/autofill/core/browser/address_data_manager_unittest.cc
+++ b/components/autofill/core/browser/address_data_manager_unittest.cc
@@ -1084,16 +1084,8 @@
}
#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
-class AddressDataManagerExplicitSigninTest
- : public base::test::WithFeatureOverride,
- public AddressDataManagerTest {
- public:
- AddressDataManagerExplicitSigninTest()
- : base::test::WithFeatureOverride(
- ::switches::kExplicitBrowserSigninUIOnDesktop) {}
-};
-TEST_P(AddressDataManagerExplicitSigninTest,
+TEST_F(AddressDataManagerTest,
IsEligibleForAddressAccountStorageSigninPending) {
// Setup account in auth error.
CoreAccountInfo account_info =
@@ -1116,13 +1108,10 @@
ASSERT_TRUE(sync_service_.GetUserSettings()->GetSelectedTypes().Has(
syncer::UserSelectableType::kAutofill));
- // Account storage is eligible when explicit signin UI is enabled.
- EXPECT_EQ(address_data_manager().IsEligibleForAddressAccountStorage(),
- ::switches::IsExplicitBrowserSigninUIOnDesktopEnabled());
+ // Account storage is not eligible.
+ EXPECT_FALSE(address_data_manager().IsEligibleForAddressAccountStorage());
}
-INSTANTIATE_FEATURE_OVERRIDE_TEST_SUITE(AddressDataManagerExplicitSigninTest);
-
TEST_F(AddressDataManagerTest, AutofillSyncToggleAvailableInTransportMode) {
ResetAddressDataManager(
/*use_sync_transport_mode=*/true);
