Use Managed properties for Preferred and Provider.

chrome/browser/resources/options/chromeos/internet_detail.js

Index: chrome/browser/resources/options/chromeos/internet_detail.js
diff --git a/chrome/browser/resources/options/chromeos/internet_detail.js b/chrome/browser/resources/options/chromeos/internet_detail.js
index 7447b526586eb45acfe14dbc28d5f8b6c3a34ae3..331d48905b10fd62df1dccfb2ac50e814ffc030b 100644
--- a/chrome/browser/resources/options/chromeos/internet_detail.js
+++ b/chrome/browser/resources/options/chromeos/internet_detail.js
@@ -16,22 +16,39 @@ cr.define('options.internet', function() {
   /** @const */ var IPAddressField = options.internet.IPAddressField;
 
   /**
-   * Helper function to get the "Active" value of a property from a dictionary
-   * that includes ONC managed properties, e.g. getActiveValue(data, 'Name').
-   * We use (data, key) instead of getActiveValue(data[key]) so that we can
-   * (possibly) add ONC key validation once all properties use ONC.
+   * Gets the value of a property from a dictionary |data| that includes ONC
+   * managed properties, e.g. getManagedValue(data, 'Name'). See notes for
+   * getManagedProperty.
    * @param {object} data The properties dictionary.
    * @param {string} key The property key.
+   * @param {string} type (Optional) The type of property to get:
+   *                      'active' (default)  - gets the active value
+   *                      'translated' - gets the traslated or active value
+   *                      'recommended' - gets the recommended or active value

[pneubeck (no reviews), 2014/08/19 16:24:02]

why should recommended fallback to the active value?
The caller could do this instead by calling
  value = getManagedValue(..., recommended)
  if (!recommended)
    value = getManagedValue(..., active)

to make this corner case (?) more explicit.

[stevenjb, 2014/08/19 18:41:47]

That is actually what it does, just the comment is wrong. Fixed.

    * @return {*} the property value or undefined.
    */
-  function getActiveValue(data, key) {
-    if (!(key in data))
+  function getManagedValue(data, key, type) {
+    var property = getManagedProperty(data, key);
+    if (!property)

[pneubeck (no reviews), 2014/08/19 16:24:02]

be careful about the implicit conversion into boolean here!
please replace by data === undefined
or check 'key in data'.

[stevenjb, 2014/08/19 18:41:47]

Actually, this check isn't necessary since typeof undefined == 'undefined' (i.e.
!= 'object). Removed.

       return undefined;
-    var property = data[key];
     if (typeof property != 'object')
       return property;
+    if (type == 'recommended')
+      return getEffectiveValue(property);

[pneubeck (no reviews), 2014/08/19 16:24:02]

this doesn't look like it would return the recommended value if the user has
modified the property.
Should instead be:

function getRecommendedValue(propertyDict) {  // a'la getEffectValue
  if (property['UserEditable'])
    return property['UserPolicy']
  if (property['DeviceEditable'])
    return property['DevicePolicy']
  // No value recommended by policy.
  return undefined
}

[stevenjb, 2014/08/19 18:41:47]

Bah, ugh, right, fixed.

+    if (type == 'translate' && 'Translated' in property)
+      return property['Translated'];
     if ('Active' in property)

[pneubeck (no reviews), 2014/08/19 16:24:02]

nit: add a comment before this line saying "Otherwise get the 'active' value".

[stevenjb, 2014/08/19 18:41:47]

Done.

       return property['Active'];
+    // If no Active value is defined, return the effective (recommended) value.
+    return getEffectiveValue(property);
+  }
+
+  /**
+   * Get the effective value from a Managed property ONC dictionary.
+   * @param {object} property The managed property ONC dictionary.
+   * @return {*} the effective value or undefined.
+   */
+  function getEffectiveValue(property) {
     if ('Effective' in property) {
       var effective = property.Effective;
       if (effective in property)
@@ -41,15 +58,24 @@ cr.define('options.internet', function() {
   }
 
   /**
-   * Helper function for nested ONC properties, e.g. data[WiFi][Strength].
-   * @param {object|string} property The property which must ether be
-   *   a dictionary object or not be present.
+   * Gets either a managed property dictionary or an unmanaged value from
+   * dictionary |data| that includes ONC managed properties. This supports
+   * nested dictionaries, e.g. getManagedProperty(data, 'VPN.Type').
+   * @param {object} data The properties dictionary.
+   * @param {string} key The property key.
    * @return {*} the property value or undefined.

[pneubeck (no reviews), 2014/08/19 16:24:02]

'property value or dictionary if it exists. Otherwise undefined.'

[stevenjb, 2014/08/19 18:41:47]

Done.

    */
-  function getActiveDictionaryValue(data, dict, key) {
-    if (!(dict in data))
-      return undefined;
-    return getActiveValue(data[dict], key);
+  function getManagedProperty(data, key) {
+    var index = key.indexOf('.');
+    while (index >= 0) {
+      var dictKey = key.substr(0, index);

[pneubeck (no reviews), 2014/08/19 16:24:02]

optional nit:
 dictKey -> keyComponent?

[stevenjb, 2014/08/19 18:41:47]

Sure.

+      data = data[dictKey];
+      if (!data)

[pneubeck (no reviews), 2014/08/19 16:24:02]

again, be careful about implicit conversion into boolean.

[stevenjb, 2014/08/19 18:41:47]

Fixed.

+        return undefined;
+      key = key.substr(index + 1);
+      index = key.indexOf('.');

[pneubeck (no reviews), 2014/08/19 16:24:02]

optional nit: could be merged with line 69 by
  while(true) {
    index = ...
    if (index < 0)
      break;
    ...
  }

[stevenjb, 2014/08/19 18:41:47]

Yeah, I'm not a big fan of that pattern, but duplicated code also sucks.
Changed.

+    }
+    return data[key];
   }
 
   /**
@@ -135,7 +161,7 @@ cr.define('options.internet', function() {
   function getNetworkName(data) {
     if (data.type == 'Ethernet')
       return loadTimeData.getString('ethernetName');
-    return getActiveValue(data, 'Name');
+    return getManagedValue(data, 'Name');
   }
 
   /////////////////////////////////////////////////////////////////////////////
@@ -908,7 +934,7 @@ cr.define('options.internet', function() {
       return;
     }
 
-    var connectState = getActiveValue(data, 'ConnectionState');
+    var connectState = getManagedValue(data, 'ConnectionState');
     if (connectState == 'NotConnected') {
       $('details-internet-login').hidden = false;
       // Connecting to an unconfigured network might trigger certificate
@@ -921,7 +947,7 @@ cr.define('options.internet', function() {
       $('details-internet-disconnect').hidden = false;
     }
 
-    var connectable = getActiveValue(data, 'Connectable');
+    var connectable = getManagedValue(data, 'Connectable');
     if (connectState != 'Connected' &&
         (!connectable || this.hasSecurity ||
         (data.type == 'Wimax' || data.type == 'VPN'))) {
@@ -946,7 +972,7 @@ cr.define('options.internet', function() {
     // Update our cached data object.
     updateDataObject(data, update);
 
-    var connectionState = getActiveValue(data, 'ConnectionState');
+    var connectionState = getManagedValue(data, 'ConnectionState');
     var connectionStateString = networkOncStateString(connectionState);
     detailsPage.deviceConnected = data.deviceConnected;
     detailsPage.connected = connectionState == 'Connected';
@@ -982,11 +1008,11 @@ cr.define('options.internet', function() {
   DetailsInternetPage.showDetailedInfo = function(data) {
     var detailsPage = DetailsInternetPage.getInstance();
 
-    data.type = getActiveValue(data, 'Type');  // Get Active Type value.
+    data.type = getManagedValue(data, 'Type');  // Get Active Type value.
 
     // Populate header
     $('network-details-title').textContent = getNetworkName(data);
-    var connectionState = getActiveValue(data, 'ConnectionState');
+    var connectionState = getManagedValue(data, 'ConnectionState');
     var connectionStateString = networkOncStateString(connectionState);
     detailsPage.connected = connectionState == 'Connected';
     $('network-details-subtitle-status').textContent = connectionStateString;
@@ -1130,7 +1156,7 @@ cr.define('options.internet', function() {
 
     DetailsInternetPage.updateNameServerDisplay(data.nameServerType);
 
-    var macAddress = getActiveValue(data, 'MacAddress');
+    var macAddress = getManagedValue(data, 'MacAddress');
     if (macAddress) {
       $('hardware-address').textContent = macAddress;
       $('hardware-address-row').style.display = 'table-row';
@@ -1153,8 +1179,7 @@ cr.define('options.internet', function() {
     // Signal strength as percentage (for WiFi and Wimax).
     var signalStrength;
     if (data.type == 'WiFi' || data.type == 'Wimax') {
-      signalStrength =
-          getActiveDictionaryValue(data, data.type, 'SignalStrength');
+      signalStrength = getManagedValue(data, data.type + '.SignalStrength');
     }
     if (!signalStrength)
       signalStrength = 0;
@@ -1163,21 +1188,20 @@ cr.define('options.internet', function() {
 
     detailsPage.type = data.type;
     if (data.type == 'WiFi') {
-      assert(data.WiFi, 'WiFi network has no WiFi object' + networkName);
+      assert('WiFi' in data, 'WiFi network has no WiFi object' + networkName);

[pneubeck (no reviews), 2014/08/19 16:24:02]

good catch!

[stevenjb, 2014/08/19 18:41:47]

Acknowledged.

       OptionsPage.showTab($('wifi-network-nav-tab'));
       detailsPage.gsm = false;
       detailsPage.shared = data.shared;
       $('wifi-connection-state').textContent = connectionStateString;
-      var ssid = getActiveDictionaryValue(data, 'WiFi', 'SSID');
+      var ssid = getManagedValue(data, 'WiFi.SSID');
       $('wifi-ssid').textContent = ssid ? ssid : networkName;
-      setOrHideParent('wifi-bssid',
-                      getActiveDictionaryValue(data, 'WiFi', 'BSSID'));
-      var security = getActiveDictionaryValue(data, 'WiFi', 'Security');
+      setOrHideParent('wifi-bssid', getManagedValue(data, 'WiFi.BSSID'));
+      var security = getManagedValue(data, 'WiFi.Security');
       if (security == 'None')
         security = undefined;
       setOrHideParent('wifi-security', security);
       // Frequency is in MHz.
-      var frequency = getActiveDictionaryValue(data, 'WiFi', 'Frequency');
+      var frequency = getManagedValue(data, 'WiFi.Frequency');
       if (!frequency)
         frequency = 0;
       var frequencyFormat = loadTimeData.getString('inetFrequencyFormat');
@@ -1185,31 +1209,33 @@ cr.define('options.internet', function() {
       $('wifi-frequency').textContent = frequencyFormat;
       $('wifi-signal-strength').textContent = strengthString;
       setOrHideParent('wifi-hardware-address',
-                      getActiveValue(data, 'MacAddress'));
+                      getManagedValue(data, 'MacAddress'));
       detailsPage.showPreferred = data.remembered;
-      $('prefer-network-wifi').checked = data.preferred.value;
+      var priority = getManagedValue(data, 'Priority');
+      $('prefer-network-wifi').checked = priority > 0;
       $('prefer-network-wifi').disabled = !data.remembered;
       $('auto-connect-network-wifi').checked =
-          getActiveValue(data, 'AutoConnect');
+          getManagedValue(data, 'AutoConnect');
       $('auto-connect-network-wifi').disabled = !data.remembered;
       detailsPage.hasSecurity = security != undefined;
     } else if (data.type == 'Wimax') {
-      assert(data.Wimax, 'Wimax network has no Wimax object' + networkName);
+      assert('Wimax' in data,
+             'Wimax network has no Wimax object' + networkName);
       OptionsPage.showTab($('wimax-network-nav-tab'));
       detailsPage.gsm = false;
       detailsPage.shared = data.shared;
       detailsPage.showPreferred = data.remembered;
       $('wimax-connection-state').textContent = connectionStateString;
       $('auto-connect-network-wimax').checked =
-          getActiveValue(data, 'AutoConnect');
+          getManagedValue(data, 'AutoConnect');
       $('auto-connect-network-wimax').disabled = !data.remembered;
       var identity;
       if (data.Wimax.EAP)
-        identity = getActiveValue(data.Wimax.EAP, 'Identity');
+        identity = getManagedValue(data.Wimax.EAP, 'Identity');
       setOrHideParent('wimax-eap-identity', identity);
       $('wimax-signal-strength').textContent = strengthString;
     } else if (data.type == 'Cellular') {
-      assert(data.Cellular,
+      assert('Cellular' in data,
              'Cellular network has no Cellular object' + networkName);
       OptionsPage.showTab($('cellular-conn-nav-tab'));
       if (data.showCarrierSelect && data.currentCarrierIndex != -1) {
@@ -1227,27 +1253,29 @@ cr.define('options.internet', function() {
       }
 
       $('network-technology').textContent =
-          getActiveDictionaryValue(data, 'Cellular', 'NetworkTechnology');
+          getManagedValue(data, 'Cellular.NetworkTechnology');

[pneubeck (no reviews), 2014/08/19 16:24:02]

hm. You could have done a
  var cellular = getManagedProperty(data, 'Cellular')
  ...
    ...technology... = getManagedValue(data, 'NetworkTechnology')

to save repetitions of the 'Cellular' constant prefix.
(maybe similar for other network types)

[stevenjb, 2014/08/19 18:41:47]

Yeah, I thought of that, but wanted to limit the changes here. Also, eventually
everything will use getManagedValue, so this will all be able to be a lot more
data driven, so I think I will leave it for now.

       $('activation-state').textContent = data.activationState;
       $('roaming-state').textContent = data.roamingState;
       $('restricted-pool').textContent = data.restrictedPool;
       $('error-state').textContent = data.errorMessage;
       $('manufacturer').textContent =
-          getActiveDictionaryValue(data, 'Cellular', 'Manufacturer');
-      $('model-id').textContent =
-          getActiveDictionaryValue(data, 'Cellular', 'ModelID');
+          getManagedValue(data, 'Cellular.Manufacturer');
+      $('model-id').textContent = getManagedValue(data, 'Cellular.ModelID');
       $('firmware-revision').textContent =
-          getActiveDictionaryValue(data, 'Cellular', 'FirmwareRevision');
+          getManagedValue(data, 'Cellular.FirmwareRevision');
       $('hardware-revision').textContent =
-          getActiveDictionaryValue(data, 'Cellular', 'HardwareRevision');
-      $('mdn').textContent = getActiveDictionaryValue(data, 'Cellular', 'MDN');
+          getManagedValue(data, 'Cellular.HardwareRevision');
+      $('mdn').textContent = getManagedValue(data, 'Cellular.MDN');
 
       // Show ServingOperator properties only if available.
-      if (data.Cellular.ServingOperator) {
-        $('operator-name').textContent =
-            getActiveValue(data.Cellular.ServingOperator, 'Name');
-        $('operator-code').textContent =
-            getActiveValue(data.Cellular.ServingOperator, 'Code');
+      var servingOperatorName =
+          getManagedValue(data, 'Cellular.ServingOperator.Name');
+      var servingOperatorCode =
+          getManagedValue(data, 'Cellular.ServingOperator.Code');
+      if (servingOperatorName != undefined &&
+          servingOperatorCode != undefined) {
+        $('operator-name').textContent = servingOperatorName;
+        $('operator-code').textContent = servingOperatorCode;
       } else {
         $('operator-name').parentElement.hidden = true;
         $('operator-code').parentElement.hidden = true;
@@ -1258,23 +1286,18 @@ cr.define('options.internet', function() {
       updateHidden('#details-internet-page .cdma-only', false);
 
       // Show IMEI/ESN/MEID/MIN/PRL only if they are available.
-      setOrHideParent('esn', getActiveDictionaryValue(data, 'Cellular', 'ESN'));
-      setOrHideParent(
-          'imei', getActiveDictionaryValue(data, 'Cellular', 'IMEI'));
-      setOrHideParent(
-          'meid', getActiveDictionaryValue(data, 'Cellular', 'MEID'));
-      setOrHideParent('min', getActiveDictionaryValue(data, 'Cellular', 'MIN'));
-      setOrHideParent(
-          'prl-version',
-          getActiveDictionaryValue(data, 'Cellular', 'PRLVersion'));
-
-      var family = getActiveDictionaryValue(data, 'Cellular', 'GSM');
+      setOrHideParent('esn', getManagedValue(data, 'Cellular.ESN'));
+      setOrHideParent('imei', getManagedValue(data, 'Cellular.IMEI'));
+      setOrHideParent('meid', getManagedValue(data, 'Cellular.MEID'));
+      setOrHideParent('min', getManagedValue(data, 'Cellular.MIN'));
+      setOrHideParent('prl-version',
+                      getManagedValue(data, 'Cellular.PRLVersion'));
+
+      var family = getManagedValue(data, 'Cellular.GSM');
       detailsPage.gsm = family == 'GSM';
       if (detailsPage.gsm) {
-        $('iccid').textContent =
-            getActiveDictionaryValue(data, 'Cellular', 'ICCID');
-        $('imsi').textContent =
-            getActiveDictionaryValue(data, 'Cellular', 'IMSI');
+        $('iccid').textContent = getManagedValue(data, 'Cellular.ICCID');
+        $('imsi').textContent = getManagedValue(data, 'Cellular.IMSI');
 
         var apnSelector = $('select-apn');
         // Clear APN lists, keep only last element that "other".
@@ -1324,7 +1347,7 @@ cr.define('options.internet', function() {
         $('change-pin').hidden = !lockEnabled;
       }
       $('auto-connect-network-cellular').checked =
-          getActiveValue(data, 'AutoConnect');
+          getManagedValue(data, 'AutoConnect');
       $('auto-connect-network-cellular').disabled = false;
 
       $('buyplan-details').hidden = !data.showBuyButton;
@@ -1337,16 +1360,21 @@ cr.define('options.internet', function() {
       OptionsPage.showTab($('vpn-nav-tab'));
       detailsPage.gsm = false;
       $('inet-service-name').textContent = networkName;
-      $('inet-provider-type').textContent = data.providerType;
-      $('inet-username').textContent = data.username;
+      $('inet-provider-type').textContent =
+          getManagedValue(data, 'VPN.Type', 'translated');
+      var providerType = getManagedValue(data, 'VPN.Type', 'active');
+      var providerKey = 'VPN.' + providerType;
+      $('inet-username').textContent =
+          getManagedValue(data, providerKey + '.Username');
       var inetServerHostname = $('inet-server-hostname');
-      inetServerHostname.value = data.serverHostname.value;
+      inetServerHostname.value = getManagedValue(data, 'VPN.Host');
       inetServerHostname.resetHandler = function() {
         PageManager.hideBubble();
-        inetServerHostname.value = data.serverHostname.recommendedValue;
+        inetServerHostname.value =
+            getManagedValue(data, 'VPN.Host', 'recommended');
       };
       $('auto-connect-network-vpn').checked =
-          getActiveValue(data, 'AutoConnect');
+          getManagedValue(data, 'AutoConnect');
       $('auto-connect-network-vpn').disabled = false;
     } else {
       OptionsPage.showTab($('internet-nav-tab'));
@@ -1356,16 +1384,20 @@ cr.define('options.internet', function() {
     var indicators = cr.doc.querySelectorAll(
         '#details-internet-page .controlled-setting-indicator');
     for (var i = 0; i < indicators.length; i++) {
-      var attributeName =
-          indicators[i].hasAttribute('managed') ? 'managed' : 'data';
+      var managed = indicators[i].hasAttribute('managed');
+      var attributeName = managed ? 'managed' : 'data';
       var propName = indicators[i].getAttribute(attributeName);
-      if (!propName || !data[propName])
+      if (!propName)
+        continue;
+      var propValue =
+          managed ? getManagedProperty(data, propName) : data[propName];
+      if (propValue == undefined)

[pneubeck (no reviews), 2014/08/19 16:24:02]

even better catch. boolean conversion seems really evil ^^

[stevenjb, 2014/08/19 18:41:47]

I still kind of hate JS, but I'm slowly getting used to it...

         continue;
       var event;
-      if (attributeName == 'managed')
-        event = detailsPage.createManagedEvent_(propName, data[propName]);
+      if (managed)
+        event = detailsPage.createManagedEvent_(propName, propValue);
       else
-        event = detailsPage.createControlledEvent_(propName, data[propName]);
+        event = detailsPage.createControlledEvent_(propName, propValue);
       indicators[i].handlePrefChange(event);
       var forElement = $(indicators[i].getAttribute('for'));
       if (forElement) {