Revert 202388 "Remove NetworkStateInformer's dependency on Proxy..."

Suspected to break CrOS ASAN (x86) bot.

> Remove NetworkStateInformer's dependency on ProxyConfigServiceImpl.
> 
> This also completes NetworkStateInformer's migration from NetworkLibrary to
> NetworkStateHandler
> 
> BUG=189009
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201688
> 
> R=stevenjb@chromium.org, ygorshenin@chromium.org
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202192
> 
> Review URL: https://codereview.chromium.org/15294010

TBR=pneubeck@chromium.org

Review URL: https://codereview.chromium.org/15875018

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

chrome/browser/ui/webui/chromeos/login/network_state_informer.cc

@@ -7,8 +7,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
-#include "chrome/browser/prefs/proxy_config_dictionary.h"
-#include "chrome/browser/prefs/proxy_prefs.h"
+#include "chrome/browser/chromeos/proxy_config_service_impl.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
@@ -25,8 +24,7 @@ const int kNetworkStateCheckDelaySec = 3;
 namespace chromeos {
 
 NetworkStateInformer::NetworkStateInformer()
-    : state_(OFFLINE),
-      weak_ptr_factory_(this) {
+    : state_(OFFLINE) {
 }
 
 NetworkStateInformer::~NetworkStateInformer() {
@@ -90,7 +88,7 @@ void NetworkStateInformer::NetworkManagerChanged() {
     check_state_.Cancel();
     check_state_.Reset(
         base::Bind(&NetworkStateInformer::UpdateStateAndNotify,
-                   weak_ptr_factory_.GetWeakPtr()));
+                   base::Unretained(this)));
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         check_state_.callback(),
@@ -211,9 +209,19 @@ NetworkStateInformer::State NetworkStateInformer::GetNetworkState(
 bool NetworkStateInformer::IsProxyConfigured(const NetworkState* network) {
   DCHECK(network);
 
-  ProxyConfigDictionary proxy_dict(&network->proxy_config());
-  ProxyPrefs::ProxyMode mode;
-  return !proxy_dict.GetMode(&mode) || mode == ProxyPrefs::MODE_FIXED_SERVERS;
+  ProxyStateMap::iterator it = proxy_state_map_.find(network->guid());
+  if (it != proxy_state_map_.end() &&
+      it->second.proxy_config == network->proxy_config()) {
+    return it->second.configured;
+  }
+  net::ProxyConfig proxy_config;
+  if (!ProxyConfigServiceImpl::ParseProxyConfig(network->proxy_config(),
+                                                &proxy_config))
+    return false;
+  bool configured = !proxy_config.proxy_rules().empty();
+  proxy_state_map_[network->guid()] =
+      ProxyState(network->proxy_config(), configured);
+  return configured;
 }
 
 }  // namespace chromeos

chrome/browser/ui/webui/chromeos/login/network_state_informer.h

@@ -11,7 +11,6 @@
 #include "base/cancelable_callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/browser/chromeos/login/captive_portal_window_proxy.h"
 #include "chrome/browser/chromeos/login/screens/error_screen_actor.h"
@@ -95,6 +94,21 @@ class NetworkStateInformer
   std::string last_network_type() const { return last_network_type_; }
 
  private:
+  struct ProxyState {
+    ProxyState() : configured(false) {
+    }
+
+    ProxyState(const std::string& proxy_config, bool configured)
+        : proxy_config(proxy_config),
+          configured(configured) {
+    }
+
+    std::string proxy_config;
+    bool configured;
+  };
+
+  typedef std::map<std::string, ProxyState> ProxyStateMap;
+
   friend class base::RefCounted<NetworkStateInformer>;
 
   virtual ~NetworkStateInformer();
@@ -117,7 +131,8 @@ class NetworkStateInformer
   std::string last_network_type_;
   base::CancelableClosure check_state_;
 
-  base::WeakPtrFactory<NetworkStateInformer> weak_ptr_factory_;
+  // Caches proxy state for active networks.
+  ProxyStateMap proxy_state_map_;
 };
 
 }  // namespace chromeos

chromeos/network/managed_state.h

