Relanding http://codereview.chromium.org/10377115/ after some refactorings.

Moved the NetworkConfigurationUpdater to chrome_browser_main_chromeos.

BUG=chromium-os:31112
TEST=Nothing breaks, ONC still works


Review URL: https://chromiumcodereview.appspot.com/10409053

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@138524 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 82 insertions, 58 deletions

diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index a7256b0..abfe9e10 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -434,11 +434,13 @@ policy::BrowserPolicyConnector* BrowserProcessImpl::browser_policy_connector() {
   DCHECK(CalledOnValidThread());
   if (!created_browser_policy_connector_) {
     DCHECK(browser_policy_connector_.get() == NULL);
-    created_browser_policy_connector_ = true;
 #if defined(ENABLE_CONFIGURATION_POLICY)
     browser_policy_connector_.reset(new policy::BrowserPolicyConnector());
     browser_policy_connector_->Init();
 #endif
+    // Init() should not reenter this function. Updating
+    // |created_browser_policy_connector_| here makes reentering hit the DCHECK.
+    created_browser_policy_connector_ = true;
   }
   return browser_policy_connector_.get();
 }
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
index 854d60e..d2e4f9e 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -57,6 +57,7 @@
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/net/chrome_network_delegate.h"
 #include "chrome/browser/policy/browser_policy_connector.h"
+#include "chrome/browser/policy/network_configuration_updater.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -390,6 +391,13 @@ void ChromeBrowserMainPartsChromeos::PostProfileInit() {
       profile()->GetPrefs()->SetBoolean(prefs::kUseSharedProxies, false);
   }
 
+  if (parsed_command_line().HasSwitch(switches::kEnableONCPolicy)) {
+    network_config_updater_.reset(
+        new policy::NetworkConfigurationUpdater(
+            g_browser_process->policy_service(),
+            chromeos::CrosLibrary::Get()->GetNetworkLibrary()));
+  }
+
   // Tests should be able to tune login manager before showing it.
   // Thus only show login manager in normal (non-testing) mode.
   if (!parameters().ui_task)
@@ -497,6 +505,10 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
   power_button_observer_.reset();
   screen_dimming_observer_.reset();
 
+  // Delete the NetworkConfigurationUpdater while |g_browser_process| is still
+  // alive.
+  network_config_updater_.reset();
+
   ChromeBrowserMainPartsLinux::PostMainMessageLoopRun();
 }
 
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.h b/chrome/browser/chromeos/chrome_browser_main_chromeos.h
index a0d62fe..a5b076b 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.h
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.h
@@ -19,6 +19,10 @@ class SessionManagerObserver;
 class VideoPropertyWriter;
 }  // namespace chromeos
 
+namespace policy {
+class NetworkConfigurationUpdater;
+}  // namespace policy
+
 class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
  public:
   explicit ChromeBrowserMainPartsChromeos(
@@ -52,6 +56,7 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
   scoped_ptr<chromeos::PowerStateOverride> power_state_override_;
   scoped_ptr<chromeos::VideoPropertyWriter> video_property_writer_;
   scoped_ptr<chromeos::ScreenDimmingObserver> screen_dimming_observer_;
+  scoped_ptr<policy::NetworkConfigurationUpdater> network_config_updater_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsChromeos);
 };
diff --git a/chrome/browser/policy/browser_policy_connector.cc b/chrome/browser/policy/browser_policy_connector.cc
index 9d38468..2b9aa22 100644
--- a/chrome/browser/policy/browser_policy_connector.cc
+++ b/chrome/browser/policy/browser_policy_connector.cc
@@ -41,7 +41,6 @@
 #include "chrome/browser/policy/app_pack_updater.h"
 #include "chrome/browser/policy/cros_user_policy_cache.h"
 #include "chrome/browser/policy/device_policy_cache.h"
-#include "chrome/browser/policy/network_configuration_updater.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #endif
 
