chromium/src
0
Fork 0
Code Activity

Remove SyncPlusAddress flag.

Browse Source 
The CL removes the default enabled flag `SyncPlusAddress`, alongside
with code that's never executed anymore.

Change-Id: I5202851e635e20b8a1564fa90d9d09912ea98b4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5800318
Reviewed-by: Christoph Schwering <schwering@google.com>
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Commit-Queue: Norge Vizcay <vizcay@google.com>
Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1345314}
This commit is contained in:
Norge Vizcay
2024-08-22 09:40:37 +00:00
committed by Chromium LUCI CQ
parent bc357db7d9
commit eeeaa36769
14 changed files with 55 additions and 193 deletions

3
chrome/browser/sync/test/integration/single_client_plus_address_sync_test.cc

@ -88,8 +88,7 @@ class SingleClientPlusAddressSyncTest
/*enabled_features=*/{{plus_addresses::features::kPlusAddressesEnabled,
{{plus_addresses::features::
kEnterprisePlusAddressServerUrl.name,
"https://not-used.com"}}},
{syncer::kSyncPlusAddress, {}}},
"https://not-used.com"}}}},
/*disabled_features=*/{});
}

6
chrome/browser/sync/test/integration/sync_test.cc

@ -1130,10 +1130,12 @@ syncer::DataTypeSet AllowedTypesInStandaloneTransportMode() {
syncer::kSyncEnableContactInfoDataTypeInTransportMode)) {
allowed_types.Put(syncer::CONTACT_INFO);
}
if (base::FeatureList::IsEnabled(syncer::kSyncPlusAddress)) {
allowed_types.Put(syncer::PLUS_ADDRESS);
allowed_types.Put(syncer::PLUS_ADDRESS);
if (base::FeatureList::IsEnabled(syncer::kSyncPlusAddressSetting)) {
allowed_types.Put(syncer::PLUS_ADDRESS_SETTING);
}
if (base::FeatureList::IsEnabled(
syncer::kSyncEnableWalletMetadataInTransportMode)) {
allowed_types.Put(syncer::AUTOFILL_WALLET_METADATA);

3
chrome/browser/ui/autofill/autofill_context_menu_manager_browsertest.cc

@ -1686,8 +1686,7 @@ class PlusAddressContextMenuManagerTest
{{plus_addresses::features::kEnterprisePlusAddressServerUrl.name,
"https://foo.bar"}}},
{plus_addresses::features::kPlusAddressFallbackFromContextMenu, {}},
{plus_addresses::features::kPlusAddressBlocklistEnabled, {}},
{syncer::kSyncPlusAddress, {}}},
{plus_addresses::features::kPlusAddressBlocklistEnabled, {}}},
/*disabled_features=*/{});
}

3
components/browser_sync/common_controller_builder.cc

