Fix network config UI

This CL cleans up the network config UI to improve the display
of 'Connecting' and improve error handling and display.

[email protected]

(cherry picked from commit a9bc09d)

Bug: 795715, 795698
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I2f2dc1654c29524e5b7ae99cfbe43692a9fdb470
Reviewed-on: https://chromium-review.googlesource.com/874605
Commit-Queue: Steven Bennetts <[email protected]>
Reviewed-by: Toni Barzic <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#530967}
Reviewed-on: https://chromium-review.googlesource.com/882550
Reviewed-by: Steven Bennetts <[email protected]>
Cr-Commit-Position: refs/branch-heads/3325@{#51}
Cr-Branched-From: bc084a8-refs/heads/master@{#530369}

Author: stevenjb

chrome/browser/chromeos/net/shill_error.cc

@@ -38,8 +38,10 @@ base::string16 GetShillErrorString(const std::string& error,
     return l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_DHCP_FAILED);
   if (error == shill::kErrorConnectFailed)
     return l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_CONNECT_FAILED);
-  if (error == shill::kErrorBadPassphrase)
+  if (error == shill::kErrorBadPassphrase ||
+      error == shill::kErrorResultInvalidPassphrase) {
     return l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_BAD_PASSPHRASE);
+  }
   if (error == shill::kErrorBadWEPKey)
     return l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_BAD_WEPKEY);
   if (error == shill::kErrorActivationFailed) {

chrome/browser/ui/webui/chromeos/internet_config_dialog.cc

@@ -23,7 +23,8 @@ namespace chromeos {
 namespace {
 
 // Dialog height for configured networks that only require a passphrase.
-constexpr int kDialogHeightPasswordOnly = 320;
+// This height includes room for a 'connecting' or error message.
+constexpr int kDialogHeightPasswordOnly = 365;
 
 void AddInternetStrings(content::WebUIDataSource* html_source) {
   // Add default strings first.

chrome/browser/ui/webui/chromeos/network_element_localized_strings_provider.cc

@@ -296,26 +296,29 @@ void AddErrorLocalizedStrings(content::WebUIDataSource* html_source) {
     html_source->AddLocalizedString(entry.name, entry.id);
 
   // Include Shill errors.
-  const char* shill_errors[] = {shill::kErrorOutOfRange,
-                                shill::kErrorPinMissing,
-                                shill::kErrorDhcpFailed,
-                                shill::kErrorConnectFailed,
-                                shill::kErrorBadPassphrase,
-                                shill::kErrorBadWEPKey,
-                                shill::kErrorActivationFailed,
-                                shill::kErrorNeedEvdo,
-                                shill::kErrorNeedHomeNetwork,
-                                shill::kErrorOtaspFailed,
-                                shill::kErrorAaaFailed,
-                                shill::kErrorInternal,
-                                shill::kErrorDNSLookupFailed,
-                                shill::kErrorHTTPGetFailed,
-                                shill::kErrorIpsecPskAuthFailed,
-                                shill::kErrorIpsecCertAuthFailed,
-                                shill::kErrorEapAuthenticationFailed,
-                                shill::kErrorEapLocalTlsFailed,
-                                shill::kErrorEapRemoteTlsFailed,
-                                shill::kErrorPppAuthFailed};
+  const char* shill_errors[] = {
+      shill::kErrorOutOfRange,
+      shill::kErrorPinMissing,
+      shill::kErrorDhcpFailed,
+      shill::kErrorConnectFailed,
+      shill::kErrorBadPassphrase,
+      shill::kErrorBadWEPKey,
+      shill::kErrorActivationFailed,
+      shill::kErrorNeedEvdo,
+      shill::kErrorNeedHomeNetwork,
+      shill::kErrorOtaspFailed,
+      shill::kErrorAaaFailed,
+      shill::kErrorInternal,
+      shill::kErrorDNSLookupFailed,
+      shill::kErrorHTTPGetFailed,
+      shill::kErrorIpsecPskAuthFailed,
+      shill::kErrorIpsecCertAuthFailed,
+      shill::kErrorEapAuthenticationFailed,
+      shill::kErrorEapLocalTlsFailed,
+      shill::kErrorEapRemoteTlsFailed,
+      shill::kErrorPppAuthFailed,
+      shill::kErrorResultInvalidPassphrase,
+  };
   for (const auto* error : shill_errors) {
     html_source->AddString(
         error, base::UTF16ToUTF8(shill_error::GetShillErrorString(error, "")));

chromeos/network/network_configuration_handler.cc

@@ -73,6 +73,16 @@ void LogConfigProperties(const std::string& desc,
   }
 }
 
+// Returns recognized dbus error names or |default_error_name|.
+// TODO(stevenjb): Expand this list and update
+// network_element::AddErrorLocalizedStrings.
+std::string GetErrorName(const std::string& dbus_error_name,
+                         const std::string& default_error_name) {
+  if (dbus_error_name == shill::kErrorResultInvalidPassphrase)
+    return dbus_error_name;
+  return default_error_name;
+}
+
 }  // namespace
 
 // Helper class to request from Shill the profile entries associated with a
@@ -362,8 +372,8 @@ void NetworkConfigurationHandler::CreateShillConfiguration(
       base::Bind(&NetworkConfigurationHandler::ConfigurationCompleted,
                  weak_ptr_factory_.GetWeakPtr(), profile_path, source,
                  base::Passed(&properties_copy), callback),
-      base::Bind(&network_handler::ShillErrorCallbackFunction,
-                 "Config.CreateConfiguration Failed", "", error_callback));
+      base::Bind(&NetworkConfigurationHandler::ConfigurationFailed,
+                 weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 void NetworkConfigurationHandler::RemoveConfiguration(
@@ -480,6 +490,16 @@ void NetworkConfigurationHandler::Init(
   network_state_handler_->AddObserver(this, FROM_HERE);
 }
 
+void NetworkConfigurationHandler::ConfigurationFailed(
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& dbus_error_name,
+    const std::string& dbus_error_message) {
+  std::string error_name =
+      GetErrorName(dbus_error_name, "Config.CreateConfiguration Failed");
+  network_handler::ShillErrorCallbackFunction(
+      error_name, "", error_callback, dbus_error_name, dbus_error_message);
+}
+
 void NetworkConfigurationHandler::ConfigurationCompleted(
     const std::string& profile_path,
     NetworkConfigurationObserver::Source source,
@@ -596,9 +616,11 @@ void NetworkConfigurationHandler::SetPropertiesErrorCallback(
     const network_handler::ErrorCallback& error_callback,
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
-  network_handler::ShillErrorCallbackFunction(
-      "Config.SetProperties Failed", service_path, error_callback,
-      dbus_error_name, dbus_error_message);
+  std::string error_name =
+      GetErrorName(dbus_error_name, "Config.SetProperties Failed");
+  network_handler::ShillErrorCallbackFunction(error_name, service_path,
+                                              error_callback, dbus_error_name,
+                                              dbus_error_message);
   // Some properties may have changed so request an update regardless.
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }

chromeos/network/network_configuration_handler.h

@@ -161,6 +161,10 @@ class CHROMEOS_EXPORT NetworkConfigurationHandler
       const network_handler::ServiceResultCallback& callback,
       const dbus::ObjectPath& service_path);
 
+  void ConfigurationFailed(const network_handler::ErrorCallback& error_callback,
+                           const std::string& dbus_error_name,
+                           const std::string& dbus_error_message);
+
   // Called from ProfileEntryDeleter instances when they complete causing
   // this class to delete the instance.
   void ProfileEntryDeleterCompleted(const std::string& service_path,

ui/webui/resources/cr_components/chromeos/network/network_config.html

@@ -163,7 +163,8 @@
       </div>
     </template>
 
-    <template is="dom-if" if="[[propertiesSent_]]">
+    <template is="dom-if"
+        if="[[connectingIsVisible_(propertiesSent_, error_)]]">
       <div class="property-box">
         <div class="start layout horizontal center">
           <cr-network-icon is-list-item

ui/webui/resources/cr_components/chromeos/network/network_config.js

@@ -478,7 +478,7 @@ Polymer({
   setNetworkProperties_: function(properties) {
     this.propertiesReceived_ = true;
     this.networkProperties = properties;
-    this.error_ = properties.ErrorState || '';
+    this.setError_(properties.ErrorState);
 
     // Set the current shareNetwork_ value when porperties are received.
     this.setShareNetwork_();
@@ -884,10 +884,7 @@ Polymer({
 
     var requireCerts = (this.showEap_ && this.showEap_.UserCert) ||
         (this.showVpn_ && this.showVpn_.UserCert);
-    if (requireCerts && !this.userCerts_.length)
-      this.error_ = certError;
-    else
-      this.error_ = '';
+    this.setError_(requireCerts && !this.userCerts_.length ? certError : '');
   },
 
   /**
@@ -1014,6 +1011,14 @@ Polymer({
         (this.type == CrOnc.Type.WI_FI || this.type == CrOnc.Type.WI_MAX);
   },
 
+  /**
+   * @return {boolean}
+   * @private
+   */
+  connectingIsVisible_: function() {
+    return this.propertiesSent_ && !this.error_;
+  },
+
   /**
    * @return {boolean}
    * @private
@@ -1179,9 +1184,10 @@ Polymer({
 
   /** @private */
   setPropertiesCallback_: function() {
-    this.error_ = this.getRuntimeError_();
+    this.setError_(this.getRuntimeError_());
     if (this.error_) {
       console.error('setProperties error: ' + this.guid + ': ' + this.error_);
+      this.propertiesSent_ = false;
       return;
     }
     var connectState = this.networkProperties.ConnectionState;
@@ -1199,11 +1205,12 @@ Polymer({
    * @private
    */
   createNetworkCallback_: function(guid) {
-    this.error_ = this.getRuntimeError_();
+    this.setError_(this.getRuntimeError_());
     if (this.error_) {
       console.error(
           'createNetworkError, type: ' + this.networkProperties.Type + ': ' +
           'error: ' + this.error_);
+      this.propertiesSent_ = false;
       return;
     }
     this.startConnect_(guid);
@@ -1216,18 +1223,15 @@ Polymer({
   startConnect_: function(guid) {
     this.networkingPrivate.startConnect(guid, () => {
       var error = this.getRuntimeError_();
-      if (!error || error == 'connected' || error == 'connect-canceled') {
-        this.close_();  // Connect completed or canceled, close the dialog.
-        return;
-      }
-      if (error == 'connecting') {
-        // Keep the dialog open while connecting. TODO(stevenjb): Add a listener
-        // for the network properties and close the dialog if connected or show
-        // an error if not.
+      if (!error || error == 'connected' || error == 'connect-canceled' ||
+          error == 'connecting') {
+        // Connect is in progress, completed or canceled, close the dialog.
+        this.close_();
         return;
       }
-      this.error_ = error;
+      this.setError_(error);
       console.error('Error connecting to network: ' + error);
+      this.propertiesSent_ = false;
     });
   },
 
@@ -1269,6 +1273,18 @@ Polymer({
     };
   },
 
+  /**
+   * @param {string|undefined} error
+   * @private
+   */
+  setError_: function(error) {
+    if (!error) {
+      this.error_ = '';
+      return;
+    }
+    this.error_ = error;
+  },
+
   /**
    * @return {string}
    * @private