From 0c98e899346fc34dca126f81c1a1e7feb692a96a Mon Sep 17 00:00:00 2001 From: Scott Violet Date: Wed, 31 Jan 2018 00:58:18 +0000 Subject: [PATCH 10/10] cleanup how ConfigurationPolicyProviders are set It's expected that the providers are available when the service is committed, so it make it explicit. BUG=none TEST=none Change-Id: Ibc2f9be6ecec9261e1a2b9ebef18b16557882398 Reviewed-on: https://chromium-review.googlesource.com/890722 Commit-Queue: Scott Violet Reviewed-by: Maksim Ivanov Cr-Commit-Position: refs/heads/master@{#533126} --- .../browser/aw_browser_policy_connector.cc | 11 ++-- .../browser/aw_browser_policy_connector.h | 15 +++-- chrome/browser/browser_process_impl.cc | 1 - .../policy/browser_policy_connector_chromeos.cc | 8 ++- .../policy/browser_policy_connector_chromeos.h | 5 +- ...cloud_external_data_policy_observer_unittest.cc | 6 +- .../network_configuration_updater_unittest.cc | 3 +- .../policy/chrome_browser_policy_connector.cc | 28 +++++---- .../policy/chrome_browser_policy_connector.h | 12 +--- chrome/browser/policy/profile_policy_connector.cc | 5 +- .../policy/profile_policy_connector_factory.cc | 3 +- chrome/browser/prefs/proxy_policy_unittest.cc | 3 +- chrome/test/base/testing_profile.cc | 3 +- .../core/browser/browser_policy_connector_base.cc | 67 +++++++++------------- .../core/browser/browser_policy_connector_base.h | 19 +++--- .../configuration_policy_pref_store_test.cc | 3 +- .../core/browser/proxy_policy_handler_unittest.cc | 3 +- .../policy/core/common/policy_service_impl.cc | 50 ++++++---------- .../policy/core/common/policy_service_impl.h | 18 ++---- .../core/common/policy_service_impl_unittest.cc | 6 +- remoting/host/policy_watcher.cc | 3 +- 21 files changed, 113 insertions(+), 159 deletions(-) diff --git a/android_webview/browser/aw_browser_policy_connector.cc b/android_webview/browser/aw_browser_policy_connector.cc index df0be7eaa5e2..523da4a49146 100644 --- a/android_webview/browser/aw_browser_policy_connector.cc +++ b/android_webview/browser/aw_browser_policy_connector.cc @@ -62,14 +62,17 @@ std::unique_ptr BuildHandlerList( } // namespace AwBrowserPolicyConnector::AwBrowserPolicyConnector() - : BrowserPolicyConnectorBase(base::Bind(&BuildHandlerList)) { + : BrowserPolicyConnectorBase(base::Bind(&BuildHandlerList)) {} + +AwBrowserPolicyConnector::~AwBrowserPolicyConnector() = default; + +std::vector> +AwBrowserPolicyConnector::CreatePolicyProviders() { std::vector> providers; providers.push_back( std::make_unique( GetSchemaRegistry())); - SetPolicyProviders(std::move(providers)); + return providers; } -AwBrowserPolicyConnector::~AwBrowserPolicyConnector() {} - } // namespace android_webview diff --git a/android_webview/browser/aw_browser_policy_connector.h b/android_webview/browser/aw_browser_policy_connector.h index 4530657c5150..65b2cce56f69 100644 --- a/android_webview/browser/aw_browser_policy_connector.h +++ b/android_webview/browser/aw_browser_policy_connector.h @@ -13,12 +13,17 @@ namespace android_webview { // Sets up and keeps the browser-global policy objects such as the PolicyService // and the platform-specific PolicyProvider. class AwBrowserPolicyConnector : public policy::BrowserPolicyConnectorBase { -public: - AwBrowserPolicyConnector(); - ~AwBrowserPolicyConnector() override; + public: + AwBrowserPolicyConnector(); + ~AwBrowserPolicyConnector() override; -private: - DISALLOW_COPY_AND_ASSIGN(AwBrowserPolicyConnector); + protected: + // policy::BrowserPolicyConnectorBase: + std::vector> + CreatePolicyProviders() override; + + private: + DISALLOW_COPY_AND_ASSIGN(AwBrowserPolicyConnector); }; } // namespace android_webview diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc index d96fa3319a81..a7b5b1323b0e 100644 --- a/chrome/browser/browser_process_impl.cc +++ b/chrome/browser/browser_process_impl.cc @@ -650,7 +650,6 @@ BrowserProcessImpl::browser_policy_connector() { DCHECK(!browser_policy_connector_); browser_policy_connector_ = platform_part_->CreateBrowserPolicyConnector(); created_browser_policy_connector_ = true; - browser_policy_connector_->InitPolicyProviders(); } return browser_policy_connector_.get(); } diff --git a/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc b/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc index 809baa402cdc..15e9a3841e96 100644 --- a/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc +++ b/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc @@ -355,11 +355,13 @@ void BrowserPolicyConnectorChromeOS::OnDeviceCloudPolicyManagerDisconnected() { RestartDeviceCloudPolicyInitializer(); } -void BrowserPolicyConnectorChromeOS::BuildPolicyProviders( - std::vector>* providers) { +std::vector> +BrowserPolicyConnectorChromeOS::CreatePolicyProviders() { + auto providers = ChromeBrowserPolicyConnector::CreatePolicyProviders(); for (auto& provider_ptr : providers_for_init_) - providers->push_back(std::move(provider_ptr)); + providers.push_back(std::move(provider_ptr)); providers_for_init_.clear(); + return providers; } void BrowserPolicyConnectorChromeOS::SetTimezoneIfPolicyAvailable() { diff --git a/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h b/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h index d79f700a37f9..c72c00cbba4d 100644 --- a/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h +++ b/chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h @@ -180,9 +180,8 @@ class BrowserPolicyConnectorChromeOS protected: // ChromeBrowserPolicyConnector: - void BuildPolicyProviders( - std::vector>* providers) - override; + std::vector> + CreatePolicyProviders() override; private: // Set the timezone as soon as the policies are available. diff --git a/chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc b/chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc index 70da940dae38..40465dc207e9 100644 --- a/chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc +++ b/chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc @@ -334,8 +334,7 @@ void CloudExternalDataPolicyObserverTest::LogInAsDeviceLocalAccount( providers.push_back(device_local_account_policy_provider_.get()); TestingProfile::Builder builder; std::unique_ptr policy_service = - std::make_unique(); - policy_service->SetProviders(providers); + std::make_unique(std::move(providers)); builder.SetPolicyService(std::move(policy_service)); builder.SetPath(chromeos::ProfileHelper::Get()->GetProfilePathByUserIdHash( chromeos::ProfileHelper::GetUserIdHashByUserIdForTesting( @@ -370,8 +369,7 @@ void CloudExternalDataPolicyObserverTest::LogInAsRegularUser() { providers.push_back(&user_policy_provider_); TestingProfile::Builder builder; std::unique_ptr policy_service = - std::make_unique(); - policy_service->SetProviders(providers); + std::make_unique(std::move(providers)); builder.SetPolicyService(std::move(policy_service)); builder.SetPath(chromeos::ProfileHelper::Get()->GetProfilePathByUserIdHash( chromeos::ProfileHelper::GetUserIdHashByUserIdForTesting( diff --git a/chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc b/chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc index 6dfce500dacc..a184fd895368 100644 --- a/chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc +++ b/chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc @@ -220,8 +220,7 @@ class NetworkConfigurationUpdaterTest : public testing::Test { provider_.Init(); PolicyServiceImpl::Providers providers; providers.push_back(&provider_); - policy_service_ = std::make_unique(); - policy_service_->SetProviders(providers); + policy_service_ = std::make_unique(std::move(providers)); std::unique_ptr fake_toplevel_onc = chromeos::onc::ReadDictionaryFromJson(kFakeONC); diff --git a/chrome/browser/policy/chrome_browser_policy_connector.cc b/chrome/browser/policy/chrome_browser_policy_connector.cc index 4facd0f2fee5..683e9ad3a267 100644 --- a/chrome/browser/policy/chrome_browser_policy_connector.cc +++ b/chrome/browser/policy/chrome_browser_policy_connector.cc @@ -71,18 +71,6 @@ void ChromeBrowserPolicyConnector::OnResourceBundleCreated() { BrowserPolicyConnectorBase::OnResourceBundleCreated(); } -void ChromeBrowserPolicyConnector::InitPolicyProviders() { - std::vector> providers; - std::unique_ptr platform_provider = - CreatePlatformProvider(); - if (platform_provider) { - platform_provider_ = platform_provider.get(); - providers.push_back(std::move(platform_provider)); - } - BuildPolicyProviders(&providers); - SetPolicyProviders(std::move(providers)); -} - void ChromeBrowserPolicyConnector::Init( PrefService* local_state, scoped_refptr request_context) { @@ -104,6 +92,19 @@ ChromeBrowserPolicyConnector::GetPlatformProvider() { return provider ? provider : platform_provider_; } +std::vector> +ChromeBrowserPolicyConnector::CreatePolicyProviders() { + auto providers = BrowserPolicyConnector::CreatePolicyProviders(); + std::unique_ptr platform_provider = + CreatePlatformProvider(); + if (platform_provider) { + platform_provider_ = platform_provider.get(); + // PlatformProvider should be before all other providers (highest priority). + providers.insert(providers.begin(), std::move(platform_provider)); + } + return providers; +} + std::unique_ptr ChromeBrowserPolicyConnector::CreatePlatformProvider() { #if defined(OS_WIN) @@ -140,7 +141,4 @@ ChromeBrowserPolicyConnector::CreatePlatformProvider() { #endif } -void ChromeBrowserPolicyConnector::BuildPolicyProviders( - std::vector>* providers) {} - } // namespace policy diff --git a/chrome/browser/policy/chrome_browser_policy_connector.h b/chrome/browser/policy/chrome_browser_policy_connector.h index 14f1ddfae882..5b21e20fd5b3 100644 --- a/chrome/browser/policy/chrome_browser_policy_connector.h +++ b/chrome/browser/policy/chrome_browser_policy_connector.h @@ -42,9 +42,6 @@ class ChromeBrowserPolicyConnector : public BrowserPolicyConnector { // class to notify observers. void OnResourceBundleCreated(); - // TODO(sky): remove. Temporary until resolve ordering. - void InitPolicyProviders(); - void Init( PrefService* local_state, scoped_refptr request_context) override; @@ -52,12 +49,9 @@ class ChromeBrowserPolicyConnector : public BrowserPolicyConnector { ConfigurationPolicyProvider* GetPlatformProvider(); protected: - // Called from Init() to build the list of ConfigurationPolicyProviders that - // is supplied to SetPolicyProviders(). This implementation does nothing - // and is provided for subclasses. NOTE: |providers| may already contain - // some providers, generally subclasses should append. - virtual void BuildPolicyProviders( - std::vector>* providers); + // BrowserPolicyConnector: + std::vector> + CreatePolicyProviders() override; private: std::unique_ptr CreatePlatformProvider(); diff --git a/chrome/browser/policy/profile_policy_connector.cc b/chrome/browser/policy/profile_policy_connector.cc index 1ca34bab0e5f..de4286dcdbf0 100644 --- a/chrome/browser/policy/profile_policy_connector.cc +++ b/chrome/browser/policy/profile_policy_connector.cc @@ -104,10 +104,7 @@ void ProfilePolicyConnector::Init( } #endif - std::unique_ptr policy_service = - std::make_unique(); - policy_service->SetProviders(policy_providers_); - policy_service_ = std::move(policy_service); + policy_service_ = std::make_unique(policy_providers_); #if defined(OS_CHROMEOS) if (is_primary_user_) { diff --git a/chrome/browser/policy/profile_policy_connector_factory.cc b/chrome/browser/policy/profile_policy_connector_factory.cc index e96938ca1ea7..e8a5f0814d34 100644 --- a/chrome/browser/policy/profile_policy_connector_factory.cc +++ b/chrome/browser/policy/profile_policy_connector_factory.cc @@ -154,8 +154,7 @@ ProfilePolicyConnectorFactory::CreateForBrowserContextInternal( providers.push_back(test_providers_.front()); test_providers_.pop_front(); std::unique_ptr service = - std::make_unique(); - service->SetProviders(providers); + std::make_unique(std::move(providers)); connector->InitForTesting(std::move(service)); } diff --git a/chrome/browser/prefs/proxy_policy_unittest.cc b/chrome/browser/prefs/proxy_policy_unittest.cc index 4ed5293deeab..f387aa4640ad 100644 --- a/chrome/browser/prefs/proxy_policy_unittest.cc +++ b/chrome/browser/prefs/proxy_policy_unittest.cc @@ -98,8 +98,7 @@ class ProxyPolicyTest : public testing::Test { PolicyServiceImpl::Providers providers; providers.push_back(&provider_); - policy_service_ = std::make_unique(); - policy_service_->SetProviders(providers); + policy_service_ = std::make_unique(std::move(providers)); provider_.Init(); } diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc index 19a9b0e57d09..429b1cfa6d11 100644 --- a/chrome/test/base/testing_profile.cc +++ b/chrome/test/base/testing_profile.cc @@ -800,8 +800,7 @@ void TestingProfile::CreateProfilePolicyConnector() { if (!policy_service_) { std::vector providers; std::unique_ptr policy_service = - std::make_unique(); - policy_service->SetProviders(providers); + std::make_unique(std::move(providers)); policy_service_ = std::move(policy_service); } profile_policy_connector_.reset(new policy::ProfilePolicyConnector()); diff --git a/components/policy/core/browser/browser_policy_connector_base.cc b/components/policy/core/browser/browser_policy_connector_base.cc index cc9625f4f8af..ccac08072846 100644 --- a/components/policy/core/browser/browser_policy_connector_base.cc +++ b/components/policy/core/browser/browser_policy_connector_base.cc @@ -26,8 +26,7 @@ ConfigurationPolicyProvider* g_testing_provider = nullptr; } // namespace BrowserPolicyConnectorBase::BrowserPolicyConnectorBase( - const HandlerListFactory& handler_list_factory) - : is_initialized_(false) { + const HandlerListFactory& handler_list_factory) { // GetPolicyService() must be ready after the constructor is done. // The connector is created very early during startup, when the browser // threads aren't running yet; initialize components that need local_state, @@ -56,10 +55,8 @@ void BrowserPolicyConnectorBase::Shutdown() { is_initialized_ = false; if (g_testing_provider) g_testing_provider->Shutdown(); - if (policy_providers_) { - for (const auto& provider : *policy_providers_) - provider->Shutdown(); - } + for (const auto& provider : policy_providers_) + provider->Shutdown(); // Drop g_testing_provider so that tests executed with --single_process can // call SetPolicyProviderForTesting() again. It is still owned by the test. g_testing_provider = nullptr; @@ -75,12 +72,23 @@ CombinedSchemaRegistry* BrowserPolicyConnectorBase::GetSchemaRegistry() { } PolicyService* BrowserPolicyConnectorBase::GetPolicyService() { - if (!policy_service_) { - g_created_policy_service = true; - policy_service_ = std::make_unique(); - if (policy_providers_ || g_testing_provider) - policy_service_->SetProviders(GetProvidersForPolicyService()); - } + if (policy_service_) + return policy_service_.get(); + + DCHECK(!is_initialized_); + is_initialized_ = true; + + policy_providers_ = CreatePolicyProviders(); + + if (g_testing_provider) + g_testing_provider->Init(GetSchemaRegistry()); + + for (const auto& provider : policy_providers_) + provider->Init(GetSchemaRegistry()); + + g_created_policy_service = true; + policy_service_ = + std::make_unique(GetProvidersForPolicyService()); return policy_service_.get(); } @@ -111,32 +119,6 @@ BrowserPolicyConnectorBase::GetPolicyProviderForTesting() { return g_testing_provider; } -void BrowserPolicyConnectorBase::SetPolicyProviders( - std::vector> providers) { - // SetPolicyProviders() should only called once. - DCHECK(!is_initialized_); - policy_providers_ = std::move(providers); - - if (g_testing_provider) - g_testing_provider->Init(GetSchemaRegistry()); - - for (const auto& provider : *policy_providers_) - provider->Init(GetSchemaRegistry()); - - is_initialized_ = true; - - if (policy_service_) { - if (!policy_service_->has_providers()) { - policy_service_->SetProviders(GetProvidersForPolicyService()); - } else { - // GetPolicyService() triggers calling SetProviders() if - // |g_testing_provider| has been set. That's the only way that should - // result in ending up in this branch. - DCHECK(g_testing_provider); - } - } -} - std::vector BrowserPolicyConnectorBase::GetProvidersForPolicyService() { std::vector providers; @@ -144,12 +126,17 @@ BrowserPolicyConnectorBase::GetProvidersForPolicyService() { providers.push_back(g_testing_provider); return providers; } - providers.reserve(policy_providers_->size()); - for (const auto& policy : *policy_providers_) + providers.reserve(policy_providers_.size()); + for (const auto& policy : policy_providers_) providers.push_back(policy.get()); return providers; } +std::vector> +BrowserPolicyConnectorBase::CreatePolicyProviders() { + return {}; +} + void BrowserPolicyConnectorBase::OnResourceBundleCreated() { std::vector resource_bundle_callbacks; std::swap(resource_bundle_callbacks, resource_bundle_callbacks_); diff --git a/components/policy/core/browser/browser_policy_connector_base.h b/components/policy/core/browser/browser_policy_connector_base.h index a7674b55cdf4..d2d05f788c5f 100644 --- a/components/policy/core/browser/browser_policy_connector_base.h +++ b/components/policy/core/browser/browser_policy_connector_base.h @@ -10,7 +10,6 @@ #include "base/callback_forward.h" #include "base/macros.h" -#include "base/optional.h" #include "components/policy/core/browser/configuration_policy_handler_list.h" #include "components/policy/core/common/schema.h" #include "components/policy/core/common/schema_registry.h" @@ -73,10 +72,11 @@ class POLICY_EXPORT BrowserPolicyConnectorBase { explicit BrowserPolicyConnectorBase( const HandlerListFactory& handler_list_factory); - // Sets the set of providers, in decreasing order of priority. May only be - // called once. - void SetPolicyProviders( - std::vector> providers); + // Called from GetPolicyService() to create the set of + // ConfigurationPolicyProviders that are used, in decreasing order of + // priority. + virtual std::vector> + CreatePolicyProviders(); // Must be called when ui::ResourceBundle has been loaded, results in running // any callbacks scheduled in NotifyWhenResourceBundleReady(). @@ -88,8 +88,10 @@ class POLICY_EXPORT BrowserPolicyConnectorBase { // called. std::vector GetProvidersForPolicyService(); - // Whether SetPolicyProviders() but not Shutdown() has been invoked. - bool is_initialized_; + // Set to true when the PolicyService has been created, and false in + // Shutdown(). Once created the PolicyService is destroyed in the destructor, + // not Shutdown(). + bool is_initialized_ = false; // Used to convert policies to preferences. The providers declared below // may trigger policy updates during shutdown, which will result in @@ -105,8 +107,7 @@ class POLICY_EXPORT BrowserPolicyConnectorBase { CombinedSchemaRegistry schema_registry_; // The browser-global policy providers, in decreasing order of priority. - base::Optional>> - policy_providers_; + std::vector> policy_providers_; // Must be deleted before all the policy providers. std::unique_ptr policy_service_; diff --git a/components/policy/core/browser/configuration_policy_pref_store_test.cc b/components/policy/core/browser/configuration_policy_pref_store_test.cc index fbdc51e938a6..efd4559a95a3 100644 --- a/components/policy/core/browser/configuration_policy_pref_store_test.cc +++ b/components/policy/core/browser/configuration_policy_pref_store_test.cc @@ -30,8 +30,7 @@ ConfigurationPolicyPrefStoreTest::ConfigurationPolicyPrefStoreTest() .WillRepeatedly(Return(false)); provider_.Init(); providers_.push_back(&provider_); - policy_service_ = std::make_unique(); - policy_service_->SetProviders(providers_); + policy_service_ = std::make_unique(providers_); store_ = new ConfigurationPolicyPrefStore( nullptr, policy_service_.get(), &handler_list_, POLICY_LEVEL_MANDATORY); } diff --git a/components/policy/core/browser/proxy_policy_handler_unittest.cc b/components/policy/core/browser/proxy_policy_handler_unittest.cc index 8ee359336b62..6bd9bd54c8dc 100644 --- a/components/policy/core/browser/proxy_policy_handler_unittest.cc +++ b/components/policy/core/browser/proxy_policy_handler_unittest.cc @@ -32,8 +32,7 @@ class ProxyPolicyHandlerTest // preprocessor. The previous store must be nulled out first so that it // removes itself from the service's observer list. store_ = nullptr; - policy_service_ = std::make_unique(); - policy_service_->SetProviders(providers_); + policy_service_ = std::make_unique(providers_); store_ = new ConfigurationPolicyPrefStore( nullptr, policy_service_.get(), &handler_list_, POLICY_LEVEL_MANDATORY); } diff --git a/components/policy/core/common/policy_service_impl.cc b/components/policy/core/common/policy_service_impl.cc index 42b6138c853c..dd555a14634e 100644 --- a/components/policy/core/common/policy_service_impl.cc +++ b/components/policy/core/common/policy_service_impl.cc @@ -72,25 +72,12 @@ void RemapProxyPolicies(PolicyMap* policies) { } // namespace -PolicyServiceImpl::PolicyServiceImpl() : update_task_ptr_factory_(this) { - for (int domain = 0; domain < POLICY_DOMAIN_SIZE; ++domain) - initialization_complete_[domain] = false; -} - -PolicyServiceImpl::~PolicyServiceImpl() { - DCHECK(thread_checker_.CalledOnValidThread()); - if (providers_) { - for (auto* provider : *providers_) - provider->RemoveObserver(this); - } -} - -void PolicyServiceImpl::SetProviders(Providers providers) { - DCHECK(!providers_); +PolicyServiceImpl::PolicyServiceImpl(Providers providers) + : update_task_ptr_factory_(this) { providers_ = std::move(providers); for (int domain = 0; domain < POLICY_DOMAIN_SIZE; ++domain) initialization_complete_[domain] = true; - for (auto* provider : *providers_) { + for (auto* provider : providers_) { provider->AddObserver(this); for (int domain = 0; domain < POLICY_DOMAIN_SIZE; ++domain) { initialization_complete_[domain] &= @@ -102,6 +89,12 @@ void PolicyServiceImpl::SetProviders(Providers providers) { MergeAndTriggerUpdates(); } +PolicyServiceImpl::~PolicyServiceImpl() { + DCHECK(thread_checker_.CalledOnValidThread()); + for (auto* provider : providers_) + provider->RemoveObserver(this); +} + void PolicyServiceImpl::AddObserver(PolicyDomain domain, PolicyService::Observer* observer) { DCHECK(thread_checker_.CalledOnValidThread()); @@ -143,7 +136,7 @@ void PolicyServiceImpl::RefreshPolicies(const base::Closure& callback) { if (!callback.is_null()) refresh_callbacks_.push_back(callback); - if (!providers_ || providers_->empty()) { + if (providers_.empty()) { // Refresh is immediately complete if there are no providers. See the note // on OnUpdatePolicy() about why this is a posted task. update_task_ptr_factory_.InvalidateWeakPtrs(); @@ -153,15 +146,15 @@ void PolicyServiceImpl::RefreshPolicies(const base::Closure& callback) { } else { // Some providers might invoke OnUpdatePolicy synchronously while handling // RefreshPolicies. Mark all as pending before refreshing. - for (auto* provider : *providers_) + for (auto* provider : providers_) refresh_pending_.insert(provider); - for (auto* provider : *providers_) + for (auto* provider : providers_) provider->RefreshPolicies(); } } void PolicyServiceImpl::OnUpdatePolicy(ConfigurationPolicyProvider* provider) { - DCHECK_EQ(1, std::count(providers_->begin(), providers_->end(), provider)); + DCHECK_EQ(1, std::count(providers_.begin(), providers_.end(), provider)); refresh_pending_.erase(provider); // Note: a policy change may trigger further policy changes in some providers. @@ -194,13 +187,11 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() { // Merge from each provider in their order of priority. const PolicyNamespace chrome_namespace(POLICY_DOMAIN_CHROME, std::string()); PolicyBundle bundle; - if (providers_) { - for (auto* provider : *providers_) { - PolicyBundle provided_bundle; - provided_bundle.CopyFrom(provider->policies()); - RemapProxyPolicies(&provided_bundle.Get(chrome_namespace)); - bundle.MergeFrom(provided_bundle); - } + for (auto* provider : providers_) { + PolicyBundle provided_bundle; + provided_bundle.CopyFrom(provider->policies()); + RemapProxyPolicies(&provided_bundle.Get(chrome_namespace)); + bundle.MergeFrom(provided_bundle); } // Swap first, so that observers that call GetPolicies() see the current @@ -247,9 +238,6 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() { void PolicyServiceImpl::CheckInitializationComplete() { DCHECK(thread_checker_.CalledOnValidThread()); - if (!providers_) - return; - // Check if all the providers just became initialized for each domain; if so, // notify that domain's observers. for (int domain = 0; domain < POLICY_DOMAIN_SIZE; ++domain) { @@ -259,7 +247,7 @@ void PolicyServiceImpl::CheckInitializationComplete() { PolicyDomain policy_domain = static_cast(domain); bool all_complete = true; - for (auto* provider : *providers_) { + for (auto* provider : providers_) { if (!provider->IsInitializationComplete(policy_domain)) { all_complete = false; break; diff --git a/components/policy/core/common/policy_service_impl.h b/components/policy/core/common/policy_service_impl.h index 0e6003e87b89..985b27e257f1 100644 --- a/components/policy/core/common/policy_service_impl.h +++ b/components/policy/core/common/policy_service_impl.h @@ -15,7 +15,6 @@ #include "base/macros.h" #include "base/memory/weak_ptr.h" #include "base/observer_list.h" -#include "base/optional.h" #include "base/threading/thread_checker.h" #include "components/policy/core/common/configuration_policy_provider.h" #include "components/policy/core/common/policy_bundle.h" @@ -32,18 +31,11 @@ class POLICY_EXPORT PolicyServiceImpl public: using Providers = std::vector; - // Creates a new PolicyServiceImpl, it is expected SetProviders() is called - // once to complete initialization. - PolicyServiceImpl(); - + // Creates a new PolicyServiceImpl with the list of + // ConfigurationPolicyProviders, in order of decreasing priority. + explicit PolicyServiceImpl(Providers providers); ~PolicyServiceImpl() override; - // Sets the providers; see description of constructor for details. - void SetProviders(Providers providers); - - // Returns true if SetProviders() was called. - bool has_providers() const { return providers_.has_value(); } - // PolicyService overrides: void AddObserver(PolicyDomain domain, PolicyService::Observer* observer) override; @@ -76,8 +68,8 @@ class POLICY_EXPORT PolicyServiceImpl // Invokes all the refresh callbacks if there are no more refreshes pending. void CheckRefreshComplete(); - // The providers set via SetProviders(), in order of decreasing priority. - base::Optional providers_; + // The providers, in order of decreasing priority. + Providers providers_; // Maps each policy namespace to its current policies. PolicyBundle policy_bundle_; diff --git a/components/policy/core/common/policy_service_impl_unittest.cc b/components/policy/core/common/policy_service_impl_unittest.cc index 857e38226e83..bc84baa9d694 100644 --- a/components/policy/core/common/policy_service_impl_unittest.cc +++ b/components/policy/core/common/policy_service_impl_unittest.cc @@ -120,8 +120,7 @@ class PolicyServiceTest : public testing::Test { providers.push_back(&provider0_); providers.push_back(&provider1_); providers.push_back(&provider2_); - policy_service_ = std::make_unique(); - policy_service_->SetProviders(providers); + policy_service_ = std::make_unique(std::move(providers)); } void TearDown() override { @@ -561,8 +560,7 @@ TEST_F(PolicyServiceTest, IsInitializationComplete) { providers.push_back(&provider0_); providers.push_back(&provider1_); providers.push_back(&provider2_); - policy_service_ = std::make_unique(); - policy_service_->SetProviders(providers); + policy_service_ = std::make_unique(std::move(providers)); EXPECT_FALSE(policy_service_->IsInitializationComplete(POLICY_DOMAIN_CHROME)); EXPECT_FALSE( policy_service_->IsInitializationComplete(POLICY_DOMAIN_EXTENSIONS)); diff --git a/remoting/host/policy_watcher.cc b/remoting/host/policy_watcher.cc index 297dcd7b2a59..c428bad430f2 100644 --- a/remoting/host/policy_watcher.cc +++ b/remoting/host/policy_watcher.cc @@ -376,8 +376,7 @@ std::unique_ptr PolicyWatcher::CreateFromPolicyLoader( policy::PolicyServiceImpl::Providers providers; providers.push_back(policy_provider.get()); std::unique_ptr policy_service = - std::make_unique(); - policy_service->SetProviders(providers); + std::make_unique(std::move(providers)); policy::PolicyService* borrowed_policy_service = policy_service.get(); return base::WrapUnique(new PolicyWatcher( -- 2.16.2