Eliminate c/b/chromeos/options/network_connect.cc

ash/system/chromeos/network/network_state_notifier.cc

Index: ash/system/chromeos/network/network_state_notifier.cc
diff --git a/ash/system/chromeos/network/network_state_notifier.cc b/ash/system/chromeos/network/network_state_notifier.cc
index 1c971a32a44360d4df4b71d6c223f8e70343b96d..ac7b0264debf1bedad22d47161ec9a72eb45656e 100644
--- a/ash/system/chromeos/network/network_state_notifier.cc
+++ b/ash/system/chromeos/network/network_state_notifier.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.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_state.h"
@@ -41,12 +42,19 @@ string16 GetConnectErrorString(const std::string& error_name) {
   return string16();
 }
 
+void ConfigureNetwork(const std::string& service_path) {
+  // Note: this will trigger the correct UI for out of credit notifications.

[pneubeck (no reviews), 2013/08/20 08:32:26]

nit:
if you depend on that behavior, it should be mentioned in the method comment of
SystemTrayDelegate::ConfigureNetwork

[stevenjb, 2013/08/20 20:47:43]

Removed this (unused) and updated SystemTrayDelegate::ConfigureNetwork comment

+  ash::Shell::GetInstance()->system_tray_delegate()->ConfigureNetwork(
+      service_path);
+}
+
 }  // namespace
 
 namespace ash {
 
 NetworkStateNotifier::NetworkStateNotifier()
-    : cellular_out_of_credits_(false) {
+    : cellular_out_of_credits_(false),
+      weak_ptr_factory_(this) {
   if (!NetworkHandler::IsInitialized())
     return;
   NetworkHandler::Get()->network_state_handler()->AddObserver(this, FROM_HERE);
@@ -65,16 +73,6 @@ NetworkStateNotifier::~NetworkStateNotifier() {
       this, FROM_HERE);
 }
 
-void NetworkStateNotifier::NetworkListChanged() {
-  // Trigger any pending connect failed error if the network list changes
-  // (which indicates all NetworkState entries are up to date). This is in
-  // case a connect attempt fails because a network is no longer visible.
-  if (!connect_failed_network_.empty()) {
-    ShowNetworkConnectError(
-        NetworkConnectionHandler::kErrorConnectFailed, connect_failed_network_);
-  }
-}
-
 void NetworkStateNotifier::DefaultNetworkChanged(const NetworkState* network) {
   if (!network || !network->IsConnectedState())
     return;
@@ -82,43 +80,36 @@ void NetworkStateNotifier::DefaultNetworkChanged(const NetworkState* network) {
     last_active_network_ = network->path();
     // Reset state for new connected network
     cellular_out_of_credits_ = false;
+    NetworkPropertiesUpdated(network);
   }
 }
 
 void NetworkStateNotifier::NetworkPropertiesUpdated(
     const NetworkState* network) {
-  DCHECK(network);
-  // Trigger a pending connect failed error for |network| when the Error
-  // property has been set.
-  if (network->path() == connect_failed_network_ && !network->error().empty()) {
-    ShowNetworkConnectError(
-        NetworkConnectionHandler::kErrorConnectFailed, connect_failed_network_);
-  }
   // Trigger "Out of credits" notification if the cellular network is the most
   // recent default network (i.e. we have not switched to another network).
-  if (network->type() == flimflam::kTypeCellular &&
-      network->path() == last_active_network_) {
-    cellular_network_ = network->path();
-    if (network->cellular_out_of_credits() &&
-        !cellular_out_of_credits_) {
-      cellular_out_of_credits_ = true;
-      base::TimeDelta dtime = base::Time::Now() - out_of_credits_notify_time_;
-      if (dtime.InSeconds() > kMinTimeBetweenOutOfCreditsNotifySeconds) {
-        out_of_credits_notify_time_ = base::Time::Now();
-        std::vector<string16> links;
-        links.push_back(
-            l10n_util::GetStringFUTF16(IDS_NETWORK_OUT_OF_CREDITS_LINK,
-                                       UTF8ToUTF16(network->name())));
-        ash::Shell::GetInstance()->system_tray_notifier()->
-            NotifySetNetworkMessage(
-                this,
-                NetworkObserver::ERROR_OUT_OF_CREDITS,
-                NetworkObserver::GetNetworkTypeForNetworkState(network),
-                l10n_util::GetStringUTF16(IDS_NETWORK_OUT_OF_CREDITS_TITLE),
-                l10n_util::GetStringUTF16(IDS_NETWORK_OUT_OF_CREDITS_BODY),
-                links);
-      }
-    }
+  if (cellular_out_of_credits_ ||
+      network->type() != flimflam::kTypeCellular ||
+      network->path() != last_active_network_ ||

[pneubeck (no reviews), 2013/08/20 08:32:26]

if you return in this case (network->path() != last_active_network_)
Why then even listen for/implement NetworkPropertiesUpdated? Isn't
DefaultNetworkChanged then sufficient (which also notifies about state changes)?

If there's a good reason, please document it.

[stevenjb, 2013/08/20 20:47:43]

I greatly simplify this logic in the next CL, I didn't want to change too much
here, although it looks like I inadvertently changed the existing logic slightly
so I will revert that in this CL.

+      !network->cellular_out_of_credits())
+    return;
+  cellular_network_ = network->path();
+  cellular_out_of_credits_ = true;
+  base::TimeDelta dtime = base::Time::Now() - out_of_credits_notify_time_;
+  if (dtime.InSeconds() > kMinTimeBetweenOutOfCreditsNotifySeconds) {
+    out_of_credits_notify_time_ = base::Time::Now();
+    std::vector<string16> links;
+    links.push_back(
+        l10n_util::GetStringFUTF16(IDS_NETWORK_OUT_OF_CREDITS_LINK,
+                                   UTF8ToUTF16(network->name())));
+    ash::Shell::GetInstance()->system_tray_notifier()->
+        NotifySetNetworkMessage(
+            this,
+            NetworkObserver::ERROR_OUT_OF_CREDITS,
+            NetworkObserver::GetNetworkTypeForNetworkState(network),
+            l10n_util::GetStringUTF16(IDS_NETWORK_OUT_OF_CREDITS_TITLE),
+            l10n_util::GetStringUTF16(IDS_NETWORK_OUT_OF_CREDITS_BODY),
+            links);
   }
 }
 
@@ -139,43 +130,92 @@ 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);
-  if (error_name == NetworkConnectionHandler::kErrorConnectFailed &&
-      service_path != connect_failed_network_) {
-    // Shill may not have set the Error property yet. First request an update
-    // and wait for either the update to complete or the network list to be
-    // updated before displaying the error.
-    connect_failed_network_ = service_path;
-    return;
-  }
-  connect_failed_network_.clear();
+  // Get the up-to-date properties for the network and display the error.
+  NetworkHandler::Get()->network_configuration_handler()->GetProperties(
+      service_path,
+      base::Bind(&NetworkStateNotifier::ConnectErrorPropertiesSucceeded,
+                 weak_ptr_factory_.GetWeakPtr(), error_name),
+      base::Bind(&NetworkStateNotifier::ConnectErrorPropertiesFailed,
+                 weak_ptr_factory_.GetWeakPtr(), error_name, service_path));
+}
 
+void NetworkStateNotifier::ConnectErrorPropertiesSucceeded(
+    const std::string& error_name,
+    const std::string& service_path,
+    const base::DictionaryValue& shill_properties) {
+  ShowConnectErrorNotification(error_name, service_path, shill_properties);
+}
+
+void NetworkStateNotifier::ConnectErrorPropertiesFailed(
+    const std::string& error_name,
+    const std::string& service_path,
+    const std::string& shill_error_name,
+    scoped_ptr<base::DictionaryValue> shill_error_data) {
+  base::DictionaryValue shill_properties;
+  ShowConnectErrorNotification(error_name, service_path, shill_properties);
+}
+
+void NetworkStateNotifier::ShowConnectErrorNotification(
+    const std::string& error_name,
+    const std::string& service_path,
+    const base::DictionaryValue& shill_properties) {
   string16 error = GetConnectErrorString(error_name);
-  if (error.empty() && network)
-    error = network_connect::ErrorString(network->error());
-  if (error.empty())
-    error = l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_UNKNOWN);
+  if (error.empty()) {
+    std::string network_error;
+    shill_properties.GetStringWithoutPathExpansion(
+        flimflam::kErrorProperty, &network_error);
+    error = network_connect::ErrorString(network_error);
+    if (error.empty())
+      error = l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_UNKNOWN);
+  }
   NET_LOG_ERROR("Connect error notification: " + UTF16ToUTF8(error),
                 service_path);
 