@@ -141,13 +140,6 @@ void BrowserPolicyConnector::Init() {
 
   InitializeDevicePolicy();
 
-  if (command_line->HasSwitch(switches::kEnableONCPolicy)) {
-    network_configuration_updater_.reset(
-        new NetworkConfigurationUpdater(
-            managed_cloud_provider_.get(),
-            chromeos::CrosLibrary::Get()->GetNetworkLibrary()));
-  }
-
   // Create the AppPackUpdater to start updating the cache. It requires the
   // system request context, which isn't available yet; therefore it is
   // created only once the loops are running.
diff --git a/chrome/browser/policy/browser_policy_connector.h b/chrome/browser/policy/browser_policy_connector.h
index 7408a5d..2781a63 100644
--- a/chrome/browser/policy/browser_policy_connector.h
+++ b/chrome/browser/policy/browser_policy_connector.h
@@ -26,9 +26,7 @@ class CloudPolicyDataStore;
 class CloudPolicyProvider;
 class CloudPolicySubsystem;
 class ConfigurationPolicyProvider;
-class NetworkConfigurationUpdater;
 class PolicyService;
-class PolicyServiceImpl;
 class UserPolicyTokenCache;
 
 // Manages the lifecycle of browser-global policy infrastructure, such as the
@@ -163,8 +161,6 @@ class BrowserPolicyConnector : public content::NotificationObserver {
   scoped_ptr<CloudPolicyDataStore> device_data_store_;
   scoped_ptr<CloudPolicySubsystem> device_cloud_policy_subsystem_;
   scoped_ptr<EnterpriseInstallAttributes> install_attributes_;
-
-  scoped_ptr<NetworkConfigurationUpdater> network_configuration_updater_;
 #endif
 
   scoped_ptr<UserPolicyTokenCache> user_policy_token_cache_;
diff --git a/chrome/browser/policy/network_configuration_updater.cc b/chrome/browser/policy/network_configuration_updater.cc
index fba0e2e4..dcc2ead 100644
--- a/chrome/browser/policy/network_configuration_updater.cc
+++ b/chrome/browser/policy/network_configuration_updater.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/policy/network_configuration_updater.h"
 
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/policy/policy_map.h"
 #include "policy/policy_constants.h"
@@ -14,44 +18,52 @@ const char NetworkConfigurationUpdater::kEmptyConfiguration[] =
     "{\"NetworkConfigurations\":[],\"Certificates\":[]}";
 
 NetworkConfigurationUpdater::NetworkConfigurationUpdater(
-    ConfigurationPolicyProvider* provider,
+    PolicyService* policy_service,
     chromeos::NetworkLibrary* network_library)
-    : network_library_(network_library) {
+    : policy_change_registrar_(
+          policy_service, POLICY_DOMAIN_CHROME, std::string()),
+      network_library_(network_library) {
   DCHECK(network_library_);
-  provider_registrar_.Init(provider, this);
-  Update();
-}
-
-NetworkConfigurationUpdater::~NetworkConfigurationUpdater() {}
+  policy_change_registrar_.Observe(
+      key::kDeviceOpenNetworkConfiguration,
+      base::Bind(&NetworkConfigurationUpdater::ApplyNetworkConfiguration,
+                 base::Unretained(this),
+                 chromeos::NetworkUIData::ONC_SOURCE_DEVICE_POLICY,
+                 &device_network_config_));
+  policy_change_registrar_.Observe(
+      key::kOpenNetworkConfiguration,
+      base::Bind(&NetworkConfigurationUpdater::ApplyNetworkConfiguration,
+                 base::Unretained(this),
+                 chromeos::NetworkUIData::ONC_SOURCE_USER_POLICY,
+                 &user_network_config_));
 
-void NetworkConfigurationUpdater::OnUpdatePolicy(
-    ConfigurationPolicyProvider* provider) {
-  Update();
+  // Apply the current values immediately.
+  const PolicyMap& policies = policy_service->GetPolicies(POLICY_DOMAIN_CHROME,
+                                                          std::string());
+  ApplyNetworkConfiguration(
+      chromeos::NetworkUIData::ONC_SOURCE_DEVICE_POLICY,
+      &device_network_config_,
+      NULL,
+      policies.GetValue(key::kDeviceOpenNetworkConfiguration));
+  ApplyNetworkConfiguration(
+      chromeos::NetworkUIData::ONC_SOURCE_USER_POLICY,
+      &user_network_config_,
+      NULL,
+      policies.GetValue(key::kOpenNetworkConfiguration));
 }
 
-void NetworkConfigurationUpdater::Update() {
-  ConfigurationPolicyProvider* provider = provider_registrar_.provider();
-  const PolicyMap& policy = provider->policies().Get(POLICY_DOMAIN_CHROME, "");
-
-  ApplyNetworkConfiguration(policy, key::kDeviceOpenNetworkConfiguration,
-                            chromeos::NetworkUIData::ONC_SOURCE_DEVICE_POLICY,
-                            &device_network_config_);
-  ApplyNetworkConfiguration(policy, key::kOpenNetworkConfiguration,
-                            chromeos::NetworkUIData::ONC_SOURCE_USER_POLICY,
-                            &user_network_config_);
-}
+NetworkConfigurationUpdater::~NetworkConfigurationUpdater() {}
 
 void NetworkConfigurationUpdater::ApplyNetworkConfiguration(
-    const PolicyMap& policy_map,
-    const char* policy_name,
     chromeos::NetworkUIData::ONCSource onc_source,
-    std::string* cached_value) {
+    std::string* cached_value,
+    const base::Value* previous,
+    const base::Value* current) {
   std::string new_network_config;
-  const base::Value* value = policy_map.GetValue(policy_name);
-  if (value != NULL) {
+  if (current != NULL) {
     // If the policy is not a string, we issue a warning, but still clear the
     // network configuration.
-    if (!value->GetAsString(&new_network_config))
+    if (!current->GetAsString(&new_network_config))
       LOG(WARNING) << "Invalid network configuration.";
   }
 
diff --git a/chrome/browser/policy/network_configuration_updater.h b/chrome/browser/policy/network_configuration_updater.h
index 648e23f..4fd651c 100644
--- a/chrome/browser/policy/network_configuration_updater.h
+++ b/chrome/browser/policy/network_configuration_updater.h
@@ -9,7 +9,11 @@
 #include <string>
 
 #include "chrome/browser/chromeos/cros/network_ui_data.h"
-#include "chrome/browser/policy/configuration_policy_provider.h"
+#include "chrome/browser/policy/policy_service.h"
+
+namespace base {
+class Value;
+}
 
 namespace chromeos {
 class NetworkLibrary;
@@ -21,33 +25,26 @@ class PolicyMap;
 
 // Keeps track of the network configuration policy settings and updates the
 // network definitions whenever the configuration changes.
-class NetworkConfigurationUpdater
-    : public ConfigurationPolicyProvider::Observer {
+class NetworkConfigurationUpdater {
  public:
-  NetworkConfigurationUpdater(ConfigurationPolicyProvider* provider,
+  NetworkConfigurationUpdater(PolicyService* policy_service,
                               chromeos::NetworkLibrary* network_library);
   virtual ~NetworkConfigurationUpdater();
 
-  // ConfigurationPolicyProvider::Observer:
-  virtual void OnUpdatePolicy(ConfigurationPolicyProvider* provider) OVERRIDE;
-
   // Empty network configuration blob.
   static const char kEmptyConfiguration[];
 
  private:
-  // Grabs network configuration from policy and applies it.
-  void Update();
-
   // Extracts ONC string from |policy_map| and pushes the configuration to
   // |network_library_| if it's different from |*cached_value| (which is
   // updated).
-  void ApplyNetworkConfiguration(const PolicyMap& policy_map,
-                                 const char* policy_name,
-                                 chromeos::NetworkUIData::ONCSource onc_source,
-                                 std::string* cached_value);
+  void ApplyNetworkConfiguration(chromeos::NetworkUIData::ONCSource onc_source,
+                                 std::string* cached_value,
+                                 const base::Value* previous,
+                                 const base::Value* current);
 
-  // Wraps the provider we read network configuration from.
-  ConfigurationPolicyObserverRegistrar provider_registrar_;
+  // Wraps the policy service we read network configuration from.
+  PolicyChangeRegistrar policy_change_registrar_;
 
   // Network library to write network configuration to.
   chromeos::NetworkLibrary* network_library_;
diff --git a/chrome/browser/policy/network_configuration_updater_unittest.cc b/chrome/browser/policy/network_configuration_updater_unittest.cc
index d03ee8b..a61a1c2 100644
--- a/chrome/browser/policy/network_configuration_updater_unittest.cc
+++ b/chrome/browser/policy/network_configuration_updater_unittest.cc
@@ -4,9 +4,11 @@
 
 #include "chrome/browser/policy/network_configuration_updater.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/chromeos/cros/mock_network_library.h"
 #include "chrome/browser/policy/mock_configuration_policy_provider.h"
 #include "chrome/browser/policy/policy_map.h"
+#include "chrome/browser/policy/policy_service_impl.h"
 #include "policy/policy_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -25,6 +27,11 @@ class NetworkConfigurationUpdaterTest
   virtual void SetUp() OVERRIDE {
     EXPECT_CALL(network_library_, LoadOncNetworks(_, "", _, _))
         .WillRepeatedly(Return(true));
+    EXPECT_CALL(provider_, IsInitializationComplete())
+        .WillRepeatedly(Return(true));
+    PolicyServiceImpl::Providers providers;
+    providers.push_back(&provider_);
+    policy_service_.reset(new PolicyServiceImpl(providers));
   }
 
   // Maps configuration policy name to corresponding ONC source.
@@ -39,6 +46,7 @@ class NetworkConfigurationUpdaterTest
 
   chromeos::MockNetworkLibrary network_library_;
   MockConfigurationPolicyProvider provider_;
+  scoped_ptr<PolicyServiceImpl> policy_service_;
 };
 
 TEST_P(NetworkConfigurationUpdaterTest, InitialUpdate) {
@@ -51,12 +59,12 @@ TEST_P(NetworkConfigurationUpdaterTest, InitialUpdate) {
               LoadOncNetworks(kFakeONC, "", NameToONCSource(GetParam()), _))
       .WillOnce(Return(true));
 
-  NetworkConfigurationUpdater updater(&provider_, &network_library_);
+  NetworkConfigurationUpdater updater(policy_service_.get(), &network_library_);
   Mock::VerifyAndClearExpectations(&network_library_);
 }
 
 TEST_P(NetworkConfigurationUpdaterTest, PolicyChange) {
-  NetworkConfigurationUpdater updater(&provider_, &network_library_);
+  NetworkConfigurationUpdater updater(policy_service_.get(), &network_library_);
 
   // We should update if policy changes.
   EXPECT_CALL(network_library_,