Migrate ProxyConfigServiceImpl to NetworkStateHandler and NetworkProfileHandler.

chrome/browser/chromeos/proxy_config_service_impl.cc

Index: chrome/browser/chromeos/proxy_config_service_impl.cc
diff --git a/chrome/browser/chromeos/proxy_config_service_impl.cc b/chrome/browser/chromeos/proxy_config_service_impl.cc
index 71ba9761f9244b6cbc7e694ae0223396155520e1..774106a5caf7877be873e80b29ca8cfaef8e1d56 100644
--- a/chrome/browser/chromeos/proxy_config_service_impl.cc
+++ b/chrome/browser/chromeos/proxy_config_service_impl.cc
@@ -7,13 +7,15 @@
 #include <ostream>
 
 #include "base/bind.h"
-#include "base/json/json_string_value_serializer.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/string_util.h"
+#include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
+#include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/chromeos/cros/network_property_ui_data.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -26,6 +28,10 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/network/network_profile.h"
+#include "chromeos/network/network_profile_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_ui_data.h"
 #include "chromeos/network/onc/onc_constants.h"
 #include "components/user_prefs/pref_registry_syncable.h"
@@ -97,12 +103,18 @@ const char* ConfigStateToString(ProxyPrefs::ConfigState state) {
   return "";
 }
 
