Improve network error notifications and cleanup code

This CL does the following:
* Stores last_error in NetworkState
* Uses Service.PreviousError when Serice.Error is empty
* Relies on Service + NetworkState properties instead of awkwardly passing shill errors around (inconsistently)

Note: This CL was developed in parallel to PreviousError in Shill. Rather than revert the last_error changes, I decided to track both and LOG if we use one or the other under the assumption that more information is better.

BUG=333955,344398,337608,332620
R=armansito@chromium.org, pneubeck@chromium.org

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

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

ash/ash_chromeos_strings.grdp

@@ -161,7 +161,7 @@ Failed to connect to '<ph name="name">$1<ex>GoogleGuest</ex></ph>': <ph name="de
 Server message: <ph name="server_msg">$3<ex>Incorrect password</ex></ph>
   </message>
   <message name="IDS_NETWORK_CONNECTION_ERROR_MESSAGE_NO_NAME" desc="Message for network connection error where the network name is unavailable">
-    Failed to connect to network: <ph name="details">$2<ex>Unrecognized error</ex></ph>
+    Failed to connect to network: <ph name="details">$1<ex>Unrecognized error</ex></ph>
   </message>
   <message name="IDS_NETWORK_OUT_OF_CREDITS_TITLE" desc="Title for network out of data error notification">
     Network Connection Error

ash/system/chromeos/network/network_connect.cc

