Trigger NetworkConnectionHandler callbacks on success or failure

This CL triggers waits for the network state to change before triggering
success or failure callbacks in NetworkConnectionHandler.

Notifications are triggered on success or failure without requiring the
NetworkStateHandler connecting_network() hack.

It also separates the enable flag from NetworkConfigurationHandler since
enabling that has different artifacts.

BUG=243899
For ash.gyp:
TBR=gspencer@chromium.org

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

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

7 files changed, 194 insertions, 76 deletions

diff --git a/ash/system/chromeos/network/network_connect.cc b/ash/system/chromeos/network/network_connect.cc
new file mode 100644
index 0000000..03c30be
--- /dev/null
+++ b/ash/system/chromeos/network/network_connect.cc
@@ -0,0 +1,77 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/system/chromeos/network/network_connect.h"
+
+#include "ash/shell.h"
+#include "ash/system/chromeos/network/network_observer.h"
+#include "ash/system/chromeos/network/network_state_notifier.h"
+#include "ash/system/tray/system_tray_delegate.h"
+#include "ash/system/tray/system_tray_notifier.h"
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+
+using chromeos::NetworkConnectionHandler;
+using chromeos::NetworkHandler;
+using chromeos::NetworkState;
+
+namespace ash {
+
+namespace {
+
+void OnConnectFailed(const std::string& service_path,
+                     const std::string& error_name,
+                     scoped_ptr<base::DictionaryValue> error_data) {
+  VLOG(1) << "Connect Failed for " << service_path << ": " << error_name;
+  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::kErrorActivationRequired ||
+      error_name == NetworkConnectionHandler::kErrorCertificateRequired ||
+      error_name == NetworkConnectionHandler::kErrorConfigurationRequired) {
+    ash::Shell::GetInstance()->system_tray_delegate()->ConfigureNetwork(
+        service_path);
+    return;
+  }
+  if (error_name == NetworkConnectionHandler::kErrorConnected ||
+      error_name == NetworkConnectionHandler::kErrorConnecting) {
+    ash::Shell::GetInstance()->system_tray_delegate()->ShowNetworkSettings(
+        service_path);
+    return;
+  }
+  ash::Shell::GetInstance()->system_tray_notifier()->network_state_notifier()->
+      ShowNetworkConnectError(error_name, service_path);
+}
+
+void OnConnectSucceeded(const std::string& service_path) {
+  VLOG(1) << "Connect Succeeded for " << service_path;
+}
+
+}  // namespace
+
+namespace network_connect {
+
+void ConnectToNetwork(const std::string& service_path) {
+  const NetworkState* network = NetworkHandler::Get()->network_state_handler()->
+      GetNetworkState(service_path);
+  if (!network)
+    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),
+      false /* ignore_error_state */);
+}
+
+}  // network_connect
+}  // ash
diff --git a/ash/system/chromeos/network/network_connect.h b/ash/system/chromeos/network/network_connect.h
new file mode 100644
index 0000000..e2eee94
--- /dev/null
+++ b/ash/system/chromeos/network/network_connect.h
@@ -0,0 +1,21 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_SYSTEM_CHROMEOS_NETWORK_NETWORK_CONNECT_H
+#define ASH_SYSTEM_CHROMEOS_NETWORK_NETWORK_CONNECT_H
+
+#include <string>
+
+#include "ash/ash_export.h"
+
+namespace ash {
+namespace network_connect {
+
+// Request a network connection and handle any errors and notifications.
+ASH_EXPORT void ConnectToNetwork(const std::string& service_path);
+
+}  // network_connect
+}  // ash
+
+#endif  // ASH_SYSTEM_CHROMEOS_NETWORK_NETWORK_CONNECT_H
diff --git a/ash/system/chromeos/network/network_state_list_detailed_view.cc b/ash/system/chromeos/network/network_state_list_detailed_view.cc
index a55e2a8..938662e 100644
--- a/ash/system/chromeos/network/network_state_list_detailed_view.cc
+++ b/ash/system/chromeos/network/network_state_list_detailed_view.cc
@@ -7,6 +7,7 @@
 #include "ash/root_window_controller.h"
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
+#include "ash/system/chromeos/network/network_connect.h"
 #include "ash/system/chromeos/network/network_icon.h"
 #include "ash/system/chromeos/network/network_icon_animation.h"
 #include "ash/system/chromeos/network/tray_network_state_observer.h"
@@ -24,7 +25,6 @@
 #include "base/utf_string_conversions.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/network/device_state.h"