-  std::string name = network ? network->name() : "";
+  std::string network_name =
+      NetworkState::GetNameFromProperties(service_path, shill_properties);

[pneubeck (no reviews), 2013/08/20 08:32:26]

GetNameFromProperties should explicitly handle shill_properties.empty() if you
use it that way here.

[stevenjb, 2013/08/20 20:47:43]

Done.

+  std::string network_error_details;
+  shill_properties.GetStringWithoutPathExpansion(
+        shill::kErrorDetailsProperty, &network_error_details);
+
   string16 error_msg;

[pneubeck (no reviews), 2013/08/20 08:32:26]

network_name will be empty here if shill_properties is empty.

[stevenjb, 2013/08/20 20:47:43]

Yes, that logic hasn't changed; I will address that in the next CL when we
switch to using message_center, otherwise the change may get lost.

-  if (network && !network->error_details().empty()) {
+  if (!network_error_details.empty()) {
     error_msg = l10n_util::GetStringFUTF16(
         IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_SERVER_MESSAGE,
-        UTF8ToUTF16(name), error, UTF8ToUTF16(network->error_details()));
+        UTF8ToUTF16(network_name), error,
+        UTF8ToUTF16(network_error_details));
   } else {
     error_msg = l10n_util::GetStringFUTF16(
         IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_DETAILS,

[pneubeck (no reviews), 2013/08/20 08:32:26]

nit:
the names of the IDS_* constants are confusing:
here it's called ..._WITH_DETAILS, but details are only provided in the first
if-case.

[stevenjb, 2013/08/20 20:47:43]

I will also address that in the next CL.

-        UTF8ToUTF16(name), error);
+        UTF8ToUTF16(network_name), error);
+  }
+
+  std::string network_type;
+  shill_properties.GetStringWithoutPathExpansion(
+      flimflam::kTypeProperty, &network_type);
+
+  NetworkObserver::NetworkType type = NetworkObserver::NETWORK_UNKNOWN;

