Convert all connect code to use NetworkHandler instead of NetworkLibrary

ash/system/chromeos/network/network_connect.cc

Index: ash/system/chromeos/network/network_connect.cc
diff --git a/ash/system/chromeos/network/network_connect.cc b/ash/system/chromeos/network/network_connect.cc
index 5c62499aed4e43ebefd48622bcb8988dfe3fa8e3..1603c32bed19fcd1fd833cc0246daba2fc656dfc 100644
--- a/ash/system/chromeos/network/network_connect.cc
+++ b/ash/system/chromeos/network/network_connect.cc
@@ -13,33 +13,52 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "chromeos/network/device_state.h"
+#include "chromeos/network/network_configuration_handler.h"
 #include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_event_log.h"
+#include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "grit/ash_strings.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/base/l10n/l10n_util.h"
 
+using chromeos::DeviceState;
+using chromeos::NetworkConfigurationHandler;
 using chromeos::NetworkConnectionHandler;
 using chromeos::NetworkHandler;
+using chromeos::NetworkProfile;
+using chromeos::NetworkProfileHandler;
 using chromeos::NetworkState;
 
 namespace ash {
 
 namespace {
 
+bool IsDirectActivatedCarrier(const std::string& carrier) {

[pneubeck (no reviews), 2013/08/06 15:45:19]

nit: maybe it's worth explaining what "DirectActivated" means.

[stevenjb, 2013/08/06 20:23:55]

Done.

+  if (carrier == shill::kCarrierSprint)
+    return true;
+  return false;
+}
+
 void OnConnectFailed(const std::string& service_path,
+                     gfx::NativeWindow owning_window,
                      const std::string& error_name,
                      scoped_ptr<base::DictionaryValue> error_data) {
-  VLOG(1) << "Connect Failed for " << service_path << ": " << error_name;
+  NET_LOG_ERROR("Connect Failed: " + error_name, service_path);
   if (error_name == NetworkConnectionHandler::kErrorPassphraseRequired) {
     // TODO(stevenjb): Possibly add inline UI to handle passphrase entry here.
     ash::Shell::GetInstance()->system_tray_delegate()->ConfigureNetwork(
         service_path);
     return;
   }
+  if (error_name == NetworkConnectionHandler::kErrorCertificateRequired) {
+    ash::Shell::GetInstance()->system_tray_delegate()->EnrollOrConfigureNetwork(

[pneubeck (no reviews), 2013/08/06 15:45:19]

make this conditional on owning_window!=NULL and log an error otherwise.

[stevenjb, 2013/08/06 20:23:55]

It's not conditional on owning_window != NULL. The delegate should handle that
case by attaching the window to the default browser.

+        service_path, owning_window);
+    return;
+  }
   if (error_name == NetworkConnectionHandler::kErrorActivationRequired ||

[pneubeck (no reviews), 2013/08/06 15:45:19]

kErrorActivationRequired should lead to
network_connect::ActivateCellular(service_path);
and replace lines 104-109.

(lines 104-109 duplicate NetworkRequiresActivation in
network_connection_handler.cc)

[stevenjb, 2013/08/06 20:23:55]

Yes... that looks sound. Done.

-      error_name == NetworkConnectionHandler::kErrorCertificateRequired ||
       error_name == NetworkConnectionHandler::kErrorConfigurationRequired ||
       error_name == NetworkConnectionHandler::kErrorAuthenticationRequired) {
     ash::Shell::GetInstance()->system_tray_delegate()->ConfigureNetwork(
@@ -63,27 +82,191 @@ void OnConnectFailed(const std::string& service_path,
 }
 
 void OnConnectSucceeded(const std::string& service_path) {
-  VLOG(1) << "Connect Succeeded for " << service_path;
+  NET_LOG_USER("Connect Succeeded", service_path);
   ash::Shell::GetInstance()->system_tray_notifier()->NotifyClearNetworkMessage(
       NetworkObserver::ERROR_CONNECT_FAILED);
 }
 
-}  // namespace
-
-namespace network_connect {
+// If |check_error_state| is true, error state for the network is checked,
+//  otherwise any current error state is ignored (e.g. for recently configured
+//  networks or repeat connect attempts).
+void CallConnectToNetwork(const std::string& service_path,
+                          bool check_error_state,
+                          gfx::NativeWindow owning_window) {
+  NET_LOG_USER("ConnectToNetwork", service_path);
 
-void ConnectToNetwork(const std::string& service_path) {
   const NetworkState* network = NetworkHandler::Get()->network_state_handler()->

[pneubeck (no reviews), 2013/08/06 15:45:19]

if you did the change proposed above, this could be replaced by handling
NetworkConnectionHandler::kErrorNotFound in OnConnectFailed.

[stevenjb, 2013/08/06 20:23:55]

Actually, removing this will trigger a similar log entry and show a notification
which is really what we want, so removed.

       GetNetworkState(service_path);
-  if (!network)
+  if (!network) {
+    NET_LOG_USER("ConnectToNetwork: Not Found", service_path);
     return;
+  }
+  if (network->type() == flimflam::kTypeCellular &&
+      (network->activation_state() != flimflam::kActivationStateActivated ||
+       network->cellular_out_of_credits())) {
+    network_connect::ActivateCellular(service_path);
+    return;
+  }
+
   ash::Shell::GetInstance()->system_tray_notifier()->NotifyClearNetworkMessage(
       NetworkObserver::ERROR_CONNECT_FAILED);
   NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
       service_path,
       base::Bind(&OnConnectSucceeded, service_path),
-      base::Bind(&OnConnectFailed, service_path),
-      true /* check_error_state */);
+      base::Bind(&OnConnectFailed, service_path, owning_window),
+      check_error_state);
+}
+
+void OnActivateFailed(const std::string& service_path,
+                     const std::string& error_name,
+                      scoped_ptr<base::DictionaryValue> error_data) {
+  NET_LOG_ERROR("Unable to activate network", service_path);
+}
+
+void OnActivateSucceeded(const std::string& service_path) {
+  NET_LOG_USER("Activation Succeeded", service_path);
+}
+
+void OnConfigureFailed(const std::string& error_name,
+                       scoped_ptr<base::DictionaryValue> error_data) {
+  NET_LOG_ERROR("Unable to configure network", "");
+}
+
+void OnConfigureSucceeded(const std::string& service_path) {
+  NET_LOG_USER("Configure Succeeded", service_path);
+  // After configuring a network, ignore any (possibly stale) error state.
+  const bool check_error_state = false;
+  CallConnectToNetwork(service_path, check_error_state, NULL);
+}
+
+void SetPropertiesFailed(const std::string& desc,
+                         const std::string& service_path,
+                         const std::string& config_error_name,
+                         scoped_ptr<base::DictionaryValue> error_data) {
+  NET_LOG_ERROR(desc + ": Failed: " + config_error_name, service_path);
+}
+
+void SetPropertiesToClear(base::DictionaryValue* properties_to_set,
+                          std::vector<std::string>* properties_to_clear) {
+  // Move empty string properties to properties_to_clear.
+  for (base::DictionaryValue::Iterator iter(*properties_to_set);
+       !iter.IsAtEnd(); iter.Advance()) {
+    if (iter.value().GetType() == base::Value::TYPE_STRING) {

[pneubeck (no reviews), 2013/08/06 15:45:19]

how about:

std::string value_str;
if (iter.value().GetAsString(&value_str) && value_str.empty())
  properties_to_clear->push_back(iter.key());

[stevenjb, 2013/08/06 20:23:55]

Done.

+      std::string value_str;
+      iter.value().GetAsString(&value_str);
+      if (value_str.empty())
+        properties_to_clear->push_back(iter.key());
+    }
+  }
+  // Remove cleared properties from properties_to_set.
+  for (std::vector<std::string>::iterator iter = properties_to_clear->begin();
+       iter != properties_to_clear->end(); ++iter) {
+    properties_to_set->RemoveWithoutPathExpansion(*iter, NULL);
+  }
+}
+
+void ClearPropertiesAndConnect(
+    const std::string& service_path,
+    const std::vector<std::string>& properties_to_clear) {
+  NET_LOG_USER("ClearPropertiesAndConnect", service_path);
+  // After configuring a network, ignore any (possibly stale) error state.
+  const bool check_error_state = false;
+  if (properties_to_clear.empty()) {
+    CallConnectToNetwork(service_path, check_error_state, NULL);

[pneubeck (no reviews), 2013/08/06 15:45:19]

either document that parent=NULL is a valid argument at the definition of
CallConnectToNetwork/OnConnectFailed and handle it in OnConnectFailed.
or provide a valid parent window here.

Similarly it should be mentioned if parent=NULL is a valid arg to
ConnectToNetwork.

[stevenjb, 2013/08/06 20:23:55]

Done.

+  } else {
+    NetworkHandler::Get()->network_configuration_handler()->ClearProperties(

[pneubeck (no reviews), 2013/08/06 15:45:19]

NetworkConfigurationHandler::ClearProperties could early return if the list is
empty.

[stevenjb, 2013/08/06 20:23:55]

Done.

+        service_path,
+        properties_to_clear,
+        base::Bind(&CallConnectToNetwork,
+                   service_path, check_error_state,
+                   static_cast<gfx::NativeWindow>(NULL)),
+        base::Bind(&SetPropertiesFailed, "ClearProperties", service_path));
+  }
+}
+
+}  // namespace
+
+namespace network_connect {
+
+void ConnectToNetwork(const std::string& service_path,
+                      gfx::NativeWindow owning_window) {
+  const bool check_error_state = true;
+  CallConnectToNetwork(service_path, check_error_state, owning_window);
+}
+
+void ActivateCellular(const std::string& service_path) {
+  NET_LOG_USER("ActivateCellular", service_path);
+  const DeviceState* cellular_device =
+      NetworkHandler::Get()->network_state_handler()->
+      GetDeviceStateByType(flimflam::kTypeCellular);
+  if (!cellular_device) {
+    NET_LOG_ERROR("ActivateCellular with no Device", service_path);
+    return;
+  }
+  if (!IsDirectActivatedCarrier(cellular_device->carrier())) {
+    // For non direct activation, show the mobile setup dialog which can be
+    // used to activate the network.
+    ash::Shell::GetInstance()->system_tray_delegate()->ShowMobileSetup(
+        service_path);
+    return;
+  }
+  const NetworkState* cellular =
+      NetworkHandler::Get()->network_state_handler()->
+      FirstNetworkByType(flimflam::kTypeCellular);
+  if (!cellular) {

[pneubeck (no reviews), 2013/08/06 15:45:19]

what if cellular->path() != service_path ?
why not instead GetNetworkState(service_path) and check that the type is
kTypeCellular.

[stevenjb, 2013/08/06 20:23:55]

Yeah, this was converted a little too literally. Done.

+    NET_LOG_ERROR("ActivateCellular with no Service", service_path);
+    return;
+  }
+  if (cellular->activation_state() == flimflam::kActivationStateActivated) {
+    NET_LOG_ERROR("ActivateCellular for activated service", service_path);
+    return;
+  }
+
+  NetworkHandler::Get()->network_connection_handler()->ActivateNetwork(
+      service_path,
+      "",  // carrier
+      base::Bind(&OnActivateSucceeded, service_path),
+      base::Bind(&OnActivateFailed, service_path));
+}
+
+void ConfigureNetworkAndConnect(const std::string& service_path,
+                                const base::DictionaryValue& properties) {
+  NET_LOG_USER("ConfigureNetworkAndConnect", service_path);
+  scoped_ptr<base::DictionaryValue> properties_to_set(properties.DeepCopy());
+  std::vector<std::string> properties_to_clear;
+  SetPropertiesToClear(properties_to_set.get(), &properties_to_clear);
+  if (properties_to_set->empty()) {
+    ClearPropertiesAndConnect(service_path,
+                              properties_to_clear);
+  } else {
+    NetworkHandler::Get()->network_configuration_handler()->SetProperties(

[pneubeck (no reviews), 2013/08/06 15:45:19]

NetworkConfigurationHandler::SetProperties could early return if the dict is
empty.

[stevenjb, 2013/08/06 20:23:55]

Done.

+        service_path,
+        *properties_to_set,
+        base::Bind(&ClearPropertiesAndConnect,
+                   service_path,
+                   properties_to_clear),
+        base::Bind(&SetPropertiesFailed, "SetProperties", service_path));
+  }
+}
+
+void CreateConfigurationAndConnect(base::DictionaryValue* properties,
+                                   bool shared) {

[pneubeck (no reviews), 2013/08/06 15:45:19]

|shared| is not used.

[stevenjb, 2013/08/06 20:23:55]

Oops. Fixed this on the calling side, removed 'shared'.

+  NET_LOG_USER("CreateConfigurationAndConnect", "");
+  const NetworkProfile* profile =

[pneubeck (no reviews), 2013/08/06 15:45:19]

enclose this in a 'if (!shared)'

log error if GetDefaultUserProfile() returns NULL.
but ensure that shared profile is not used if shared=false.

[stevenjb, 2013/08/06 20:23:55]

Done.

+      NetworkHandler::Get()->network_profile_handler()->
+      GetDefaultUserProfile();
+  if (profile) {
+    properties->SetStringWithoutPathExpansion(
+        flimflam::kProfileProperty, profile->path);

[pneubeck (no reviews), 2013/08/06 15:45:19]

set a 'std::string profile_path' in the if clauses and call
 properties->SetStringWithoutPathExpansion(
   flimflam::kProfileProperty, profile_path);
afterwards.

[stevenjb, 2013/08/06 20:23:55]

Done.

+  } else {
+    properties->SetStringWithoutPathExpansion(
+        flimflam::kProfileProperty,
+        NetworkProfileHandler::kSharedProfilePath);
+  }
+  NetworkHandler::Get()->network_configuration_handler()->CreateConfiguration(
+      *properties,
+      base::Bind(&OnConfigureSucceeded),
+      base::Bind(&OnConfigureFailed));
 }
 
 string16 ErrorString(const std::string& error) {