Network config: Explicitly show save or connect

Using the configured and saved state of a network to determine whether
to show 'Save' or 'Connect' was not always correct; instead let the UI
determine which button to show.

Also includes a minor improvement to the error messaging.

Bug: 814939
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ifb9a854826a3e4f45c17cb22ef03ed6b23c6f58c
Reviewed-on: https://chromium-review.googlesource.com/943922
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540624}

chrome/browser/resources/chromeos/internet_config_dialog/internet_config_dialog.html

@@ -52,7 +52,7 @@
               networking-private="[[networkingPrivate]]"
               global-policy="[[globalPolicy_]]"
               network-properties="{{networkProperties_}}"
-              enable-connect="{{enableConnect_}}" connect-on-save
+              enable-connect="{{enableConnect_}}"
               share-allow-enable="[[shareAllowEnable_]]"
               share-default="[[shareDefault_]]"
               error="{{error_}}"

chrome/browser/resources/chromeos/internet_config_dialog/internet_config_dialog.js

@@ -131,6 +131,6 @@ Polymer({
 
   /** @private */
   onConnectTap_: function() {
-    this.$.networkConfig.saveOrConnect();
+    this.$.networkConfig.connect();
   },
 });

chrome/browser/resources/settings/internet_page/internet_config.html

@@ -43,14 +43,14 @@
         <paper-button class="cancel-button" on-click="onCancelTap_">
           $i18n{cancel}
         </paper-button>
-        <template is="dom-if" if="[[isConfigured_(networkProperties_, guid)]]">
-          <paper-button class="action-button" on-click="onSaveOrConnectTap_"
+        <template is="dom-if" if="[[!showConnect]]">
+          <paper-button class="action-button" on-click="onSaveTap_"
               disabled="[[!enableSave_]]">
             $i18n{save}
           </paper-button>
         </template>
-        <template is="dom-if" if="[[!isConfigured_(networkProperties_, guid)]]">
-          <paper-button class="action-button" on-click="onSaveOrConnectTap_"
+        <template is="dom-if" if="[[showConnect]]">
+          <paper-button class="action-button" on-click="onConnectTap_"
               disabled="[[!enableConnect_]]">
             $i18n{networkButtonConnect}
           </paper-button>

chrome/browser/resources/settings/internet_page/internet_config.js