-#include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "grit/ash_resources.h"
@@ -40,7 +40,6 @@
 #include "ui/views/widget/widget.h"
 
 using chromeos::DeviceState;
-using chromeos::NetworkConnectionHandler;
 using chromeos::NetworkHandler;
 using chromeos::NetworkState;
 using chromeos::NetworkStateHandler;
@@ -276,7 +275,13 @@ void NetworkStateListDetailedView::OnViewClicked(views::View* sender) {
       Shell::GetInstance()->system_tray_delegate()->ShowNetworkSettings(
           service_path);
     } else {
-      ConnectToNetwork(service_path);
+      if (CommandLine::ForCurrentProcess()->HasSwitch(
+              chromeos::switches::kUseNewNetworkConnectionHandler)) {
+        ash::network_connect::ConnectToNetwork(service_path);
+      } else {
+        Shell::GetInstance()->system_tray_delegate()->ConnectToNetwork(
+            service_path);
+      }
     }
   }
 }
@@ -769,49 +774,6 @@ views::View* NetworkStateListDetailedView::CreateNetworkInfoView() {
   return container;
 }
 
-void NetworkStateListDetailedView::ConnectToNetwork(
-    const std::string& service_path) {
-  const NetworkState* network = NetworkHandler::Get()->network_state_handler()->
-      GetNetworkState(service_path);
-  if (!network)
-    return;
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kUseNewNetworkConfigurationHandlers)) {
-    const bool ignore_error_state = false;
-    NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
-        service_path,
-        base::Bind(&base::DoNothing),
-        base::Bind(&NetworkStateListDetailedView::OnConnectFailed,
-                   AsWeakPtr(), service_path),
-        ignore_error_state);
-  } else {
-    Shell::GetInstance()->system_tray_delegate()->ConnectToNetwork(
-        service_path);
-  }
-}
-
-void NetworkStateListDetailedView::OnConnectFailed(
-    const std::string& service_path,
-    const std::string& error_name,
-    scoped_ptr<base::DictionaryValue> error_data) {
-  if (error_name == NetworkConnectionHandler::kErrorNotFound)
-    return;
-  if (error_name == NetworkConnectionHandler::kErrorPassphraseRequired) {
-    // TODO(stevenjb): Add inline UI to handle passphrase entry here.
-    Shell::GetInstance()->system_tray_delegate()->ConfigureNetwork(
-        service_path);
-    return;
-  }
-  if (error_name == NetworkConnectionHandler::kErrorCertificateRequired ||
-      error_name == NetworkConnectionHandler::kErrorConfigurationRequired) {
-    Shell::GetInstance()->system_tray_delegate()->ConfigureNetwork(
-        service_path);
-    return;
-  }
-  Shell::GetInstance()->system_tray_delegate()->ShowNetworkSettings(
-      service_path);
-}
-
 void NetworkStateListDetailedView::CallRequestScan() {
   VLOG(1) << "Requesting Network Scan.";
   NetworkHandler::Get()->network_state_handler()->RequestScan();
diff --git a/ash/system/chromeos/network/network_state_list_detailed_view.h b/ash/system/chromeos/network/network_state_list_detailed_view.h
index 9f689d6..6abeb72 100644
--- a/ash/system/chromeos/network/network_state_list_detailed_view.h
+++ b/ash/system/chromeos/network/network_state_list_detailed_view.h
@@ -107,14 +107,6 @@ class NetworkStateListDetailedView
   bool ResetInfoBubble();
   views::View* CreateNetworkInfoView();
 
-  // Handle click (connect) action.
-  void ConnectToNetwork(const std::string& service_path);
-
-  // Handle connect failures (new handlers only).
-  void OnConnectFailed(const std::string& service_path,
-                       const std::string& error_name,
-                       scoped_ptr<base::DictionaryValue> error_data);
-
   // Periodically request a network scan.
   void CallRequestScan();
 
diff --git a/ash/system/chromeos/network/network_state_notifier.cc b/ash/system/chromeos/network/network_state_notifier.cc
index bf4edce..a00f7bc 100644
--- a/ash/system/chromeos/network/network_state_notifier.cc
+++ b/ash/system/chromeos/network/network_state_notifier.cc
@@ -10,6 +10,7 @@
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
+#include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
@@ -17,6 +18,7 @@
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/base/l10n/l10n_util.h"
 
+using chromeos::NetworkConnectionHandler;
 using chromeos::NetworkHandler;
 using chromeos::NetworkState;
 using chromeos::NetworkStateHandler;
@@ -25,7 +27,19 @@ namespace {
 
 const int kMinTimeBetweenOutOfCreditsNotifySeconds = 10 * 60;
 
-string16 GetErrorString(const std::string& error) {
+// Error messages based on |error_name|, not network_state->error().
+string16 GetConnectErrorString(const std::string& error_name) {
+  if (error_name == NetworkConnectionHandler::kErrorNotFound)
+    return l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_CONNECT_FAILED);
+  return string16();
+}
+
+// Error messages based on network_state->error().
+string16 GetErrorString(const NetworkState* network_state) {
+  DCHECK(network_state);
+  const std::string& error = network_state->error();
+  if (error.empty())
+    return string16();
   if (error == flimflam::kErrorOutOfRange)
     return l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_OUT_OF_RANGE);
   if (error == flimflam::kErrorPinMissing)
@@ -137,17 +151,7 @@ void NetworkStateNotifier::NetworkConnectionStateChanged(
 
   NET_LOG_EVENT("ConnectionFailure", network->path());
 
-  std::vector<string16> no_links;
-  string16 error = GetErrorString(network->error());
-  ash::Shell::GetInstance()->system_tray_notifier()->NotifySetNetworkMessage(
-      this,
-      NetworkObserver::ERROR_CONNECT_FAILED,
-      NetworkObserver::GetNetworkTypeForNetworkState(network),
-      l10n_util::GetStringUTF16(IDS_NETWORK_CONNECTION_ERROR_TITLE),
-      l10n_util::GetStringFUTF16(
-            IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_DETAILS,
-            UTF8ToUTF16(network->name()), error),
-      no_links);
+  ShowConnectError("", network);
 }
 
 void NetworkStateNotifier::NetworkPropertiesUpdated(
@@ -195,6 +199,42 @@ void NetworkStateNotifier::NotificationLinkClicked(
   }
 }
 
+void NetworkStateNotifier::ShowNetworkConnectError(
+    const std::string& error_name,
+    const std::string& service_path) {
+  const NetworkState* network = NetworkHandler::Get()->network_state_handler()->
+      GetNetworkState(service_path);
+  ShowConnectError(error_name, network);
+}
+
+void NetworkStateNotifier::ShowConnectError(const std::string& error_name,
+                                            const NetworkState* network) {
+  std::vector<string16> no_links;
+  string16 error = GetConnectErrorString(error_name);
+  if (error.empty() && network)
+    error = GetErrorString(network);
+  if (error.empty())
+    error = l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_UNKNOWN);
+  std::string name = network ? network->name() : "";
+  string16 error_msg;
+  if (network && !network->error_details().empty()) {
+    error_msg = l10n_util::GetStringFUTF16(
+        IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_SERVER_MESSAGE,
+        UTF8ToUTF16(name), error, UTF8ToUTF16(network->error_details()));
+  } else {
+    error_msg = l10n_util::GetStringFUTF16(
+        IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_DETAILS,
+        UTF8ToUTF16(name), error);
+  }
+  ash::Shell::GetInstance()->system_tray_notifier()->NotifySetNetworkMessage(
+      this,
+      NetworkObserver::ERROR_CONNECT_FAILED,
+      NetworkObserver::GetNetworkTypeForNetworkState(network),
+      l10n_util::GetStringUTF16(IDS_NETWORK_CONNECTION_ERROR_TITLE),
+      error_msg,
+      no_links);
+}
+
 void NetworkStateNotifier::InitializeNetworks() {
   NetworkStateList network_list;
   NetworkHandler::Get()->network_state_handler()->GetNetworkList(&network_list);
diff --git a/ash/system/chromeos/network/network_state_notifier.h b/ash/system/chromeos/network/network_state_notifier.h
index 9fce0a2..89d46c3 100644
--- a/ash/system/chromeos/network/network_state_notifier.h
+++ b/ash/system/chromeos/network/network_state_notifier.h
@@ -14,10 +14,18 @@
 #include "base/time.h"
 #include "chromeos/network/network_state_handler_observer.h"
 
+namespace chromeos {
+class NetworkState;
+}
+
 namespace ash {
 
 // This class observes NetworkStateHandler and generates notifications
-// on connection failures.
+// on connection failures. NOTE: The Observer for this class only triggers
+// "Out of credits" notifications, and failures triggered from NetworkLibrary
+// calls (which sets NetworkStateHandler::connecting_network()).
+// Failures from NetworkStateListDetailedView::ConnectToNetwork are now
+// handled by the ConnectToNetwork callbacks.
 class ASH_EXPORT NetworkStateNotifier :
       public chromeos::NetworkStateHandlerObserver,
       public NetworkTrayDelegate {
@@ -38,9 +46,16 @@ class ASH_EXPORT NetworkStateNotifier :
       NetworkObserver::MessageType message_type,
       size_t link_index) OVERRIDE;
 
+  // Show a connection error notification.
+  void ShowNetworkConnectError(const std::string& error_name,
+                               const std::string& service_path);
+
  private:
   typedef std::map<std::string, std::string> CachedStateMap;
 
+  void ShowConnectError(const std::string& error_name,
+                        const chromeos::NetworkState* network);
+
   void InitializeNetworks();
 
   CachedStateMap cached_state_;
diff --git a/ash/system/chromeos/network/tray_network.cc b/ash/system/chromeos/network/tray_network.cc
index b554529..aa24989 100644
--- a/ash/system/chromeos/network/tray_network.cc
+++ b/ash/system/chromeos/network/tray_network.cc
@@ -9,6 +9,7 @@
 #include "ash/system/chromeos/network/network_icon_animation.h"
 #include "ash/system/chromeos/network/network_state_list_detailed_view.h"
 #include "ash/system/chromeos/network/network_tray_delegate.h"
+#include "ash/system/chromeos/network/tray_network_state_observer.h"
 #include "ash/system/tray/system_tray.h"
 #include "ash/system/tray/system_tray_delegate.h"
 #include "ash/system/tray/system_tray_notifier.h"
@@ -19,6 +20,7 @@
 #include "ash/system/tray/tray_utils.h"
 #include "base/command_line.h"
 #include "base/utf_string_conversions.h"
+#include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "grit/ash_resources.h"
@@ -33,6 +35,7 @@
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
 
+using chromeos::NetworkConnectionHandler;
 using chromeos::NetworkHandler;
 using chromeos::NetworkState;
 using chromeos::NetworkStateHandler;
@@ -540,13 +543,20 @@ void TrayNetwork::GetNetworkStateHandlerImageAndLabel(
     gfx::ImageSkia* image,
     base::string16* label,
     bool* animating) {
-  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
-  const NetworkState* connected_network = handler->ConnectedNetworkByType(
-      NetworkStateHandler::kMatchTypeNonVirtual);
-  const NetworkState* connecting_network = handler->ConnectingNetworkByType(
-      NetworkStateHandler::kMatchTypeWireless);
-  if (!connecting_network && icon_type == network_icon::ICON_TYPE_TRAY)
-    connecting_network = handler->ConnectingNetworkByType(flimflam::kTypeVPN);
+  NetworkStateHandler* state_handler =
+      NetworkHandler::Get()->network_state_handler();
+  NetworkConnectionHandler* connect_handler =
+      NetworkHandler::Get()->network_connection_handler();
+  const NetworkState* connected_network =
+      state_handler->ConnectedNetworkByType(
+          NetworkStateHandler::kMatchTypeNonVirtual);
+  const NetworkState* connecting_network =
+      state_handler->ConnectingNetworkByType(
+          NetworkStateHandler::kMatchTypeWireless);
+  if (!connecting_network && icon_type == network_icon::ICON_TYPE_TRAY) {
+    connecting_network =
+        state_handler->ConnectingNetworkByType(flimflam::kTypeVPN);
+  }
 
   const NetworkState* network;
   // If we are connecting to a network, and there is either no connected
@@ -554,7 +564,8 @@ void TrayNetwork::GetNetworkStateHandlerImageAndLabel(
   // network.
   if (connecting_network &&
       (!connected_network ||
-       handler->connecting_network() == connecting_network->path())) {
+       state_handler->connecting_network() == connecting_network->path() ||
+       connect_handler->HasConnectingNetwork(connecting_network->path()))) {
     network = connecting_network;
   } else {
     network = connected_network;
@@ -570,7 +581,7 @@ void TrayNetwork::GetNetworkStateHandlerImageAndLabel(
 
   if (!network) {
     // If no connecting network, check if we are activating a network.
-    const NetworkState* mobile_network = handler->FirstNetworkByType(
+    const NetworkState* mobile_network = state_handler->FirstNetworkByType(
         NetworkStateHandler::kMatchTypeMobile);
     if (mobile_network && (mobile_network->activation_state() ==
                            flimflam::kActivationStateActivating)) {