@@ -60,10 +60,9 @@ bool IsDirectActivatedCarrier(const std::string& carrier) {
 }
 
 void ShowErrorNotification(const std::string& error_name,
-                           const std::string& shill_error,
                            const std::string& service_path) {
   Shell::GetInstance()->system_tray_notifier()->network_state_notifier()->
-      ShowNetworkConnectError(error_name, shill_error, service_path);
+      ShowNetworkConnectError(error_name, service_path);
 }
 
 void HandleUnconfiguredNetwork(const std::string& service_path,
@@ -151,10 +150,7 @@ void OnConnectFailed(const std::string& service_path,
   }
 
   // ConnectFailed or unknown error; show a notification.
-  std::string shill_error;
-  error_data.get()->GetString(
-      chromeos::network_handler::kErrorDetail, &shill_error);
-  ShowErrorNotification(error_name, shill_error, service_path);
+  ShowErrorNotification(error_name, service_path);
 
   // Only show a configure dialog if there was a ConnectFailed error and the
   // screen is not locked.
@@ -204,8 +200,7 @@ 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);
-  ShowErrorNotification(
-      network_connect::kErrorActivateFailed, "", service_path);
+  ShowErrorNotification(network_connect::kErrorActivateFailed, service_path);
 }
 
 void OnActivateSucceeded(const std::string& service_path) {
@@ -215,8 +210,7 @@ void OnActivateSucceeded(const std::string& service_path) {
 void OnConfigureFailed(const std::string& error_name,
                        scoped_ptr<base::DictionaryValue> error_data) {
   NET_LOG_ERROR("Unable to configure network", "");
-  ShowErrorNotification(
-      NetworkConnectionHandler::kErrorConfigureFailed, "", "");
+  ShowErrorNotification(NetworkConnectionHandler::kErrorConfigureFailed, "");
 }
 
 void OnConfigureSucceeded(const std::string& service_path) {
@@ -233,7 +227,7 @@ void SetPropertiesFailed(const std::string& desc,
                          scoped_ptr<base::DictionaryValue> error_data) {
   NET_LOG_ERROR(desc + ": Failed: " + config_error_name, service_path);
   ShowErrorNotification(
-      NetworkConnectionHandler::kErrorConfigureFailed, "", service_path);
+      NetworkConnectionHandler::kErrorConfigureFailed, service_path);
 }
 
 void SetPropertiesToClear(base::DictionaryValue* properties_to_set,
@@ -327,8 +321,8 @@ void ConnectToNetwork(const std::string& service_path,
                       gfx::NativeWindow parent_window) {
   NET_LOG_USER("ConnectToNetwork", service_path);
   const NetworkState* network = GetNetworkState(service_path);
-  if (network && !network->error().empty()) {
-    NET_LOG_USER("Configure: " + network->error(), service_path);
+  if (network && !network->last_error().empty()) {
+    NET_LOG_USER("Configure: " + network->last_error(), service_path);
     // If the network is in an error state, show the configuration UI directly
     // to avoid a spurious notification.
     HandleUnconfiguredNetwork(service_path, parent_window);
@@ -439,7 +433,7 @@ void ShowMobileSetup(const std::string& service_path) {
                                        base::UTF8ToUTF16(cellular->name())),
             ui::ResourceBundle::GetSharedInstance().GetImageNamed(
                 IDR_AURA_UBER_TRAY_CELLULAR_NETWORK_FAILED),
-            ash::system_notifier::kNotifierNetwork,
+            ash::system_notifier::kNotifierNetworkError,
             base::Bind(&ash::network_connect::ShowNetworkSettings,
                        service_path)));
     return;
@@ -458,7 +452,7 @@ void ConfigureNetworkAndConnect(const std::string& service_path,
   std::string profile_path;
   if (!GetNetworkProfilePath(shared, &profile_path)) {
     ShowErrorNotification(
-        NetworkConnectionHandler::kErrorConfigureFailed, "", service_path);
+        NetworkConnectionHandler::kErrorConfigureFailed, service_path);
     return;
   }
   NetworkHandler::Get()->network_configuration_handler()->SetNetworkProfile(
@@ -474,8 +468,7 @@ void CreateConfigurationAndConnect(base::DictionaryValue* properties,
   NET_LOG_USER("CreateConfigurationAndConnect", "");
   std::string profile_path;
   if (!GetNetworkProfilePath(shared, &profile_path)) {
-    ShowErrorNotification(
-        NetworkConnectionHandler::kErrorConfigureFailed, "", "");
+    ShowErrorNotification(NetworkConnectionHandler::kErrorConfigureFailed, "");
     return;
   }
   properties->SetStringWithoutPathExpansion(

ash/system/chromeos/network/network_state_notifier.cc

@@ -38,6 +38,13 @@ const char kNetworkOutOfCreditsNotificationId[] =
 
 const int kMinTimeBetweenOutOfCreditsNotifySeconds = 10 * 60;
 
+// Ignore in-progress error.
+bool ShillErrorIsIgnored(const std::string& shill_error) {
+  if (shill_error == shill::kErrorResultInProgress)
+    return true;
+  return false;
+}
+
 // Error messages based on |error_name|, not network_state->error().
 base::string16 GetConnectErrorString(const std::string& error_name) {
   if (error_name == NetworkConnectionHandler::kErrorNotFound)
@@ -191,27 +198,23 @@ void NetworkStateNotifier::UpdateCellularActivating(
 
 void NetworkStateNotifier::ShowNetworkConnectError(
     const std::string& error_name,
-    const std::string& shill_error,
     const std::string& service_path) {
   if (service_path.empty()) {
     base::DictionaryValue shill_properties;
-    ShowConnectErrorNotification(error_name, shill_error, service_path,
-                                 shill_properties);
+    ShowConnectErrorNotification(error_name, service_path, shill_properties);
     return;
   }
   // 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, shill_error),
+                 weak_ptr_factory_.GetWeakPtr(), error_name),
       base::Bind(&NetworkStateNotifier::ConnectErrorPropertiesFailed,
-                 weak_ptr_factory_.GetWeakPtr(), error_name, shill_error,
-                 service_path));
+                 weak_ptr_factory_.GetWeakPtr(), error_name, service_path));
 }
 
 void NetworkStateNotifier::ConnectErrorPropertiesSucceeded(
     const std::string& error_name,
-    const std::string& shill_error,
     const std::string& service_path,
     const base::DictionaryValue& shill_properties) {
   std::string state;
@@ -222,36 +225,52 @@ void NetworkStateNotifier::ConnectErrorPropertiesSucceeded(
     // Idle state transition occurs, see crbug.com/333955.
     return;
   }
-  ShowConnectErrorNotification(error_name, shill_error, service_path,
-                               shill_properties);
+  ShowConnectErrorNotification(error_name, service_path, shill_properties);
 }
 
 void NetworkStateNotifier::ConnectErrorPropertiesFailed(
     const std::string& error_name,
-    const std::string& shill_error,
     const std::string& service_path,
     const std::string& shill_connect_error,
     scoped_ptr<base::DictionaryValue> shill_error_data) {
   base::DictionaryValue shill_properties;
-  ShowConnectErrorNotification(error_name, shill_error, service_path,
-                               shill_properties);
+  ShowConnectErrorNotification(error_name, service_path, shill_properties);
 }
 
 void NetworkStateNotifier::ShowConnectErrorNotification(
     const std::string& error_name,
-    const std::string& shill_error,
     const std::string& service_path,
     const base::DictionaryValue& shill_properties) {
   base::string16 error = GetConnectErrorString(error_name);
   if (error.empty()) {
-    // Service.Error gets cleared shortly after State transitions to Failure,
-    // so rely on |shill_error| unless empty.
-    std::string network_error = shill_error;
-    if (network_error.empty()) {
+    std::string shill_error;
+    shill_properties.GetStringWithoutPathExpansion(shill::kErrorProperty,
+                                                   &shill_error);
+    if (shill_error.empty()) {
+      NET_LOG_DEBUG("Service.Error is empty, trying PreviousError",
+                    service_path);
       shill_properties.GetStringWithoutPathExpansion(
-          shill::kErrorProperty, &network_error);
+          shill::kPreviousErrorProperty, &shill_error);
     }
-    error = network_connect::ErrorString(network_error, service_path);
+    std::string last_error;
+    const NetworkState* network =
+        NetworkHandler::Get()->network_state_handler()->GetNetworkState(
+            service_path);
+    if (network)
+      last_error = network->last_error();
+    if (!last_error.empty() && last_error != shill_error) {
+      NET_LOG_DEBUG(
+          "last_error:" + last_error + " != Service.Error: " + shill_error,
+          service_path);
+      // Use Shill Error unless empty, since it is more recent.
+      if (shill_error.empty())
+        shill_error = last_error;
+    }
+    if (ShillErrorIsIgnored(shill_error)) {
+      NET_LOG_DEBUG("Ignoring error: " + error_name, service_path);
+      return;
+    }
+    error = network_connect::ErrorString(shill_error, service_path);
     if (error.empty())
       error = l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_UNKNOWN);
   }
@@ -262,28 +281,29 @@ void NetworkStateNotifier::ShowConnectErrorNotification(
       chromeos::shill_property_util::GetNameFromProperties(service_path,
                                                            shill_properties);
   std::string network_error_details;
-  shill_properties.GetStringWithoutPathExpansion(
-        shill::kErrorDetailsProperty, &network_error_details);
+  shill_properties.GetStringWithoutPathExpansion(shill::kErrorDetailsProperty,
+                                                 &network_error_details);
 
   base::string16 error_msg;
   if (!network_error_details.empty()) {
     // network_name should't be empty if network_error_details is set.
     error_msg = l10n_util::GetStringFUTF16(
         IDS_NETWORK_CONNECTION_ERROR_MESSAGE_WITH_SERVER_MESSAGE,
-        base::UTF8ToUTF16(network_name), error,
+        base::UTF8ToUTF16(network_name),
+        error,
         base::UTF8ToUTF16(network_error_details));
   } else if (network_name.empty()) {
     error_msg = l10n_util::GetStringFUTF16(
         IDS_NETWORK_CONNECTION_ERROR_MESSAGE_NO_NAME, error);
   } else {
-    error_msg = l10n_util::GetStringFUTF16(
-        IDS_NETWORK_CONNECTION_ERROR_MESSAGE,
-        base::UTF8ToUTF16(network_name), error);
+    error_msg = l10n_util::GetStringFUTF16(IDS_NETWORK_CONNECTION_ERROR_MESSAGE,
+                                           base::UTF8ToUTF16(network_name),
+                                           error);
   }
 
   std::string network_type;
-  shill_properties.GetStringWithoutPathExpansion(
-      shill::kTypeProperty, &network_type);
+  shill_properties.GetStringWithoutPathExpansion(shill::kTypeProperty,
+                                                 &network_type);
 
   ShowErrorNotification(
       network_connect::kNetworkConnectNotificationId,

ash/system/chromeos/network/network_state_notifier.h

@@ -46,28 +46,23 @@ class ASH_EXPORT NetworkStateNotifier :
 
   // Show a connection error notification. If |error_name| matches an error
   // defined in NetworkConnectionHandler for connect, configure, or activation
-  // failed, then the associated message is shown, otherwise |shill_error|
-  // is expected to contain Service.Error (which might get cleared before
-  // GetProperties returns).
+  // failed, then the associated message is shown; otherwise use the last_error
+  // value for the network or a Shill property if available.
   void ShowNetworkConnectError(const std::string& error_name,
-                               const std::string& shill_error,
                                const std::string& service_path);
 
  private:
   void ConnectErrorPropertiesSucceeded(
       const std::string& error_name,
-      const std::string& shill_error,
       const std::string& service_path,
       const base::DictionaryValue& shill_properties);
   void ConnectErrorPropertiesFailed(
       const std::string& error_name,
-      const std::string& shill_error,
       const std::string& service_path,
       const std::string& shill_connect_error,
       scoped_ptr<base::DictionaryValue> shill_error_data);
   void ShowConnectErrorNotification(
       const std::string& error_name,
-      const std::string& shill_error,
       const std::string& service_path,
       const base::DictionaryValue& shill_properties);
 

ash/system/system_notifier.cc

@@ -16,6 +16,7 @@ namespace {
 const char* kAlwaysShownNotifierIds[] = {
   kNotifierDisplay,
   kNotifierDisplayError,
+  kNotifierNetworkError,
   kNotifierPower,
   NULL
 };

chrome/browser/chromeos/options/vpn_config_view.cc

@@ -989,7 +989,8 @@ void VPNConfigView::UpdateErrorLabel() {
     const NetworkState* vpn = NetworkHandler::Get()->network_state_handler()->
         GetNetworkState(service_path_);
     if (vpn && vpn->connection_state() == shill::kStateFailure)
-      error_msg = ash::network_connect::ErrorString(vpn->error(), vpn->path());
+      error_msg = ash::network_connect::ErrorString(
+          vpn->last_error(), vpn->path());
   }
   if (!error_msg.empty()) {
     error_label_->SetText(error_msg);

chrome/browser/chromeos/options/wifi_config_view.cc

@@ -586,7 +586,7 @@ void WifiConfigView::UpdateErrorLabel() {
         GetNetworkState(service_path_);
     if (wifi && wifi->connection_state() == shill::kStateFailure)
       error_msg = ash::network_connect::ErrorString(
-          wifi->error(), wifi->path());
+          wifi->last_error(), wifi->path());
   }
   if (!error_msg.empty()) {
     error_label_->SetText(error_msg);

chrome/browser/chromeos/options/wimax_config_view.cc

@@ -104,7 +104,7 @@ void WimaxConfigView::UpdateErrorLabel() {
         GetNetworkState(service_path_);
     if (wimax && wimax->connection_state() == shill::kStateFailure)
       error_msg = ash::network_connect::ErrorString(
-          wimax->error(), wimax->path());
+          wimax->last_error(), wimax->path());
   }
   if (!error_msg.empty()) {
     error_label_->SetText(error_msg);

chromeos/network/network_connection_handler.cc

@@ -33,10 +33,9 @@ namespace {
 void InvokeErrorCallback(const std::string& service_path,
                          const network_handler::ErrorCallback& error_callback,
                          const std::string& error_name) {
-  std::string error_msg = "Connect Error: " + error_name;
-  NET_LOG_ERROR(error_msg, service_path);
+  NET_LOG_ERROR("Connect Error: " + error_name, service_path);
   network_handler::RunErrorCallback(
-      error_callback, service_path, error_name, error_msg);
+      error_callback, service_path, error_name, "");
 }
 
 bool IsAuthenticationError(const std::string& error) {
@@ -253,7 +252,7 @@ void NetworkConnectionHandler::ConnectToNetwork(
     }
 
     if (check_error_state) {
-      const std::string& error = network->error();
+      const std::string& error = network->last_error();
       if (error == shill::kErrorBadPassphrase) {
         InvokeErrorCallback(service_path, error_callback, error);
         return;
@@ -527,6 +526,7 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
 void NetworkConnectionHandler::CallShillConnect(
     const std::string& service_path) {
   NET_LOG_EVENT("Sending Connect Request to Shill", service_path);
+  network_state_handler_->ClearLastErrorForNetwork(service_path);
   DBusThreadManager::Get()->GetShillServiceClient()->Connect(
       dbus::ObjectPath(service_path),
       base::Bind(&NetworkConnectionHandler::HandleShillConnectSuccess,
@@ -622,9 +622,6 @@ void NetworkConnectionHandler::CheckPendingRequest(
 
   // Network is neither connecting or connected; an error occurred.
   std::string error_name;  // 'Canceled' or 'Failed'
-  // If network->error() is empty here, we will look it up later, but we
-  // need to preserve it in case Shill clears it before then. crbug.com/302020.
-  std::string shill_error = network->error();
   if (network->connection_state() == shill::kStateIdle &&
       pending_requests_.size() > 1) {
     // Another connect request canceled this one.
@@ -636,17 +633,14 @@ void NetworkConnectionHandler::CheckPendingRequest(
                     service_path);
     }
   }
-  std::string error_msg = error_name;
-  if (!shill_error.empty())
-    error_msg += ": " + shill_error;
-  NET_LOG_ERROR(error_msg, service_path);
 
   network_handler::ErrorCallback error_callback = request->error_callback;
   pending_requests_.erase(service_path);
-  if (error_callback.is_null())
+  if (error_callback.is_null()) {
+    NET_LOG_ERROR("Connect Error, no callback: " + error_name, service_path);
     return;
-  network_handler::RunErrorCallback(
-      error_callback, service_path, error_name, shill_error);
+  }
+  InvokeErrorCallback(service_path, error_callback, error_name);
 }
 
 void NetworkConnectionHandler::CheckAllPendingRequests() {

chromeos/network/network_state.cc

@@ -88,6 +88,8 @@ bool NetworkState::PropertyChanged(const std::string& key,
     // Shill uses "Unknown" to indicate an unset error state.
     if (error_ == kErrorUnknown)
       error_.clear();
+    if (!error_.empty())
+      last_error_ = error_;
     return true;
   } else if (key == IPConfigProperty(shill::kAddressProperty)) {
     return GetStringValue(key, value, &ip_address_);