@@ -56,6 +56,12 @@ Polymer({
      */
     name: String,
 
+    /**
+     * Set to true to show the 'connect' button instead of 'save'.
+     * @private
+     */
+    showConnect: Boolean,
+
     /** @private */
     enableConnect_: Boolean,
 
@@ -134,22 +140,18 @@ Polymer({
     return this.i18n('networkErrorUnknown');
   },
 
-  /**
-   * @return {boolean}
-   * @private
-   */
-  isConfigured_: function() {
-    const source = this.networkProperties_.Source;
-    return !!this.guid && !!source && source != CrOnc.Source.NONE;
-  },
-
   /** @private */
   onCancelTap_: function() {
     this.close();
   },
 
   /** @private */
-  onSaveOrConnectTap_: function() {
-    this.$.networkConfig.saveOrConnect();
+  onSaveTap_: function() {
+    this.$.networkConfig.save();
+  },
+
+  /** @private */
+  onConnectTap_: function() {
+    this.$.networkConfig.connect();
   },
 });

chrome/browser/resources/settings/internet_page/internet_detail_page.js

@@ -630,7 +630,9 @@ Polymer({
       this.showTetherDialog_();
       return;
     }
-
+    // Clear the error state when 'Connect' is clicked to force a connect
+    // attempt instead of showing the configuration UI.
+    this.networkProperties.ErrorState = '';
     this.fire('network-connect', {networkProperties: this.networkProperties});
   },
 

chrome/browser/resources/settings/internet_page/internet_page.js

@@ -272,23 +272,27 @@ Polymer({
    */
   onShowConfig_: function(event) {
     const properties = event.detail;
+    let configAndConnect = !properties.GUID;  // New configuration
     this.showConfig_(
-        properties.Type, properties.GUID, CrOnc.getNetworkName(properties));
+        configAndConnect, properties.Type, properties.GUID,
+        CrOnc.getNetworkName(properties));
   },
 
   /**
+   * @param {boolean} configAndConnect
    * @param {string} type
    * @param {string=} guid
    * @param {string=} name
    * @private
    */
-  showConfig_: function(type, guid, name) {
+  showConfig_: function(configAndConnect, type, guid, name) {
     const configDialog =
         /** @type {!InternetConfigElement} */ (this.$.configDialog);
     configDialog.type =
         /** @type {chrome.networkingPrivate.NetworkType} */ (type);
     configDialog.guid = guid || '';
     configDialog.name = name || '';
+    configDialog.showConnect = configAndConnect;
     configDialog.open();
   },
 
@@ -378,15 +382,15 @@ Polymer({
   /** @private */
   onAddWiFiTap_: function() {
     if (loadTimeData.getBoolean('networkSettingsConfig'))
-      this.showConfig_(CrOnc.Type.WI_FI);
+      this.showConfig_(true /* configAndConnect */, CrOnc.Type.WI_FI);
     else
       chrome.send('addNetwork', [CrOnc.Type.WI_FI]);
   },
 
   /** @private */
   onAddVPNTap_: function() {
     if (loadTimeData.getBoolean('networkSettingsConfig'))
-      this.showConfig_(CrOnc.Type.VPN);
+      this.showConfig_(true /* configAndConnect */, CrOnc.Type.VPN);
     else
       chrome.send('addNetwork', [CrOnc.Type.VPN]);
   },
@@ -600,7 +604,8 @@ Polymer({
     }
 
     if (properties.Connectable === false || properties.ErrorState) {
-      this.showConfig_(properties.Type, properties.GUID, name);
+      this.showConfig_(
+          true /* configAndConnect */, properties.Type, properties.GUID, name);
       return;
     }
 
@@ -614,7 +619,9 @@ Polymer({
         console.error(
             'networkingPrivate.startConnect error: ' + message +
             ' For: ' + properties.GUID);
-        this.showConfig_(properties.Type, properties.GUID, name);
+        this.showConfig_(
+            true /* configAndConnect */, properties.Type, properties.GUID,
+            name);
       }
     });
   },

chromeos/network/network_connection_handler_impl.cc

@@ -85,23 +85,24 @@ bool IsCertificateConfigured(const client_cert::ConfigType cert_config_type,
   return false;
 }
 
-bool VPNRequiresCredentials(const std::string& service_path,
-                            const std::string& provider_type,
-                            const base::DictionaryValue& provider_properties) {
+std::string VPNCheckCredentials(
+    const std::string& service_path,
+    const std::string& provider_type,
+    const base::DictionaryValue& provider_properties) {
   if (provider_type == shill::kProviderOpenVpn) {
     std::string username;
     provider_properties.GetStringWithoutPathExpansion(
         shill::kOpenVPNUserProperty, &username);
     if (username.empty()) {
       NET_LOG(ERROR) << "OpenVPN: No username for: " << service_path;
-      return true;
+      return NetworkConnectionHandler::kErrorConfigurationRequired;
     }
     bool passphrase_required = false;
     provider_properties.GetBooleanWithoutPathExpansion(
         shill::kPassphraseRequiredProperty, &passphrase_required);
     if (passphrase_required) {
       NET_LOG(ERROR) << "OpenVPN: Passphrase Required for: " << service_path;
-      return true;
+      return NetworkConnectionHandler::kErrorPassphraseRequired;
     }
     NET_LOG_EVENT("OpenVPN Is Configured", service_path);
   } else {
@@ -110,17 +111,17 @@ bool VPNRequiresCredentials(const std::string& service_path,
         shill::kL2tpIpsecPskRequiredProperty, &passphrase_required);
     if (passphrase_required) {
       NET_LOG(ERROR) << "VPN: PSK Required for: " << service_path;
-      return true;
+      return NetworkConnectionHandler::kErrorConfigurationRequired;
     }
     provider_properties.GetBooleanWithoutPathExpansion(
         shill::kPassphraseRequiredProperty, &passphrase_required);
     if (passphrase_required) {
       NET_LOG(ERROR) << "VPN: Passphrase Required for: " << service_path;
-      return true;
+      return NetworkConnectionHandler::kErrorPassphraseRequired;
     }
     NET_LOG(EVENT) << "VPN Is Configured: " << service_path;
   }
-  return false;
+  return std::string();
 }
 
 std::string GetDefaultUserProfilePath(const NetworkState* network) {
@@ -544,9 +545,10 @@ void NetworkConnectionHandlerImpl::VerifyConfiguredAndConnect(
     // VPN may require a username, and/or passphrase to be set. (Check after
     // ensuring that any required certificates are configured).
     DCHECK(provider_properties);
-    if (VPNRequiresCredentials(service_path, vpn_provider_type,
-                               *provider_properties)) {
-      ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
+    std::string error = VPNCheckCredentials(service_path, vpn_provider_type,
+                                            *provider_properties);
+    if (!error.empty()) {
+      ErrorCallbackForPendingRequest(service_path, error);
       return;
     }
 

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

@@ -53,9 +53,6 @@ Polymer({
      */
     type: String,
 
-    /** Set by embedder if saveOrConnect should always connect. */
-    connectOnSave: Boolean,
-
     /** True if the user configuring the network can toggle the shared state. */
     shareAllowEnable: Boolean,
 
@@ -360,7 +357,19 @@ Polymer({
     this.setShareNetwork_();
   },
 
-  saveOrConnect: function() {
+  save: function() {
+    this.saveAndConnect_(false /* connect */);
+  },
+
+  connect: function() {
+    this.saveAndConnect_(true /* connect */);
+  },
+
+  /**
+   * @param {boolean} connect If true, connect after save.
+   * @private
+   */
+  saveAndConnect_: function(connect) {
     if (this.propertiesSent_)
       return;
     this.propertiesSent_ = true;
@@ -376,13 +385,14 @@ Polymer({
            this.globalPolicy.AllowOnlyPolicyNetworksToConnect)) {
         CrOnc.setTypeProperty(propertiesToSet, 'AutoConnect', false);
       }
-      // Create the configuration, then connect to it in the callback.
       this.networkingPrivate.createNetwork(
-          this.shareNetwork_, propertiesToSet,
-          this.createNetworkCallback_.bind(this));
+          this.shareNetwork_, propertiesToSet, (guid) => {
+            this.createNetworkCallback_(connect, guid);
+          });
     } else {
-      this.networkingPrivate.setProperties(
-          this.guid, propertiesToSet, this.setPropertiesCallback_.bind(this));
+      this.networkingPrivate.setProperties(this.guid, propertiesToSet, () => {
+        this.setPropertiesCallback_(connect);
+      });
     }
   },
 
@@ -400,7 +410,7 @@ Polymer({
   connectIfConfigured_: function() {
     if (!this.isConfigured_)
       return;
-    this.saveOrConnect();
+    this.connect();
   },
 
   /** @private */
@@ -1204,16 +1214,19 @@ Polymer({
     return (chrome.runtime.lastError && chrome.runtime.lastError.message) || '';
   },
 
-  /** @private */
-  setPropertiesCallback_: function() {
+  /**
+   * @param {boolean} connect If true, connect after save.
+   * @private
+   */
+  setPropertiesCallback_: function(connect) {
     this.setError_(this.getRuntimeError_());
     if (this.error) {
       console.error('setProperties error: ' + this.guid + ': ' + this.error);
       this.propertiesSent_ = false;
       return;
     }
     var connectState = this.networkProperties.ConnectionState;
-    if (this.connectOnSave &&
+    if (connect &&
         (!connectState ||
          connectState == CrOnc.ConnectionState.NOT_CONNECTED)) {
       this.startConnect_(this.guid);
@@ -1223,10 +1236,11 @@ Polymer({
   },
 
   /**
+   * @param {boolean} connect If true, connect after save.
    * @param {string} guid
    * @private
    */
-  createNetworkCallback_: function(guid) {
+  createNetworkCallback_: function(connect, guid) {
     this.setError_(this.getRuntimeError_());
     if (this.error) {
       console.error(
@@ -1235,7 +1249,8 @@ Polymer({
       this.propertiesSent_ = false;
       return;
     }
-    this.startConnect_(guid);
+    if (connect)
+      this.startConnect_(guid);
   },
 
   /**