@@ -40,14 +40,9 @@ class ManagedState {
   NetworkState* AsNetworkState();
   DeviceState* AsDeviceState();
 
-  // Called by NetworkStateHandler when a property was received. The return
-  // value indicates if the state changed and is used to reduce the number of
-  // notifications. The only guarantee however is: If the return value is false
-  // then the state wasn't modified. This might happen because of
-  // * |key| was not recognized.
-  // * |value| was not parsed successfully.
-  // * |value| is equal to the cached property value.
-  // If the return value is true, the state might or might not be modified.
+  // Called by NetworkStateHandler when a property changes. Returns false if
+  // the property was not recognized, was not parsed successfully, or is
+  // unchanged (complex properties may be assumed to have changed).
   virtual bool PropertyChanged(const std::string& key,
                                const base::Value& value) = 0;
 

chromeos/network/network_state.cc

@@ -10,8 +10,8 @@
 #include "base/stringprintf.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversion_utils.h"
+#include "base/values.h"
 #include "chromeos/network/network_event_log.h"
-#include "chromeos/network/onc/onc_utils.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace {
@@ -88,9 +88,9 @@ bool NetworkState::PropertyChanged(const std::string& key,
   } else if (key == IPConfigProperty(flimflam::kNameServersProperty)) {
     dns_servers_.clear();
     const base::ListValue* dns_servers;
-    if (value.GetAsList(&dns_servers))
-      ConvertListValueToStringVector(*dns_servers, &dns_servers_);
-    return true;
+    if (value.GetAsList(&dns_servers) &&
+        ConvertListValueToStringVector(*dns_servers, &dns_servers_))
+      return true;
   } else if (key == flimflam::kActivationStateProperty) {
     return GetStringValue(key, value, &activation_state_);
   } else if (key == flimflam::kRoamingStateProperty) {
@@ -103,29 +103,6 @@ bool NetworkState::PropertyChanged(const std::string& key,
     return GetBooleanValue(key, value, &favorite_);
   } else if (key == flimflam::kPriorityProperty) {
     return GetIntegerValue(key, value, &priority_);
-  } else if (key == flimflam::kProxyConfigProperty) {
-    std::string proxy_config_str;
-    if (!value.GetAsString(&proxy_config_str)) {
-      LOG(WARNING) << "Failed to parse string value for:" << key;
-      return false;
-    }
-
-    proxy_config_.Clear();
-    if (proxy_config_str.empty())
-      return true;
-
-    scoped_ptr<base::DictionaryValue> proxy_config_dict(
-        onc::ReadDictionaryFromJson(proxy_config_str));
-    if (proxy_config_dict) {
-      // Warning: The DictionaryValue return from
-      // ReadDictionaryFromJson/JSONParser is an optimized derived class that
-      // doesn't allow releasing ownership of nested values. A Swap in the wrong
-      // order leads to memory access errors.
-      proxy_config_.MergeDictionary(proxy_config_dict.get());
-    } else {
-      LOG(WARNING) << "Failed to parse dictionary value for: " << key;
-    }
-    return true;
   } else if (key == flimflam::kNetworkTechnologyProperty) {
     return GetStringValue(key, value, &technology_);
   } else if (key == flimflam::kDeviceProperty) {
@@ -134,6 +111,8 @@ bool NetworkState::PropertyChanged(const std::string& key,
     return GetStringValue(key, value, &guid_);
   } else if (key == flimflam::kProfileProperty) {
     return GetStringValue(key, value, &profile_path_);
+  } else if (key == flimflam::kProxyConfigProperty) {
+    return GetStringValue(key, value, &proxy_config_);
   } else if (key == shill::kActivateOverNonCellularNetworkProperty) {
     return GetBooleanValue(key, value, &activate_over_non_cellular_networks_);
   } else if (key == shill::kOutOfCreditsProperty) {
@@ -192,11 +171,6 @@ void NetworkState::GetProperties(base::DictionaryValue* dictionary) const {
                                              favorite_);
   dictionary->SetIntegerWithoutPathExpansion(flimflam::kPriorityProperty,
                                              priority_);
-  // Proxy config is intentionally omitted: This property is
-  // placed in NetworkState to transition proxy configuration from
-  // NetworkLibrary to the new network stack. The networking extension API
-  // shouldn't depend on this member. Once ManagedNetworkConfigurationHandler
-  // is used instead of NetworkLibrary, we can remove them again.
   dictionary->SetStringWithoutPathExpansion(
       flimflam::kNetworkTechnologyProperty,
       technology_);
@@ -205,6 +179,8 @@ void NetworkState::GetProperties(base::DictionaryValue* dictionary) const {
   dictionary->SetStringWithoutPathExpansion(flimflam::kGuidProperty, guid_);
   dictionary->SetStringWithoutPathExpansion(flimflam::kProfileProperty,
                                             profile_path_);
+  dictionary->SetStringWithoutPathExpansion(flimflam::kProxyConfigProperty,
+                                            proxy_config_);
   dictionary->SetBooleanWithoutPathExpansion(
       shill::kActivateOverNonCellularNetworkProperty,
       activate_over_non_cellular_networks_);

chromeos/network/network_state.h

@@ -8,9 +8,12 @@
 #include <string>
 #include <vector>
 
-#include "base/values.h"
 #include "chromeos/network/managed_state.h"
 
+namespace base {
+class DictionaryValue;
+}
+
 namespace chromeos {
 
 // Simple class to provide network state information about a network service.
@@ -42,12 +45,12 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   const std::string& guid() const { return guid_; }
   const std::string& connection_state() const { return connection_state_; }
   const std::string& profile_path() const { return profile_path_; }
+  const std::string& proxy_config() const { return proxy_config_; }
   const std::string& error() const { return error_; }
   const std::string& error_details() const { return error_details_; }
   bool auto_connect() const { return auto_connect_; }
   bool favorite() const { return favorite_; }
   int priority() const { return priority_; }
-  const base::DictionaryValue& proxy_config() const { return proxy_config_; }
   // Wireless property accessors
   int signal_strength() const { return signal_strength_; }
   bool connectable() const { return connectable_; }
@@ -97,14 +100,12 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   std::string guid_;
   std::string connection_state_;
   std::string profile_path_;
+  std::string proxy_config_;
   std::string error_;
   std::string error_details_;
   bool auto_connect_;
   bool favorite_;
   int priority_;
-  // TODO(pneubeck): Remove this property once NetworkConfigurationHandler
-  // provides proxy configuration. crbug/241775
-  base::DictionaryValue proxy_config_;
   // IPConfig properties.
   // Note: These do not correspond to actual Shill.Service properties
   // but are derived from the service's corresponding IPConfig object.