-// Returns true if proxy settings from |network| is editable.
-bool IsNetworkProxySettingsEditable(const Network* network) {
-  if (!network)
-    return true;  // editable if no network given.
-
+// Returns true if proxy settings of |network_state| are editable.
+bool IsNetworkProxySettingsEditable(const NetworkState& network_state) {
   NetworkLibrary* network_library = CrosLibrary::Get()->GetNetworkLibrary();
+  Network* network = network_library->FindNetworkByPath(network_state.path());
+
+  if (!network) {
+    LOG(ERROR) << "Could not find network " << network_state.path()
+               << " in NetworkLibrary. Falling back to user-editable proxy"
+               << " settings";
+    return true;
+  }

[stevenjb, 2013/05/08 20:32:09]

Mixing NetworkLibrary and NetworkStateHandler is prone to all sorts of bad edge
cases; we should avoid it.

Here it looks like we only need unique_id and ui_data.

So far I have avoided adding UIData to NetworkState, but I don't have any
particular objections to adding it if it makes code like this easier (otherwise
we would have to request it asynchronously which might be a pain).

The unique_id logic could easily be added to
NetworkState::InitialPropertiesReceived(). I suspect that we can just store it
in guid_, but we can add another member if that is not the case (or we are
unsure).

[pneubeck (no reviews), 2013/05/10 10:58:52]

I removed the dependency of NetworkPropertyUIData on NetworkUIData. And
unique_id in the context of policy is actually guid.

We cannot remove the network_library->FindOncForNetwork() call for now, as
policies are still applied by NetworkLibrary.

I could switch over to the new code for policy application, but that would
require to replace all calls to network_library->FindOncForNetwork in the old UI
code by some call to ManagedNetworkConfigHandler. It's not much to change, but
maybe we don't want to have that dependency?

I thought about that step previously. But I'm not sure if that's worth it.

+
   const base::DictionaryValue* onc =
        network_library->FindOncForNetwork(network->unique_id());
 
@@ -239,7 +251,8 @@ bool ProxyConfigServiceImpl::ProxyConfig::FromNetProxyConfig(
   return false;
 }
 
-DictionaryValue* ProxyConfigServiceImpl::ProxyConfig::ToPrefProxyConfig() {
+base::DictionaryValue*
+    ProxyConfigServiceImpl::ProxyConfig::ToPrefProxyConfig() {
   switch (mode) {
     case MODE_DIRECT: {
       return ProxyConfigDictionary::CreateDirect();
@@ -353,7 +366,7 @@ bool ProxyConfigServiceImpl::ProxyConfig::SerializeForNetwork(
     return false;
 
   // Return empty string for direct mode for portal check to work correctly.
-  DictionaryValue *dict = proxy_dict_ptr.get();
+  base::DictionaryValue *dict = proxy_dict_ptr.get();
   ProxyConfigDictionary proxy_dict(dict);
   ProxyPrefs::ProxyMode mode;
   if (proxy_dict.GetMode(&mode)) {
@@ -362,8 +375,8 @@ bool ProxyConfigServiceImpl::ProxyConfig::SerializeForNetwork(
       return true;
     }
   }
-  JSONStringValueSerializer serializer(output);
-  return serializer.Serialize(*dict);
+  base::JSONWriter::Write(dict, output);
+  return true;
 }
 
 //----------- ProxyConfigServiceImpl::ProxyConfig: private methods -------------
@@ -403,43 +416,50 @@ ProxyConfigServiceImpl::ProxyConfigServiceImpl(PrefService* pref_service)
 
   FetchProxyPolicy();
 
-  // Register for shill network notifications.
-  NetworkLibrary* network_lib = CrosLibrary::Get()->GetNetworkLibrary();
-  OnActiveNetworkChanged(network_lib, network_lib->active_network());
-  network_lib->AddNetworkManagerObserver(this);
+  // Register for changes to the default network.
+  NetworkStateHandler* state_handler = NetworkStateHandler::Get();
+  state_handler->AddObserver(this);
+  DefaultNetworkChanged(state_handler->DefaultNetwork());
 }
 
 ProxyConfigServiceImpl::~ProxyConfigServiceImpl() {
-  NetworkLibrary* netlib = CrosLibrary::Get()->GetNetworkLibrary();
-  if (netlib) {
-    netlib->RemoveNetworkManagerObserver(this);
-    netlib->RemoveObserverForAllNetworks(this);
-  }
+  NetworkStateHandler::Get()->RemoveObserver(this);
 }
 
 void ProxyConfigServiceImpl::UISetCurrentNetwork(
     const std::string& current_network) {
-  Network* network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
-      current_network);
+  const NetworkState* network =
+      NetworkStateHandler::Get()->GetNetworkState(current_network);
   if (!network) {
     ResetUICache();
     LOG(ERROR) << "Can't find requested network " << current_network;
     return;
   }
   current_ui_network_ = current_network;
-  OnUISetCurrentNetwork(network);
-}
 
-void ProxyConfigServiceImpl::UIMakeActiveNetworkCurrent() {
-  Network* network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
-      active_network_);
-  if (!network) {
-    ResetUICache();
-    LOG(ERROR) << "Can't find requested network " << active_network_;
-    return;
+  DetermineEffectiveConfig(network, false);
+  VLOG(1) << "Current ui network: "
+          << network->name()
+          << ", " << ModeToString(current_ui_config_.mode)
+          << ", " << ConfigStateToString(current_ui_config_.state)
+          << ", modifiable:" << current_ui_config_.user_modifiable;
+  // Notify observers.
+  std::vector<base::Closure>::iterator iter = callbacks_.begin();
+  while (iter != callbacks_.end()) {
+    if (iter->is_null()) {
+      iter = callbacks_.erase(iter);
+    } else {
+      iter->Run();
+      ++iter;
+    }
   }
-  current_ui_network_ = active_network_;
-  OnUISetCurrentNetwork(network);
+}
+
+void ProxyConfigServiceImpl::UIMakeDefaultNetworkCurrent() {
+  const NetworkState* default_network =
+      NetworkStateHandler::Get()->DefaultNetwork();
+  UISetCurrentNetwork(
+      default_network ? default_network->path() : std::string());
 }
 
 void ProxyConfigServiceImpl::UIGetCurrentNetworkName(
@@ -463,7 +483,7 @@ void ProxyConfigServiceImpl::UIGetCurrentNetworkName(
 
 void ProxyConfigServiceImpl::UIGetProxyConfig(ProxyConfig* config) {
   // Simply returns the copy last set from UI via UISetCurrentNetwork or
-  // UIMakeActiveNetworkCurrent.
+  // UIMakeDefaultNetworkCurrent.
   *config = current_ui_config_;
 }
 
@@ -542,46 +562,16 @@ void ProxyConfigServiceImpl::OnProxyConfigChanged(
     const net::ProxyConfig& config) {
   VLOG(1) << "Got prefs change: " << ConfigStateToString(config_state)
           << ", mode=" << config.proxy_rules().type;
-  Network* network = NULL;
-  if (!active_network_.empty()) {
-    network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
-        active_network_);
-    if (!network)
-      LOG(ERROR) << "can't find requested network " << active_network_;
-  }
-  DetermineEffectiveConfig(network, true);
-}
-
-void ProxyConfigServiceImpl::OnNetworkManagerChanged(
-    NetworkLibrary* network_lib) {
-  VLOG(1) << "OnNetworkManagerChanged: use-shared-proxies="
-          << GetUseSharedProxies();
-  OnActiveNetworkChanged(network_lib, network_lib->active_network());
-}
-
-void ProxyConfigServiceImpl::OnNetworkChanged(NetworkLibrary* network_lib,
-    const Network* network) {
-  if (!network)
-    return;
-  VLOG(1) << "OnNetworkChanged: "
-          << (network->name().empty() ? network->service_path() :
-                                        network->name())
-          << ", use-shared-proxies=" << GetUseSharedProxies();
-  // We only care about active network.
-  if (network == network_lib->active_network())
-    OnActiveNetworkChanged(network_lib, network);
+  UpdateProxyConfigOfDefaultNetwork();
 }
 
 // static
-bool ProxyConfigServiceImpl::ParseProxyConfig(const Network* network,
+bool ProxyConfigServiceImpl::ParseProxyConfig(const NetworkState& network,
                                               net::ProxyConfig* proxy_config) {
-  if (!network || !proxy_config)
-    return false;
-  JSONStringValueSerializer serializer(network->proxy_config());
-  scoped_ptr<Value> value(serializer.Deserialize(NULL, NULL));
-  if (!value.get() || value->GetType() != Value::TYPE_DICTIONARY)
+  if (!proxy_config || network.proxy_config().empty())
     return false;
-  ProxyConfigDictionary proxy_dict(static_cast<DictionaryValue*>(value.get()));
+
+  ProxyConfigDictionary proxy_dict(&network.proxy_config());
   return PrefProxyConfigTrackerImpl::PrefConfigToNetConfig(proxy_dict,
                                                            proxy_config);
 }
@@ -606,17 +596,7 @@ void ProxyConfigServiceImpl::RegisterUserPrefs(
 
 void ProxyConfigServiceImpl::OnUseSharedProxiesChanged() {
   VLOG(1) << "New use-shared-proxies = " << GetUseSharedProxies();
-
-  // Determine new proxy config which may have changed because of new
-  // use-shared-proxies. If necessary, activate it.
-  Network* network = NULL;
-  if (!active_network_.empty()) {
-    network = CrosLibrary::Get()->GetNetworkLibrary()->FindNetworkByPath(
-        active_network_);
-    if (!network)
-      LOG(WARNING) << "Can't find requested network " << active_network_;
-  }
-  DetermineEffectiveConfig(network, true);
+  UpdateProxyConfigOfDefaultNetwork();
 }
 
 void ProxyConfigServiceImpl::OnUISetProxyConfig() {
@@ -632,58 +612,34 @@ void ProxyConfigServiceImpl::OnUISetProxyConfig() {
   }
 }
 
-void ProxyConfigServiceImpl::OnActiveNetworkChanged(NetworkLibrary* network_lib,
-    const Network* active_network) {
-  std::string new_network;
-  if (active_network)
-    new_network = active_network->service_path();
-
-  if (active_network_ == new_network) {  // Same active network.
-    VLOG(1) << "Same active network: "
-            << (new_network.empty() ? "empty" :
-                    (active_network->name().empty() ?
-                         new_network : active_network->name()));
-    // Even though network is the same, its proxy config (e.g. if private
-    // version of network replaces the shared version after login), or
-    // use-shared-proxies setting (e.g. after login) may have changed,
-    // so re-determine effective proxy config, and activate if different.
-    if (active_network) {
-      VLOG(1) << "Profile=" << active_network->profile_type()
-              << "," << active_network->profile_path()
-              << ", proxy=" << active_network->proxy_config();
-      DetermineEffectiveConfig(active_network, true);
-    }
-    return;
-  }
+void ProxyConfigServiceImpl::DefaultNetworkChanged(
+    const NetworkState* new_network) {
+  std::string new_network_path;
+  if (new_network)
+    new_network_path = new_network->path();
 
-  // If there was a previous active network, remove it as observer.
-  if (!active_network_.empty())
-    network_lib->RemoveNetworkObserver(active_network_, this);
+  VLOG(1) << "DefaultNetworkChanged: network "
+          << default_network_ << " -> " << new_network_path << ".";
+  VLOG_IF(1, new_network) << "New network: name=" << new_network->name()
+                          << ", proxy=" << new_network->proxy_config()
+                          << ", profile=" << new_network->profile_path();
 
-  active_network_ = new_network;
+  default_network_ = new_network_path;
 
-  if (active_network_.empty()) {
-    VLOG(1) << "New active network: empty";
-    DetermineEffectiveConfig(active_network, true);
-    return;
-  }
-
-  VLOG(1) << "New active network: path=" << active_network->service_path()
-          << ", name=" << active_network->name()
-          << ", profile=" << active_network->profile_type()
-          << "," << active_network->profile_path()
-          << ", proxy=" << active_network->proxy_config();
-
-  // Register observer for new network.
-  network_lib->AddNetworkObserver(active_network_, this);
+  // Even if the default network is the same, its proxy config (e.g. if private
+  // version of network replaces the shared version after login), or
+  // use-shared-proxies setting (e.g. after login) may have changed, so
+  // re-determine effective proxy config, and activate if different.
 
-  // If necessary, migrate config to shill.
-  if (active_network->proxy_config().empty() && !device_config_.empty()) {
-    VLOG(1) << "Try migrating device config to " << active_network_;
-    SetProxyConfigForNetwork(active_network_, device_config_, true);
+  // If necessary, migrate device proxy settings to per network settings.
+  if (new_network && new_network->proxy_config().empty() &&
+      !device_config_.empty()) {
+    VLOG(1) << "Try migrating device config to " << new_network_path;
+    SetProxyConfigForNetwork(new_network_path, device_config_, true);
+    // This will trigger a notification afterwards, so application
+    // of the migrated proxy config is not necessary here.
   } else  {
-    // Otherwise, determine and activate possibly new effective proxy config.
-    DetermineEffectiveConfig(active_network, true);
+    UpdateProxyConfigOfDefaultNetwork();
   }
 }
 
@@ -701,8 +657,8 @@ void ProxyConfigServiceImpl::SetProxyConfigForNetwork(
     VLOG(1) << "Set proxy for "
             << (network->name().empty() ? network_path : network->name())
             << ", value=" << value;
-    if (network_path == active_network_)
-      DetermineEffectiveConfig(network, true);
+    // Even in case the default network was changed, there's no need to apply
+    // the new proxy settings, because we will be notified afterwards.
   }
 }
 
@@ -716,18 +672,22 @@ bool ProxyConfigServiceImpl::GetUseSharedProxies() {
   return use_shared_proxies_.GetValue();
 }
 
-bool ProxyConfigServiceImpl::IgnoreProxy(const Network* network) {
-  if (network->profile_type() == PROFILE_USER)
+bool ProxyConfigServiceImpl::IgnoreProxy(const NetworkState& network) {
+  const NetworkProfile* profile =
+      NetworkProfileHandler::Get()->GetProfileForPath(network.profile_path());
+  if (!profile)
+    return true;
+  if (profile->type() == NetworkProfile::TYPE_USER)
     return false;
 
-  if (network->ui_data().onc_source() == onc::ONC_SOURCE_DEVICE_POLICY &&
+  if (network.onc_source() == onc::ONC_SOURCE_DEVICE_POLICY &&
       UserManager::Get()->IsUserLoggedIn()) {
     policy::BrowserPolicyConnector* connector =
         g_browser_process->browser_policy_connector();
     const User* logged_in_user = UserManager::Get()->GetLoggedInUser();
     if (connector->GetUserAffiliation(logged_in_user->email()) ==
             policy::USER_AFFILIATION_MANAGED) {
-      VLOG(1) << "Respecting proxy for network " << network->name()
+      VLOG(1) << "Respecting proxy for network " << network.name()
               << ", as logged-in user belongs to the domain the device "
               << "is enrolled to.";
       return false;
@@ -737,8 +697,17 @@ bool ProxyConfigServiceImpl::IgnoreProxy(const Network* network) {
   return !GetUseSharedProxies();
 }
 
-void ProxyConfigServiceImpl::DetermineEffectiveConfig(const Network* network,
-                                                      bool activate) {
+void ProxyConfigServiceImpl::UpdateProxyConfigOfDefaultNetwork() {
+  const NetworkState* network = NetworkStateHandler::Get()->DefaultNetwork();
+  LOG_IF(ERROR, network && network->path() != default_network_)
+      << "Missed network change notification: " << default_network_
+      << " -> " << network->path();
+  DetermineEffectiveConfig(network, true);
+}
+
+void ProxyConfigServiceImpl::DetermineEffectiveConfig(
+    const NetworkState* network,
+    bool activate) {
   // Get prefs proxy config if available.
   net::ProxyConfig pref_config;
   ProxyPrefs::ConfigState pref_state = GetProxyConfig(&pref_config);
@@ -751,18 +720,16 @@ void ProxyConfigServiceImpl::DetermineEffectiveConfig(const Network* network,
   if (network) {
     // If we're activating proxy, ignore proxy if necessary;
     // otherwise, for ui, get actual proxy to show user.
-    ignore_proxy = activate ? IgnoreProxy(network) : false;
+    ignore_proxy = activate ? IgnoreProxy(*network) : false;
     // If network is shared but use-shared-proxies is off, use direct mode.
     if (ignore_proxy) {
       VLOG(1) << "Shared network && !use-shared-proxies, use direct";
       network_availability = net::ProxyConfigService::CONFIG_VALID;
-    } else if (!network->proxy_config().empty()) {
+    } else if (ParseProxyConfig(*network, &network_config)) {
       // Network is private or shared with user using shared proxies.
-      if (ParseProxyConfig(network, &network_config)) {
-        VLOG(1) << this << ": using network proxy: "
-                << network->proxy_config();
-        network_availability = net::ProxyConfigService::CONFIG_VALID;
-      }
+      VLOG(1) << this << ": using network proxy: "
+              << network->proxy_config();
+      network_availability = net::ProxyConfigService::CONFIG_VALID;
     }
   }
 
@@ -800,47 +767,26 @@ void ProxyConfigServiceImpl::DetermineEffectiveConfig(const Network* network,
       PrefProxyConfigTrackerImpl::OnProxyConfigChanged(effective_config_state,
                                                        effective_config);
       if (VLOG_IS_ON(1) && !update_pending()) {  // Update was successful.
-        scoped_ptr<DictionaryValue> config_dict(static_cast<DictionaryValue*>(
-            effective_config.ToValue()));
-        std::string config_value;
-        JSONStringValueSerializer serializer(&config_value);
-        serializer.Serialize(*config_dict.get());
+        scoped_ptr<base::DictionaryValue> config_dict(
+            effective_config.ToValue());
         VLOG(1) << this << ": Proxy changed: "
                 << ConfigStateToString(active_config_state_)
-                << ", " << config_value;
+                << ", " << *config_dict;
       }
     }
   } else {  // For UI, store effective proxy into |current_ui_config_|.
+    DCHECK(network);
     current_ui_config_.FromNetProxyConfig(effective_config);
     current_ui_config_.state = effective_config_state;
     if (PrefPrecedes(effective_config_state)) {
       current_ui_config_.user_modifiable = false;
-    } else if (!IsNetworkProxySettingsEditable(network)) {
+    } else if (!IsNetworkProxySettingsEditable(*network)) {
       // TODO(xiyuan): Figure out the right way to set config state for managed
       // network.
       current_ui_config_.state = ProxyPrefs::CONFIG_POLICY;
       current_ui_config_.user_modifiable = false;
     } else {
-      current_ui_config_.user_modifiable = !network || !IgnoreProxy(network);
-    }
-  }
-}
-
-void ProxyConfigServiceImpl::OnUISetCurrentNetwork(const Network* network) {
-  DetermineEffectiveConfig(network, false);
-  VLOG(1) << "Current ui network: "
-          << (network->name().empty() ? current_ui_network_ : network->name())
-          << ", " << ModeToString(current_ui_config_.mode)
-          << ", " << ConfigStateToString(current_ui_config_.state)
-          << ", modifiable:" << current_ui_config_.user_modifiable;
-  // Notify whoever is interested in this change.
-  std::vector<base::Closure>::iterator iter = callbacks_.begin();
-  while (iter != callbacks_.end()) {
-    if (iter->is_null()) {
-      iter = callbacks_.erase(iter);
-    } else {
-      iter->Run();
-      ++iter;
+      current_ui_config_.user_modifiable = !IgnoreProxy(*network);
     }
   }
 }
@@ -875,9 +821,11 @@ void ProxyConfigServiceImpl::FetchProxyPolicy() {
     device_config_.clear();
     return;
   }
-  if (!active_network_.empty()) {
-    VLOG(1) << "Try migrating device config to " << active_network_;
-    SetProxyConfigForNetwork(active_network_, device_config_, true);
+  const NetworkState* default_network =
+      NetworkStateHandler::Get()->DefaultNetwork();
+  if (default_network) {
+    VLOG(1) << "Try migrating device config to " << default_network->path();
+    SetProxyConfigForNetwork(default_network->path(), device_config_, true);
   }
 }