@ -555,8 +555,7 @@ CommonControllerBuilder::Build(syncer::DataTypeSet disabled_types,
// `kEnterprisePlusAddressServerUrl` is checked to prevent enabling the
// feature in dev builds via the field trial config.
if (!disabled_types.Has(syncer::PLUS_ADDRESS) &&
plus_address_webdata_service_.value() && google_groups_manager_.value() &&
base::FeatureList::IsEnabled(syncer::kSyncPlusAddress)) {
plus_address_webdata_service_.value() && google_groups_manager_.value()) {
controllers.push_back(
std::make_unique<plus_addresses::PlusAddressDataTypeController>(
syncer::PLUS_ADDRESS,

1
components/plus_addresses/plus_address_http_client_impl.h

@ -64,6 +64,7 @@ class PlusAddressHttpClientImpl : public PlusAddressHttpClient {
PlusAddressRequestCallback on_completed) override;
void PreallocatePlusAddresses(
PreallocatePlusAddressesCallback callback) override;
// TODO(crbug.com/322147254): Remove unused method.
void GetAllPlusAddresses(PlusAddressMapRequestCallback on_completed) override;
void Reset() override;

4
components/plus_addresses/plus_address_parsing_utils.cc

@ -67,10 +67,6 @@ std::optional<PlusProfile> ParsePlusProfileFromV1Dict(base::Value::Dict dict) {
!is_confirmed.has_value()) {
return std::nullopt;
}
if (!IsSyncingPlusAddresses()) {
return PlusProfile(std::move(profile_id), std::move(facet_str),
std::move(plus_address), *is_confirmed);
}
affiliations::FacetURI facet =
affiliations::FacetURI::FromPotentiallyInvalidSpec(facet_str);
if (!facet.is_valid()) {

113
components/plus_addresses/plus_address_service.cc

@ -74,24 +74,14 @@ base::flat_set<std::string> GetAndParseExcludedSites() {
}
PlusProfile::facet_t OriginToFacet(const url::Origin& origin) {
PlusProfile::facet_t facet;
if (IsSyncingPlusAddresses()) {
// For a valid `origin`, `origin.GetURL().spec()` is always a valid spec.
// However, using `FacetURI::FromCanonicalSpec(spec)` can lead to mismatches
// in the underlying representation, since it uses the spec verbatim. E.g.,
// a trailing "/" is removed by `FacetURI::FromPotentiallyInvalidSpec()`,
// but kept by `FacetURI::FromCanonicalSpec(spec)`.
// TODO(b/338342346): Revise `FacetURI::FromCanonicalSpec()`.
facet = affiliations::FacetURI::FromPotentiallyInvalidSpec(
origin.GetURL().spec());
} else {
std::string etld_plus_one = GetEtldPlusOne(origin);
// TODO(crbug.com/327838014): Remove the fallback and use
// `affiliations::FacetURI` in the tests.
// For empty `etld_plus_one`, fallback to `origin`.
facet = etld_plus_one.empty() ? origin.GetURL().spec() : etld_plus_one;
}
return facet;
// For a valid `origin`, `origin.GetURL().spec()` is always a valid spec.
// However, using `FacetURI::FromCanonicalSpec(spec)` can lead to mismatches
// in the underlying representation, since it uses the spec verbatim. E.g.,
// a trailing "/" is removed by `FacetURI::FromPotentiallyInvalidSpec()`,
// but kept by `FacetURI::FromCanonicalSpec(spec)`.
// TODO(b/338342346): Revise `FacetURI::FromCanonicalSpec()`.
return affiliations::FacetURI::FromPotentiallyInvalidSpec(
origin.GetURL().spec());
}
// Returns `true` when we wish to offer plus address creation on a form with
@ -190,13 +180,12 @@ PlusAddressService::PlusAddressService(
base::BindRepeating(&PlusAddressService::IsEnabled,
base::Unretained(this)));
if (IsSyncingPlusAddresses() && webdata_service_) {
if (webdata_service_) {
webdata_service_observation_.Observe(webdata_service_.get());
if (IsEnabled()) {
webdata_service_->GetPlusProfiles(this);
}
}
CreateAndStartTimer();
identity_manager_observation_.Observe(identity_manager);
if (!base::FeatureList::IsEnabled(features::kPlusAddressBlocklistEnabled)) {
@ -299,12 +288,12 @@ std::optional<PlusProfile> PlusAddressService::GetPlusProfile(
void PlusAddressService::SavePlusProfile(const PlusProfile& profile) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
CHECK(profile.is_confirmed);
// New plus addresses are requested directly from the PlusAddress backend. If
// `IsSyncingPlusAddresses()`, these addresses become later available through
// sync. Until the address shows up in sync, it should still be available
// through `PlusAddressService`, even after reloading the data. This requires
// adding the address to the database.
if (webdata_service_ && IsSyncingPlusAddresses()) {
// New plus addresses are requested directly from the PlusAddress backend.
// These addresses become later available through sync. Until the address
// shows up in sync, it should still be available through
// `PlusAddressService`, even after reloading the data. This requires adding
// the address to the database.
if (webdata_service_) {
webdata_service_->AddOrUpdatePlusProfile(profile);
}
// Update the in-memory plus profiles cache.
@ -500,13 +489,7 @@ void PlusAddressService::HandleCreateOrConfirmResponse(
if (maybe_profile.has_value()) {
account_is_forbidden_ = false;
if (maybe_profile->is_confirmed) {
if (IsSyncingPlusAddresses()) {
SavePlusProfile(*maybe_profile);
} else {
PlusProfile profile_to_save = *maybe_profile;
profile_to_save.facet = GetEtldPlusOne(origin);
SavePlusProfile(profile_to_save);
}
SavePlusProfile(*maybe_profile);
}
} else {
HandlePlusAddressRequestError(maybe_profile.error());
@ -546,62 +529,6 @@ bool PlusAddressService::IsEnabled() const {
.state() == GoogleServiceAuthError::State::NONE;
}
void PlusAddressService::CreateAndStartTimer() {
if (!IsEnabled() || !features::kSyncWithEnterprisePlusAddressServer.Get() ||
polling_timer_.IsRunning()) {
return;
}
if (IsSyncingPlusAddresses()) {
return;
}
SyncPlusAddressMapping();
polling_timer_.Start(
FROM_HERE, features::kEnterprisePlusAddressTimerDelay.Get(),
base::BindRepeating(&PlusAddressService::SyncPlusAddressMapping,
// base::Unretained(this) is safe here since the timer
// that is created has same lifetime as this service.
base::Unretained(this)));
}
void PlusAddressService::SyncPlusAddressMapping() {
if (!IsEnabled()) {
return;
}
plus_address_http_client_->GetAllPlusAddresses(base::BindOnce(
[](PlusAddressService* service,
const PlusAddressMapOrError& maybe_mapping) {
if (maybe_mapping.has_value()) {
if (service->IsEnabled()) {
service->UpdatePlusAddressMap(maybe_mapping.value());
}
service->account_is_forbidden_ = false;
} else {
service->HandlePlusAddressRequestError(maybe_mapping.error());
// If `kDisableForForbiddenUsers` is on, we retry 403 responses.
if (features::kDisableForForbiddenUsers.Get() &&
maybe_mapping.error() == PlusAddressRequestError::AsNetworkError(
net::HTTP_FORBIDDEN) &&
!service->account_is_forbidden_.value_or(false)) {
service->SyncPlusAddressMapping();
}
}
},
// base::Unretained is safe here since PlusAddressService owns
// the PlusAddressHttpClient and they have the same lifetime.
base::Unretained(this)));
}
void PlusAddressService::UpdatePlusAddressMap(const PlusAddressMap& map) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
plus_address_cache_.Clear();
for (const auto& [facet, address] : map) {
// `UpdatePlusAddressMap()` is only called when sync support is disabled.
// In this case, profile_ids don't matter.
plus_address_cache_.InsertProfile(
PlusProfile(/*profile_id=*/"", facet, address, /*is_confirmed=*/true));
}
}
void PlusAddressService::OnWebDataChangedBySync(
const std::vector<PlusAddressDataChange>& changes) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@ -678,8 +605,6 @@ void PlusAddressService::OnPrimaryAccountChanged(
event.GetEventTypeFor(signin::ConsentLevel::kSignin);
if (type == signin::PrimaryAccountChangeEvent::Type::kCleared) {
HandleSignout();
} else if (type == signin::PrimaryAccountChangeEvent::Type::kSet) {
CreateAndStartTimer();
}
}
@ -695,17 +620,11 @@ void PlusAddressService::OnErrorStateOfRefreshTokenUpdatedForAccount(
}
if (error.state() != GoogleServiceAuthError::NONE) {
HandleSignout();
} else {
CreateAndStartTimer();
}
}
void PlusAddressService::HandleSignout() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!IsSyncingPlusAddresses()) {
plus_address_cache_.Clear();
polling_timer_.Stop();
}
plus_address_http_client_->Reset();
}

20
components/plus_addresses/plus_address_service.h

@ -14,7 +14,6 @@
#include "base/observer_list.h"
#include "base/observer_list_types.h"
#include "base/scoped_observation.h"
#include "base/timer/timer.h"
#include "components/autofill/core/browser/autofill_plus_address_delegate.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/plus_addresses/affiliations/plus_address_affiliation_match_helper.h"
@ -200,16 +199,6 @@ class PlusAddressService : public KeyedService,
bool IsEnabled() const;
private:
// Creates and starts a timer to keep `plus_profiles_` and
// `plus_addresses_` in sync with a remote plus address server.
// This has no effect if this service is not enabled or the timer is already
// running.
void CreateAndStartTimer();
// Gets the up-to-date plus address mapping mapping from the remote server
// from the PlusAddressHttpClient.
void SyncPlusAddressMapping();
// Checks whether `error` is a `HTTP_FORBIDDEN` network error and, if there
// have been more than `kMaxAllowedForbiddenResponses` such calls without a
// successful one, disables plus addresses for the session.
@ -238,10 +227,6 @@ class PlusAddressService : public KeyedService,
// on `excluded_sites_` set, and scheme is http or https.
bool IsSupportedOrigin(const url::Origin& origin) const;
// Updates `plus_profiles_` and `plus_addresses_` using `map`.
// TODO(b/322147254): Remove once integration has finished.
void UpdatePlusAddressMap(const PlusAddressMap& map);
// Called when PlusAddressService::OnGetAffiliatedPlusProfiles is resolved.
// Builds a list of suggestions from the list of `affiliated_profiles` and
// returns it via the `callback`.
@ -265,11 +250,8 @@ class PlusAddressService : public KeyedService,
metrics::PlusAddressSubmissionLogger submission_logger_;
// A timer to periodically retrieve all plus addresses from a remote server
// to keep this service in sync.
base::RepeatingTimer polling_timer_;
// Handles requests to a remote server that this service uses.
// TODO(crbug.com/322147254): Move to allocator.
std::unique_ptr<PlusAddressHttpClient> plus_address_http_client_;
// Responsible for communicating with `PlusAddressTable`.

4
components/plus_addresses/plus_address_service_unittest.cc

@ -498,8 +498,6 @@ TEST_F(PlusAddressServiceRequestsTest,
// the request ends in an error and the eventual server response is ignored.
TEST_F(PlusAddressServiceRequestsTest,
PrimaryRefreshTokenError_ResetsHttpRequests) {
base::test::ScopedFeatureList sync_feature_{
GetSyncPlusAddressFeatureForTests()};
PlusProfile profile = test::CreatePlusProfile();
base::test::TestFuture<const PlusProfileOrError&> future;
service().ReservePlusAddress(OriginFromFacet(profile.facet),
@ -1560,8 +1558,6 @@ class PlusAddressAffiliationsTest : public PlusAddressServiceTest {
features::kPlusAddressesEnabled,
{{"server-url", "https://server.example"},
{"oauth-scope", "scope.example"}}),
base::test::FeatureRefAndParams(
{GetSyncPlusAddressFeatureForTests(), {}}),
base::test::FeatureRefAndParams(
{features::kPlusAddressAffiliations, {}})},
// Disable features:

6
components/plus_addresses/plus_address_types.h

@ -44,10 +44,8 @@ struct PreallocatedPlusAddress final {
};
struct PlusProfile {
// When `syncer::kSyncPlusAddress` is enabled, the facet is stored as a
// `FacetURI`. Before sync support, the facet represents an eTLD+1, stored as
// a string.
// TODO(b/322147254): Remove variant when sync support is launched.
// The facet is stored as a `FacetURI`.
// TODO(b/322147254): Remove unnecessary variant.
using facet_t = absl::variant<std::string, affiliations::FacetURI>;
PlusProfile(std::optional<std::string> profile_id,

68
components/plus_addresses/webdata/plus_address_webdata_service.cc

@ -36,39 +36,36 @@ PlusAddressWebDataService::PlusAddressWebDataService(
: WebDataServiceBase(wdbs, ui_task_runner),
ui_task_runner_(std::move(ui_task_runner)),
db_task_runner_(std::move(db_task_runner)) {
if (base::FeatureList::IsEnabled(syncer::kSyncPlusAddress)) {
sync_bridge_wrapper_ =
base::MakeRefCounted<SyncBridgeDBSequenceWrapper>(db_task_runner_);
sync_bridge_wrapper_ =
base::MakeRefCounted<SyncBridgeDBSequenceWrapper>(db_task_runner_);
// When sync changes `PlusAddressTable`, observers on the `ui_task_runner_`
// are notified. To avoid round trips to the `db_task_runner_`, this
// notification includes the set of addition and removal operations
// committed to the database from the sync bridge.
PlusAddressSyncBridge::DataChangedBySyncCallback notify_sync_observers =
base::BindPostTask(
ui_task_runner_,
base::BindRepeating(
&PlusAddressWebDataService::NotifyOnWebDataChangedBySync,
weak_factory_.GetWeakPtr()));
// When sync changes `PlusAddressTable`, observers on the `ui_task_runner_`
// are notified. To avoid round trips to the `db_task_runner_`, this
// notification includes the set of addition and removal operations
// committed to the database from the sync bridge.
PlusAddressSyncBridge::DataChangedBySyncCallback notify_sync_observers =
base::BindPostTask(
ui_task_runner_,
base::BindRepeating(
&PlusAddressWebDataService::NotifyOnWebDataChangedBySync,
weak_factory_.GetWeakPtr()));
// The `state->sync_bridge` can only be used on the sequence that it
// was constructed on. Ensure it is created on the `db_task_runner_`.
db_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(
[](scoped_refptr<WebDatabaseBackend> db_backend,
PlusAddressSyncBridge::DataChangedBySyncCallback
notify_observers,
SyncBridgeDBSequenceWrapper* wrapper) {
wrapper->sync_bridge = std::make_unique<PlusAddressSyncBridge>(
std::make_unique<syncer::ClientTagBasedDataTypeProcessor>(
syncer::PLUS_ADDRESS,
/*dump_stack=*/base::DoNothing()),
std::move(db_backend), std::move(notify_observers));
},
wdbs_->GetBackend(), std::move(notify_sync_observers),
base::RetainedRef(sync_bridge_wrapper_)));
}
// The `state->sync_bridge` can only be used on the sequence that it
// was constructed on. Ensure it is created on the `db_task_runner_`.
db_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(
[](scoped_refptr<WebDatabaseBackend> db_backend,
PlusAddressSyncBridge::DataChangedBySyncCallback notify_observers,
SyncBridgeDBSequenceWrapper* wrapper) {
wrapper->sync_bridge = std::make_unique<PlusAddressSyncBridge>(
std::make_unique<syncer::ClientTagBasedDataTypeProcessor>(
syncer::PLUS_ADDRESS,
/*dump_stack=*/base::DoNothing()),
std::move(db_backend), std::move(notify_observers));
},
wdbs_->GetBackend(), std::move(notify_sync_observers),
base::RetainedRef(sync_bridge_wrapper_)));
}
PlusAddressWebDataService::~PlusAddressWebDataService() = default;
@ -122,7 +119,6 @@ void PlusAddressWebDataService::ClearPlusProfiles() {
std::unique_ptr<syncer::DataTypeControllerDelegate>
PlusAddressWebDataService::GetSyncControllerDelegate() {
CHECK(ui_task_runner_->RunsTasksInCurrentSequence());
CHECK(base::FeatureList::IsEnabled(syncer::kSyncPlusAddress));
// `sync_bridge` operates on the `db_task_runner_` - use a
// `ProxyDataTypeControllerDelegate` to forward calls to that sequence.
// Because `db_task_runner_` is a `SequencedTaskRunner`, the `sync_bridge`
@ -145,12 +141,4 @@ void PlusAddressWebDataService::NotifyOnWebDataChangedBySync(
}
}
bool IsSyncingPlusAddresses() {
return base::FeatureList::IsEnabled(syncer::kSyncPlusAddress);
}
const base::Feature& GetSyncPlusAddressFeatureForTests() {
return syncer::kSyncPlusAddress;
}
} // namespace plus_addresses

8
components/plus_addresses/webdata/plus_address_webdata_service.h

@ -111,14 +111,6 @@ class PlusAddressWebDataService : public WebDataServiceBase {
base::WeakPtrFactory<PlusAddressWebDataService> weak_factory_{this};
};
// Returns true if `syncer::kSyncPlusAddress` is enabled. This only exists to
// avoid a sync dependency in components/plus_addresses.
bool IsSyncingPlusAddresses();
// Returns the `syncer::kSyncPlusAddress` feature to be accessed from tests.
// This only exists to avoid a sync dependency in components/plus_addresses.
const base::Feature& GetSyncPlusAddressFeatureForTests();
} // namespace plus_addresses
#endif // COMPONENTS_PLUS_ADDRESSES_WEBDATA_PLUS_ADDRESS_WEBDATA_SERVICE_H_

4
components/sync/base/features.cc

@ -34,10 +34,6 @@ BASE_FEATURE(kSyncAutofillWalletCredentialData,
"SyncAutofillWalletCredentialData",
base::FEATURE_DISABLED_BY_DEFAULT);
BASE_FEATURE(kSyncPlusAddress,
"SyncPlusAddress",
base::FEATURE_ENABLED_BY_DEFAULT);
BASE_FEATURE(kSyncPlusAddressSetting,
"SyncPlusAddressSetting",
base::FEATURE_DISABLED_BY_DEFAULT);

5
components/sync/base/features.h

@ -45,11 +45,6 @@ BASE_DECLARE_FEATURE(kSyncAutofillWalletUsageData);
// Controls whether to enable syncing of Autofill Wallet Credential Data.
BASE_DECLARE_FEATURE(kSyncAutofillWalletCredentialData);
// Controls if the `PlusAddressSyncBridge`, controlling PLUS_ADDRESS should be
// instantiated.
// TODO(b/322147254): Cleanup when launched.
BASE_DECLARE_FEATURE(kSyncPlusAddress);
// Controls if the `PlusAddressSettingSyncBridge`, controlling
// PLUS_ADDRESS_SETTING should be instantiated.
// TODO(b/342089839): Cleanup when launched.