[pneubeck (no reviews), 2013/08/20 08:32:26]

this duplicates the type mapping.
could you instead change GetNetworkTypeForNetworkState from a NetworkState
argument to  (std::string& shill_network_type, std::string&
shill_network_technology) arguments?

[stevenjb, 2013/08/20 20:47:43]

This is temporary and going away soon.

+  if (network_type == flimflam::kTypeCellular) {
+    std::string network_technology;
+    shill_properties.GetStringWithoutPathExpansion(
+        flimflam::kNetworkTechnologyProperty, &network_technology);
+    if (network_technology == flimflam::kNetworkTechnologyLte ||
+        network_technology == flimflam::kNetworkTechnologyLteAdvanced)
+      type = NetworkObserver::NETWORK_CELLULAR_LTE;
+    else
+      type = NetworkObserver::NETWORK_CELLULAR;
+  } else if (network_type == flimflam::kTypeEthernet) {
+    type = NetworkObserver:: NETWORK_ETHERNET;
+  } else if (network_type == flimflam::kTypeWifi) {
+    type = NetworkObserver:: NETWORK_WIFI;
+  } else {
+    NOTREACHED();
   }
 
   std::vector<string16> no_links;
   ash::Shell::GetInstance()->system_tray_notifier()->NotifySetNetworkMessage(
       this,
       NetworkObserver::ERROR_CONNECT_FAILED,
-      NetworkObserver::GetNetworkTypeForNetworkState(network),
+      type,
       l10n_util::GetStringUTF16(IDS_NETWORK_CONNECTION_ERROR_TITLE),
       error_msg,
       no_links);