Replace PolicyBundle::CopyFrom with PolicyBundle::Clone

The existing `CopyFrom` is not immediately obvious as the name does
not clearly indicate whether the copy would be replacing or adding to
the existing data.

This CL therefore introduces a `Clone` function. Its results can be
move assigned, for instance, which allows for more idiomatic code.

Bug: b/255284552
Change-Id: I8fb4d94b5c8491f65b881a79b43c9b5f68d21966
Tests: Existing tests
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3725548
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Roland Bock <rbock@google.com>
Quick-Run: Roland Bock <rbock@google.com>
Reviewed-by: Igor <igorcov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067475}

chrome/browser/ash/policy/core/device_local_account_policy_provider.cc

@@ -128,7 +128,7 @@ void DeviceLocalAccountPolicyProvider::UpdateFromBroker() {
     // Keep existing policy, but do send an update.
     waiting_for_policy_refresh_ = false;
     weak_factory_.InvalidateWeakPtrs();
-    bundle.CopyFrom(policies());
+    bundle = policies().Clone();
   }
 
   PolicyMap& chrome_policy =

chrome/browser/extensions/extension_management_test_util.cc

@@ -334,8 +334,7 @@ void ExtensionManagementPrefUpdaterBase::RemoveStringFromList(
 
 ExtensionManagementPolicyUpdater::ExtensionManagementPolicyUpdater(
     policy::MockConfigurationPolicyProvider* policy_provider)
-    : provider_(policy_provider) {
-  policies_.CopyFrom(provider_->policies());
+    : provider_(policy_provider), policies_(provider_->policies().Clone()) {
   const base::Value* policy_value =
       policies_
           .Get(policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME,

chrome/browser/policy/restricted_mgs_policy_provider.cc

@@ -66,9 +66,8 @@ RestrictedMGSPolicyProvider::Create() {
 void RestrictedMGSPolicyProvider::RefreshPolicies() {}
 
 void RestrictedMGSPolicyProvider::UpdatePolicyBundle() {
-  PolicyBundle bundle;
   weak_factory_.InvalidateWeakPtrs();
-  bundle.CopyFrom(policies());
+  PolicyBundle bundle = policies().Clone();
 
   PolicyMap& chrome_policy =
       bundle.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));

components/policy/core/common/async_policy_provider_unittest.cc

@@ -65,10 +65,7 @@ MockPolicyLoader::MockPolicyLoader(
 MockPolicyLoader::~MockPolicyLoader() {}
 
 PolicyBundle MockPolicyLoader::Load() {
-  PolicyBundle bundle;
-  const PolicyBundle* loaded = MockLoad();
-  bundle.CopyFrom(*loaded);
-  return bundle;
+  return MockLoad()->Clone();
 }
 
 }  // namespace

components/policy/core/common/cloud/component_cloud_policy_service.cc

@@ -203,8 +203,7 @@ void ComponentCloudPolicyService::Backend::InitIfNeeded() {
   updater_ = std::make_unique<ComponentCloudPolicyUpdater>(
       task_runner_, std::move(external_policy_data_fetcher_), &store_);
 
-  std::unique_ptr<PolicyBundle> bundle(std::make_unique<PolicyBundle>());
-  bundle->CopyFrom(store_.policy());
+  auto bundle(std::make_unique<PolicyBundle>(store_.policy().Clone()));
   service_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&ComponentCloudPolicyService::SetPolicy, service_,
@@ -235,8 +234,8 @@ void ComponentCloudPolicyService::Backend::
   }
   DVLOG(2) << "Installing updated policy from the component policy store";
 
-  std::unique_ptr<PolicyBundle> bundle(std::make_unique<PolicyBundle>());
-  bundle->CopyFrom(store_.policy());
+  std::unique_ptr<PolicyBundle> bundle(
+      std::make_unique<PolicyBundle>(store_.policy().Clone()));
   service_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&ComponentCloudPolicyService::SetPolicy, service_,
@@ -525,7 +524,7 @@ void ComponentCloudPolicyService::FilterAndInstallPolicy() {
 
   // Make a copy in |policy_| and filter it and validate against the schemas;
   // this is what's passed to the outside world.
-  policy_.CopyFrom(*unfiltered_policy_);
+  policy_ = unfiltered_policy_->Clone();
   current_schema_map_->FilterBundle(policy_,
                                     /*drop_invalid_component_policies=*/false);
 

components/policy/core/common/fake_async_policy_loader.cc

@@ -15,17 +15,15 @@ FakeAsyncPolicyLoader::FakeAsyncPolicyLoader(
     : AsyncPolicyLoader(task_runner, /*periodic_updates=*/true) {}
 
 PolicyBundle FakeAsyncPolicyLoader::Load() {
-  PolicyBundle bundle;
-  bundle.CopyFrom(policy_bundle_);
-  return bundle;
+  return policy_bundle_.Clone();
 }
 
 void FakeAsyncPolicyLoader::InitOnBackgroundThread() {
   // Nothing to do.
 }
 
 void FakeAsyncPolicyLoader::SetPolicies(const PolicyBundle& policy_bundle) {
-  policy_bundle_.CopyFrom(policy_bundle);
+  policy_bundle_ = policy_bundle.Clone();
 }
 
 void FakeAsyncPolicyLoader::PostReloadOnBackgroundThread(bool force) {

components/policy/core/common/mock_configuration_policy_provider.cc

@@ -57,9 +57,7 @@ void MockConfigurationPolicyProvider::SetAutoRefresh() {
 }
 
 void MockConfigurationPolicyProvider::RefreshWithSamePolicies() {
-  PolicyBundle bundle;
-  bundle.CopyFrom(policies());
-  UpdatePolicy(std::move(bundle));
+  UpdatePolicy(policies().Clone());
 }
 
 MockConfigurationPolicyObserver::MockConfigurationPolicyObserver() {}

components/policy/core/common/policy_bundle.cc

@@ -36,13 +36,12 @@ const PolicyMap& PolicyBundle::Get(const PolicyNamespace& ns) const {
   return it == end() ? GetEmptyPolicyMap() : it->second;
 }
 
-void PolicyBundle::CopyFrom(const PolicyBundle& other) {
-  DCHECK_NE(this, &other);
-
-  Clear();
-  for (const auto& entry_other : other) {
-    policy_bundle_[entry_other.first] = entry_other.second.Clone();
+PolicyBundle PolicyBundle::Clone() const {
+  PolicyBundle clone;
+  for (const auto& entry : policy_bundle_) {
+    clone.policy_bundle_[entry.first] = entry.second.Clone();
   }
+  return clone;
 }
 
 void PolicyBundle::MergeFrom(const PolicyBundle& other) {

components/policy/core/common/policy_bundle.h

@@ -35,8 +35,8 @@ class POLICY_EXPORT PolicyBundle {
   // empty map if no entry for `ns` is present.
   const PolicyMap& Get(const PolicyNamespace& ns) const;
 
-  // |this| becomes a copy of |other|. Any existing PolicyMaps are dropped.
-  void CopyFrom(const PolicyBundle& other);
+  // Create a clone of `this`.
+  PolicyBundle Clone() const;
 
   // Merges the PolicyMaps of |this| with those of |other| for each namespace
   // in common. Also adds copies of the (namespace, PolicyMap) pairs in |other|

components/policy/core/common/policy_bundle_unittest.cc

@@ -116,7 +116,7 @@ TEST(PolicyBundleTest, CopyFrom) {
   EXPECT_TRUE(bundle0.Get(PolicyNamespace(POLICY_DOMAIN_EXTENSIONS,
                                           kExtension0)).Equals(policy));
 
-  bundle1.CopyFrom(bundle0);
+  bundle1 = bundle0.Clone();
   EXPECT_FALSE(IsEmpty(bundle0));
   EXPECT_FALSE(IsEmpty(bundle1));
   EXPECT_TRUE(bundle0.Get(PolicyNamespace(POLICY_DOMAIN_CHROME,
@@ -231,24 +231,24 @@ TEST(PolicyBundleTest, Equals) {
 
   PolicyBundle other;
   EXPECT_FALSE(bundle.Equals(other));
-  other.CopyFrom(bundle);
+  other = bundle.Clone();
   EXPECT_TRUE(bundle.Equals(other));
 
   AddTestPolicies(&bundle.Get(
       PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, kExtension1)));
   EXPECT_FALSE(bundle.Equals(other));
-  other.CopyFrom(bundle);
+  other = bundle.Clone();
   EXPECT_TRUE(bundle.Equals(other));
   AddTestPolicies(&other.Get(
       PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, kExtension2)));
   EXPECT_FALSE(bundle.Equals(other));
 
-  other.CopyFrom(bundle);
+  other = bundle.Clone();
   bundle.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
       .Set(kPolicy0, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
            POLICY_SOURCE_CLOUD, base::Value(123), nullptr);
   EXPECT_FALSE(bundle.Equals(other));
-  other.CopyFrom(bundle);
+  other = bundle.Clone();
   EXPECT_TRUE(bundle.Equals(other));
   bundle.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
       .Set(kPolicy0, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,

components/policy/core/common/policy_service_impl.cc

@@ -323,8 +323,7 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() {
   DefaultChromeAppsMigrator chrome_apps_migrator;
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   for (auto* provider : providers_) {
-    PolicyBundle provided_bundle;
-    provided_bundle.CopyFrom(provider->policies());
+    PolicyBundle provided_bundle = provider->policies().Clone();
     IgnoreUserCloudPrecedencePolicies(&provided_bundle.Get(chrome_namespace));
     DowngradeMetricsReportingToRecommendedPolicy(
         &provided_bundle.Get(chrome_namespace));

components/policy/core/common/proxy_policy_provider.cc

@@ -48,9 +48,7 @@ void ProxyPolicyProvider::RefreshPolicies() {
     // Subtle: if a RefreshPolicies() call comes after Shutdown() then the
     // current bundle should be served instead. This also does the right thing
     // if SetDelegate() was never called before.
-    PolicyBundle bundle;
-    bundle.CopyFrom(policies());
-    UpdatePolicy(std::move(bundle));
+    UpdatePolicy(policies().Clone());
   }
 }
 
@@ -64,9 +62,7 @@ void ProxyPolicyProvider::OnUpdatePolicy(
     return;
 
   DCHECK_EQ(delegate_, provider);
-  PolicyBundle bundle;
-  bundle.CopyFrom(delegate_->policies());
-  UpdatePolicy(std::move(bundle));
+  UpdatePolicy(delegate_->policies().Clone());
 }
 
 }  // namespace policy

components/policy/core/common/proxy_policy_provider_unittest.cc

@@ -36,12 +36,6 @@ class ProxyPolicyProviderTest : public testing::Test {
   MockConfigurationPolicyObserver observer_;
   MockConfigurationPolicyProvider mock_provider_;
   ProxyPolicyProvider proxy_provider_;
-
-  static PolicyBundle CopyBundle(const PolicyBundle& bundle) {
-    PolicyBundle copy;
-    copy.CopyFrom(bundle);
-    return copy;
-  }
 };
 
 TEST_F(ProxyPolicyProviderTest, Init) {
@@ -54,7 +48,7 @@ TEST_F(ProxyPolicyProviderTest, Delegate) {
   bundle.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
       .Set("policy", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
            POLICY_SOURCE_CLOUD, base::Value("value"), nullptr);
-  mock_provider_.UpdatePolicy(CopyBundle(bundle));
+  mock_provider_.UpdatePolicy(bundle.Clone());
 
   EXPECT_CALL(observer_, OnUpdatePolicy(&proxy_provider_));
   proxy_provider_.SetDelegate(&mock_provider_);
@@ -65,7 +59,7 @@ TEST_F(ProxyPolicyProviderTest, Delegate) {
   bundle.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
       .Set("policy", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
            POLICY_SOURCE_CLOUD, base::Value("new value"), nullptr);
-  mock_provider_.UpdatePolicy(CopyBundle(bundle));
+  mock_provider_.UpdatePolicy(bundle.Clone());
   Mock::VerifyAndClearExpectations(&observer_);
   EXPECT_TRUE(bundle.Equals(proxy_provider_.policies()));
 

components/policy/core/common/schema_map_unittest.cc

@@ -146,7 +146,7 @@ TEST_F(SchemaMapTest, FilterBundle) {
   expected_bundle.Get(chrome_ns).Set("ChromePolicy", POLICY_LEVEL_MANDATORY,
                                      POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
                                      base::Value("value"), nullptr);
-  bundle.CopyFrom(expected_bundle);
+  bundle = expected_bundle.Clone();
 
   // Unknown components are filtered out.
   PolicyNamespace another_extension_ns(POLICY_DOMAIN_EXTENSIONS, "xyz");

components/policy/core/common/schema_registry_tracking_policy_provider.cc

@@ -92,7 +92,7 @@ void SchemaRegistryTrackingPolicyProvider::OnUpdatePolicy(
 
   PolicyBundle bundle;
   if (state_ == READY) {
-    bundle.CopyFrom(delegate_->policies());
+    bundle = delegate_->policies().Clone();
     schema_map()->FilterBundle(bundle,
                                /*drop_invalid_component_policies=*/true);
   } else {

components/policy/core/common/schema_registry_tracking_policy_provider_unittest.cc

@@ -89,8 +89,7 @@ TEST_F(SchemaRegistryTrackingPolicyProviderTest, PassOnChromePolicy) {
                             nullptr);
 
   EXPECT_CALL(observer_, OnUpdatePolicy(&schema_registry_tracking_provider_));
-  PolicyBundle delegate_bundle;
-  delegate_bundle.CopyFrom(bundle);
+  PolicyBundle delegate_bundle = bundle.Clone();
   delegate_bundle.Get(PolicyNamespace(POLICY_DOMAIN_EXTENSIONS, "xyz"))
       .Set("foo", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
            POLICY_SOURCE_CLOUD, base::Value("not visible"), nullptr);
@@ -207,8 +206,7 @@ TEST_F(SchemaRegistryTrackingPolicyProviderTest, RemoveAndAddComponent) {
   PolicyBundle platform_policy;
   platform_policy.Get(ns).Set("foo", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
                               POLICY_SOURCE_CLOUD, base::Value("omg"), nullptr);
-  PolicyBundle copy;
-  copy.CopyFrom(platform_policy);
+  PolicyBundle copy = platform_policy.Clone();
   EXPECT_CALL(observer_, OnUpdatePolicy(_));
   mock_provider_.UpdatePolicy(std::move(copy));
   Mock::VerifyAndClearExpectations(&observer_);
@@ -229,8 +227,7 @@ TEST_F(SchemaRegistryTrackingPolicyProviderTest, RemoveAndAddComponent) {
   Mock::VerifyAndClearExpectations(&mock_provider_);
 
   EXPECT_CALL(observer_, OnUpdatePolicy(_));
-  copy = PolicyBundle();
-  copy.CopyFrom(platform_policy);
+  copy = platform_policy.Clone();
   mock_provider_.UpdatePolicy(std::move(copy));
   Mock::VerifyAndClearExpectations(&observer_);
   EXPECT_TRUE(