Use NetworkConnectionObserver to trigger connect failure notifications

ui/chromeos/network/network_connect.cc

Index: ui/chromeos/network/network_connect.cc
diff --git a/ui/chromeos/network/network_connect.cc b/ui/chromeos/network/network_connect.cc
index e1d98566c0c6661e05b321bbb1fa57fdc91fdb67..2ef55562d953b2d8bc344fea880f3bc85e9210c4 100644
--- a/ui/chromeos/network/network_connect.cc
+++ b/ui/chromeos/network/network_connect.cc
@@ -74,8 +74,8 @@ class NetworkConnectImpl : public NetworkConnect {
                                      bool shared) override;
   void CreateConfiguration(base::DictionaryValue* properties,
                            bool shared) override;
-  base::string16 GetErrorString(const std::string& error,
-                                const std::string& service_path) override;
+  base::string16 GetShillErrorString(const std::string& error,
+                                     const std::string& service_path) override;
   void ShowNetworkSettings(const std::string& service_path) override;
 
  private:
@@ -198,12 +198,20 @@ void NetworkConnectImpl::OnConnectFailed(
     scoped_ptr<base::DictionaryValue> error_data) {
   NET_LOG_ERROR("Connect Failed: " + error_name, service_path);
 
-  // If a new connect attempt canceled this connect, no need to notify the
-  // user.
-  if (error_name == NetworkConnectionHandler::kErrorConnectCanceled)
+  // If a new connect attempt canceled this connect, or a connect attempt is
+  // in progress, no need to notify the user.
+  if (error_name == NetworkConnectionHandler::kErrorConnectCanceled ||
+      error_name == NetworkConnectionHandler::kErrorConnecting) {

[pneubeck (no reviews), 2015/01/04 16:57:00]

is this a change compared to before? not sure whether users can run into long
running attempts of shill to connect which they cannot abort anymore except
through disabling wifi or going to the settings page.

[stevenjb, 2015/01/09 18:18:03]

This handles the case where we attempt to connect to network A while it is
connecting, e.g. by having two settings UI windows open (or a bug in the UI).
The user will be notified (or the appropriate UI  will be spawned) when the
initial attempt fails or succeeds. Notifying them again just adds noise /
confusion.

+    return;
+  }
+
+  // Already connected to the network, show the settings UI for the network.
+  if (error_name == NetworkConnectionHandler::kErrorConnected) {
+    ShowNetworkSettings(service_path);
     return;
+  }
 
-  if (error_name == shill::kErrorBadPassphrase ||
+  if (error_name == NetworkConnectionHandler::kErrorBadPassphrase ||
       error_name == NetworkConnectionHandler::kErrorPassphraseRequired ||
       error_name == NetworkConnectionHandler::kErrorConfigurationRequired ||
       error_name == NetworkConnectionHandler::kErrorAuthenticationRequired) {
@@ -212,38 +220,16 @@ void NetworkConnectImpl::OnConnectFailed(
   }
 
   if (error_name == NetworkConnectionHandler::kErrorCertificateRequired) {
-    if (!delegate_->ShowEnrollNetwork(service_path)) {
+    if (!delegate_->ShowEnrollNetwork(service_path))
       HandleUnconfiguredNetwork(service_path);
-    }
-    return;
-  }
-
-  if (error_name == NetworkConnectionHandler::kErrorActivationRequired) {
-    ActivateCellular(service_path);
     return;
   }
 
-  if (error_name == NetworkConnectionHandler::kErrorConnected ||
-      error_name == NetworkConnectionHandler::kErrorConnecting) {
-    ShowNetworkSettings(service_path);
-    return;
-  }
-
-  // ConnectFailed or unknown error; show a notification.
-  network_state_notifier_->ShowNetworkConnectError(error_name, service_path);
-
   // Only show a configure dialog if there was a ConnectFailed error.

[pneubeck (no reviews), 2015/01/04 16:57:00]

'if there was a ConnectFailed error' is not very informative.
maybe 'if there was an unexpected error during the connect attempt' ?

[stevenjb, 2015/01/09 18:18:03]

Again, 'ConnectFailed' could be caused by a number of things, which (at this
point) we do not differentiate. I don't think that "unexpected" or "unknown"
adds any clarity here. I will try to clarify the comment a bit though.

-  if (error_name != shill::kErrorConnectFailed)
-    return;
-
-  // If Shill reports an InProgress error, don't try to configure the network.
-  std::string dbus_error_name;
-  error_data.get()->GetString(chromeos::network_handler::kDbusErrorName,
-                              &dbus_error_name);
-  if (dbus_error_name == shill::kErrorResultInProgress)
-    return;
+  if (error_name == NetworkConnectionHandler::kErrorConnectFailed)
+    HandleUnconfiguredNetwork(service_path);
 
-  HandleUnconfiguredNetwork(service_path);
+  // Notifications on connect failure are handled by NetworkStateNotifier.

[pneubeck (no reviews), 2015/01/04 16:57:00]

should this be a comment at the top of the function?

[stevenjb, 2015/01/09 18:18:02]

This explains why we don't do anything else in this function. I will clarify
this comment and add a note above the function.

 }
 
 void NetworkConnectImpl::OnConnectSucceeded(const std::string& service_path) {
@@ -530,7 +516,7 @@ void NetworkConnectImpl::CreateConfiguration(base::DictionaryValue* properties,
   CallCreateConfiguration(properties, shared, false /* connect_on_configure */);
 }
 
-base::string16 NetworkConnectImpl::GetErrorString(
+base::string16 NetworkConnectImpl::GetShillErrorString(
     const std::string& error,
     const std::string& service_path) {
   if (error.empty())