CrOS Settings > Network: Improved UI support for custom APN

This addresses some inconsistencies with the 'Other' APN UI
encountered when switching between carrier networks within a
Cellular Service.

It also adds support for 'Other' entries with accessPointName
values matching an existing entry but with different username
or password values.

Additionally it fixes some instability in the UI while scanning
(when the device properties become unavailable). This fixes that
by postponing Cellular property updates until scanning completes.

Bug: 986921
Change-Id: If5a012e38e5af32ffe4832c553396b94361b86d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333536Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796489}

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

@@ -490,6 +490,11 @@ Polymer({
         // Update just the scanning state to avoid interrupting other parts of
         // the UI (e.g. custom IP addresses or nameservers).
         this.deviceState_.scanning = newDeviceState.scanning;
+        // Cellular properties are not updated while scanning (since they
+        // may be invalid), so request them on scan completion.
+        if (type === mojom.NetworkType.kCellular) {
+          shouldGetNetworkDetails = true;
+        }
       }
       if (shouldGetNetworkDetails) {
         this.getNetworkDetails_();
@@ -669,6 +674,14 @@ Polymer({
    */
   updateManagedProperties_(properties) {
     this.applyingChanges_ = true;
+    if (this.managedProperties_ &&
+        this.managedProperties_.type === mojom.NetworkType.kCellular &&
+        this.deviceState_ && this.deviceState_.scanning) {
+      // Cellular properties may be invalid while scanning, so keep the existing
+      // properties instead.
+      properties.typeProperties.cellular =
+          this.managedProperties_.typeProperties.cellular;
+    }
     this.managedProperties_ = properties;
     Polymer.RenderStatus.afterNextRender(
         this, () => this.applyingChanges_ = false);

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

@@ -16,16 +16,19 @@
     <div class="property-box">
       <div class="start">[[i18n('networkAccessPoint')]]</div>
       <select id="selectApn" class="md-select" on-change="onSelectApnChange_"
+          value="[[selectedApn_]]"
+          disabled="[[isDisabled_(selectedApn_)]]"
           aria-label="[[i18n('networkAccessPoint')]]">
         <template is="dom-repeat" items="[[apnSelectList_]]">
-          <option value="[[item.accessPointName]]"
-                  selected$="[[isApnItemSelected_(item, selectedApn_)]]">[[apnDesc_(item)]]</option>
+          <option value="[[item.name]]">
+            [[apnDesc_(item)]]
+          </option>
         </template>
       </select>
     </div>
 
-    <div class="property-box single-column indented"
-        hidden$="[[!isOtherSelected_(selectedApn_, managedProperties)]]">
+    <template is="dom-if" if="[[showOtherApn_(selectedApn_)]]">    
+      <div class="property-box single-column indented">
         <network-property-list-mojo on-property-change="onOtherApnChange_"
             fields="[[otherApnFields_]]" property-dict="[[otherApn_]]"
             edit-field-types="[[otherApnEditTypes_]]" prefix="cellular.apn.">
@@ -35,5 +38,6 @@
         </cr-button>
       </div>
     </template>
+  </template>
   <script src="network_apnlist.js"></script>
 </dom-module>

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

@@ -25,7 +25,12 @@ Polymer({
     },
 
     /**
-     * accessPointName value of the selected APN.
+     * The name property of the selected APN. If a name property is empty, the
+     * accessPointName property will be used. We use 'name' so that multiple
+     * APNs with the same accessPointName can be supported, so long as they have
+     * a unique 'name' property. This is necessary to allow custom  'other'
+     * entries (which are always named 'Other') that match an existing
+     * accessPointName but provide a different username/password.
      * @private
      */
     selectedApn_: {
@@ -49,10 +54,16 @@ Polymer({
      * The user settable properties for a new ('other') APN. The values for
      * accessPointName, username, and password will be set to the currently
      * active APN if it does not match an existing list entry.
-     * @private {chromeos.networkConfig.mojom.ApnProperties|undefined}
+     * @private {!chromeos.networkConfig.mojom.ApnProperties}
      */
     otherApn_: {
       type: Object,
+      value() {
+        return {
+          accessPointName: kDefaultAccessPointName,
+          name: kOtherAccessPointName,
+        };
+      }
     },
 
     /**
@@ -91,9 +102,8 @@ Polymer({
    */
   getApnFromManaged_(apn) {
     return {
+      // authentication and language are ignored in this UI.
       accessPointName: OncMojo.getActiveString(apn.accessPointName),
-      authentication: OncMojo.getActiveString(apn.authentication),
-      language: OncMojo.getActiveString(apn.language),
       localizedName: OncMojo.getActiveString(apn.localizedName),
       name: OncMojo.getActiveString(apn.name),
       password: OncMojo.getActiveString(apn.password),
@@ -111,80 +121,84 @@ Polymer({
     } else if (cellular.lastGoodApn && cellular.lastGoodApn.accessPointName) {
       activeApn = cellular.lastGoodApn;
     }
+    if (activeApn && !activeApn.accessPointName) {
+      activeApn = undefined;
+    }
     this.setApnSelectList_(activeApn);
   },
 
   /**
    * Sets the list of selectable APNs for the UI. Appends an 'Other' entry
    * (see comments for |otherApn_| above).
-   * @param {chromeos.networkConfig.mojom.ApnProperties|undefined} activeApn The
-   *     currently active APN properties.
+   * @param {chromeos.networkConfig.mojom.ApnProperties|undefined} activeApn
    * @private
    */
   setApnSelectList_(activeApn) {
-    // Copy the list of APNs from this.managedProperties.
-    const apnList = this.getApnList_().slice();
-
-    // Test whether |activeApn| is in the current APN list in managedProperties.
-    const activeApnInList = activeApn && apnList.some(function(a) {
-      return a.accessPointName === activeApn.accessPointName;
-    });
-
-    // If |activeApn| is specified and not in the list, use the active
-    // properties for 'other'. Otherwise use any existing 'other' properties.
-    const otherApnProperties =
-        (activeApn && !activeApnInList) ? activeApn : this.otherApn_;
-    const otherApn = this.createApnObject_(otherApnProperties);
-
-    // Always use 'Other' for the name of custom APN entries (the name does
-    // not get saved).
-    otherApn.name = kOtherAccessPointName;
-
-    // If no 'active' or 'other' AccessPointName was provided, use the default.
-    otherApn.accessPointName =
-        otherApn.accessPointName || kDefaultAccessPointName;
-
-    // Save the 'other' properties.
-    this.otherApn_ = otherApn;
+    assert(!activeApn || activeApn.accessPointName);
+    // The generated APN list ensures nonempty accessPointName and name
+    // properties.
+    const apnList = this.generateApnList_();
+    if (apnList === undefined) {
+      // No APNList property indicates that the network is not in a
+      // connectable state. Disable the UI.
+      this.apnSelectList_ = [];
+      this.set('selectedApn_', '');
+      return;
+    }
+    // Get the list entry for activeApn if it exists. It will have 'name' set.
+    let activeApnInList;
+    if (activeApn) {
+      activeApnInList = apnList.find(a => a.name === activeApn.name);
+    }
 
-    // Append 'other' to the end of the list of APNs.
-    apnList.push(otherApn);
+    // If the active APN is not in the list, copy it to otherApn_.
+    if (!activeApnInList && activeApn && activeApn.accessPointName) {
+      this.otherApn_ = {
+        accessPointName: activeApn.accessPointName,
+        name: kOtherAccessPointName,
+        username: activeApn.username,
+        password: activeApn.password,
+      };
+    }
+    apnList.push(this.otherApn_);
 
     this.apnSelectList_ = apnList;
-    this.selectedApn_ =
-        (activeApn && activeApn.accessPointName) || otherApn.accessPointName;
-  },
+    const selectedApn =
+        activeApnInList ? activeApnInList.name : kOtherAccessPointName;
+    assert(selectedApn);
+    this.set('selectedApn_', selectedApn);
 
-  /**
-   * @param {chromeos.networkConfig.mojom.ApnProperties=}
-   *     apnProperties
-   * @return {!chromeos.networkConfig.mojom.ApnProperties} A new APN object with
-   *     properties from |apnProperties| if provided.
-   * @private
-   */
-  createApnObject_(apnProperties) {
-    const newApn = {accessPointName: ''};
-    if (apnProperties) {
-      Object.assign(newApn, apnProperties);
-    }
-    return newApn;
+    // Wait for the dom-repeat to populate the <option> entries then explicitly
+    // set the selected value.
+    this.async(function() {
+      this.$.selectApn.value = this.selectedApn_;
+    });
   },
 
   /**
-   * @return {!Array<!chromeos.networkConfig.mojom.ApnProperties>} The list of
-   *     APN properties in |managedProperties| or an empty list if the property
-   *     is not set.
+   * Returns a modified copy of the APN properties or undefined if the
+   * property is not set. All entries in the returned copy will have nonempty
+   * name and accessPointName properties.
+   * @return {!Array<!chromeos.networkConfig.mojom.ApnProperties>|undefined}
    * @private
    */
-  getApnList_() {
+  generateApnList_() {
     if (!this.managedProperties) {
-      return [];
+      return undefined;
     }
     const apnList = this.managedProperties.typeProperties.cellular.apnList;
     if (!apnList) {
-      return [];
+      return undefined;
     }
-    return apnList.activeValue;
+    return apnList.activeValue.filter(apn => !!apn.accessPointName).map(apn => {
+      return {
+        accessPointName: apn.accessPointName,
+        localizedName: apn.localizedName,
+        name: apn.name || apn.accessPointName,
+        username: apn.username,
+        password: apn.password,
+      };
+    });
   },
 
   /**
@@ -194,16 +208,18 @@ Polymer({
    */
   onSelectApnChange_(event) {
     const target = /** @type {!HTMLSelectElement} */ (event.target);
-    const accessPointName = target.value;
-    // When selecting 'Other', don't set a change event unless a valid
+    const name = target.value;
+    // When selecting 'Other', don't send a change event unless a valid
     // non-default value has been set for Other.
-    if (this.isOtherSelected_(accessPointName) &&
-        (!this.otherApn_ || !this.otherApn_.accessPointName ||
+    if (name === kOtherAccessPointName &&
+        (!this.otherApn_.accessPointName ||
          this.otherApn_.accessPointName === kDefaultAccessPointName)) {
-      this.selectedApn_ = accessPointName;
+      this.selectedApn_ = name;
       return;
     }
-    this.sendApnChange_(accessPointName);
+    // The change will generate an update which will update selectedApn_ and
+    // refresh the UI.
+    this.sendApnChange_(name);
   },
 
   /**
@@ -232,57 +248,57 @@ Polymer({
 
   /**
    * Send the apn-change event.
-   * @param {string} accessPointName
+   * @param {string} name The APN name property.
    * @private
    */
-  sendApnChange_(accessPointName) {
-    const apnList = this.getApnList_();
-    let apn = this.findApnInList_(apnList, accessPointName);
+  sendApnChange_(name) {
+    let apn;
+    if (name === kOtherAccessPointName) {
+      if (!this.otherApn_.accessPointName ||
+          this.otherApn_.accessPointName === kDefaultAccessPointName) {
+        // No valid APN set, do nothing.
+        return;
+      }
+      apn = {
+        accessPointName: this.otherApn_.accessPointName,
+        username: this.otherApn_.username,
+        password: this.otherApn_.password,
+      };
+    } else {
+      apn = this.apnSelectList_.find(a => a.name === name);
       if (apn === undefined) {
-      apn = this.createApnObject_();
-      if (this.otherApn_) {
-        apn.accessPointName = this.otherApn_.accessPointName;
-        apn.username = this.otherApn_.username;
-        apn.password = this.otherApn_.password;
+        // Potential edge case if an update is received before this is invoked.
+        console.error('Selected APN not in list');
+        return;
       }
     }
     this.fire('apn-change', apn);
   },
 
   /**
-   * @param {string} accessPointName
-   * @return {boolean} True if the 'other' APN is currently selected.
+   * @return {boolean}
    * @private
    */
-  isOtherSelected_(accessPointName) {
-    if (!this.managedProperties) {
-      return false;
-    }
-    const apnList = this.getApnList_();
-    const apn = this.findApnInList_(apnList, accessPointName);
-    return apn === undefined;
+  isDisabled_() {
+    return this.selectedApn_ === '';
   },
 
   /**
-   * @param {!chromeos.networkConfig.mojom.ApnProperties} apn
-   * @return {string} The most descriptive name for the access point.
+   * @return {boolean}
    * @private
    */
-  apnDesc_(apn) {
-    return apn.localizedName || apn.name || apn.accessPointName;
+  showOtherApn_() {
+    return this.selectedApn_ === kOtherAccessPointName;
   },
 
   /**
-   * @param {!Array<!chromeos.networkConfig.mojom.ApnProperties>} apnList
-   * @param {string} accessPointName
-   * @return {chromeos.networkConfig.mojom.ApnProperties|undefined} The entry in
-   *     |apnList| matching |accessPointName| if it exists, or undefined.
+   * @param {!chromeos.networkConfig.mojom.ApnProperties} apn
+   * @return {string} The most descriptive name for the access point.
    * @private
    */
-  findApnInList_(apnList, accessPointName) {
-    return apnList.find(function(a) {
-      return a.accessPointName === accessPointName;
-    });
+  apnDesc_(apn) {
+    assert(apn.name);
+    return apn.localizedName || apn.name;
   },
 
   /**