Use setProperties for Cellular APN

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // require: onc_data.js
 
 // NOTE(stevenjb): This code is in the process of being converted to be
 // compatible with the networkingPrivate extension API:
 // * The network property dictionaries are being converted to use ONC values.
 // * chrome.send calls will be replaced with an API object that simulates the
@@ skipping 50 matching lines @@
     DetailsInternetPage.getInstance().updateControls();
   }
 
   /**
    * Simple helper method for converting a field to a string. It is used to
    * easily assign an empty string from fields that may be unknown or undefined.
    * @param {Object} value that should be converted to a string.
    * @return {string} the result.
    */
   function stringFromValue(value) {
     return value ? String(value) : '';

[pneubeck (no reviews), 2015/01/23 14:53:29]

btw. this function seems weird to me.
this should probably be something like
  if (value === undefined || value === null)
    return '';
  return String(value);
?

otherwise things like
  stringFromValue(0) -> ''
happen

[stevenjb, 2015/01/23 20:25:33]

Acknowledged.

   }
 
   /**
    * @param {string} action An action to send to coreOptionsUserMetricsAction.
    */
   function sendChromeMetricsAction(action) {
     chrome.send('coreOptionsUserMetricsAction', [action]);
   }
 
   /**
@@ skipping 90 matching lines @@
 
   /////////////////////////////////////////////////////////////////////////////
   // DetailsInternetPage class:
 
   /**
    * Encapsulated handling of ChromeOS internet details overlay page.
    * @constructor
    * @extends {cr.ui.pageManager.Page}
    */
   function DetailsInternetPage() {
-    // Cached Apn properties
+    // If non-negative, indicates a custom entry in select-apn.
     this.userApnIndex_ = -1;
+
+    // The custom APN properties associated with entry |userApnIndex_|.
+    this.userApn_ = {};
+
+    // The currently selected APN entry (which may or may not == userApnIndex_).
     this.selectedApnIndex_ = -1;
-    this.userApn_ = {};
+
     // We show the Proxy configuration tab for remembered networks and when
     // configuring a proxy from the login screen.
     this.showProxy_ = false;
 
     Page.call(this, 'detailsInternetPage', '', 'details-internet-page');
   }
 
   cr.addSingletonGetter(DetailsInternetPage);
 
   DetailsInternetPage.prototype = {
@@ skipping 581 matching lines @@
         typeLabel.textContent = loadTimeData.getString(typeKey);
         typeLabel.hidden = false;
         typeSeparator.hidden = false;
       } else {
         typeLabel.hidden = true;
         typeSeparator.hidden = true;
       }
     },
 
     /**
-     * Helper method called from initializeDetailsPage to initialize the Apn
-     * list.
+     * Helper method called from initializeApnList to populate the Apn list.
+     * @param {Array} apnList List of available APNs.
      * @private
      */
-    initializeApnList_: function() {
+    populateApnList_: function(apnList) {

[pneubeck (no reviews), 2015/01/23 14:53:29]

nit:

the semantics of this functions would be a bit simpler if it would only populate
the entries from |apnList|.
The 'user' entry could be added (in the same way as 'default') in
initializeApnList_.

[stevenjb, 2015/01/23 20:25:32]

If (when) I re-write this I would organize it very differently. I'm trying to
slightly improve readability (mostly for my own sake) without completely
re-writing it here.

       var onc = this.onc_;
-
       var apnSelector = $('select-apn');
-      // Clear APN lists, keep only last element that "other".
-      while (apnSelector.length != 1) {
-        apnSelector.remove(0);
-      }
       var otherOption = apnSelector[0];

[pneubeck (no reviews), 2015/01/23 14:53:28]

this only works if 'other' is the only option, thus this should at least be
preceded by a assert(apnSelector.size() == 1)
alternatively, don't rely on this fact and just say that you insert at the
beginning of the list and rename 'otherOption' to 'oldFirstOption' or so.

[stevenjb, 2015/01/23 20:25:33]

Acknowledged. I'll add an assert.

       var activeApn = onc.getActiveValue('Cellular.APN.AccessPointName');
       var activeUsername = onc.getActiveValue('Cellular.APN.Username');
       var activePassword = onc.getActiveValue('Cellular.APN.Password');
       var lastGoodApn =
           onc.getActiveValue('Cellular.LastGoodAPN.AccessPointName');
       var lastGoodUsername =
           onc.getActiveValue('Cellular.LastGoodAPN.Username');
       var lastGoodPassword =
           onc.getActiveValue('Cellular.LastGoodAPN.Password');
-      var apnList = onc.getActiveValue('Cellular.APNList');
       for (var i = 0; i < apnList.length; i++) {
         var apnDict = apnList[i];
         var option = document.createElement('option');
         var localizedName = apnDict['LocalizedName'];
         var name = localizedName ? localizedName : apnDict['Name'];
         var accessPointName = apnDict['AccessPointName'];
         option.textContent =
             name ? (name + ' (' + accessPointName + ')') : accessPointName;
         option.value = i;
-        // Insert new option before "other" option.
+        // Insert new option before 'other' option.
         apnSelector.add(option, otherOption);
         if (this.selectedApnIndex_ != -1)
           continue;
-        // If this matches the active Apn, or LastGoodApn (or there is no last
-        // good APN), set it as the selected Apn.
-        if ((activeApn == accessPointName &&
-             activeUsername == apnDict['Username'] &&
-             activePassword == apnDict['Password']) ||
-            (!activeApn && !lastGoodApn) ||
-            (!activeApn &&
-             lastGoodApn == accessPointName &&
-             lastGoodUsername == apnDict['Username'] &&
-             lastGoodPassword == apnDict['Password'])) {
+        // If this matches the active Apn name, or LastGoodApn name (or there
+        // is no last good APN), set it as the selected Apn.
+        if ((activeApn == accessPointName) ||
+            (!activeApn && (!lastGoodApn || lastGoodApn == accessPointName))) {
           this.selectedApnIndex_ = i;

[pneubeck (no reviews), 2015/01/23 14:53:28]

this indexing relies on the list initially being empty.
this should be documented/asserted or this assumption be removed.

[stevenjb, 2015/01/23 20:25:33]

Ack. handled by same assert above.

         }
       }
       if (this.selectedApnIndex_ == -1 && activeApn) {
+        this.userApn_ = activeApn;
+        // Create a 'user' entry for any active apn not in the list.
         var activeOption = document.createElement('option');

[pneubeck (no reviews), 2015/01/23 14:53:28]

activeOption -> userOption

[stevenjb, 2015/01/23 20:25:33]

Done.

         activeOption.textContent = activeApn;
         activeOption.value = -1;
+        // Add 'user' entry before 'other' option.
         apnSelector.add(activeOption, otherOption);
         this.selectedApnIndex_ = apnSelector.length - 2;
         this.userApnIndex_ = this.selectedApnIndex_;
       }
+    },
+
+    /**
+     * Helper method called from initializeDetailsPage to initialize the Apn
+     * list.
+     * @private
+     */
+    initializeApnList_: function() {
+      this.selectedApnIndex_ = -1;
+      this.userApnIndex_ = -1;
+
+      var onc = this.onc_;
+      var apnSelector = $('select-apn');
+
+      // Clear APN lists, keep only last element, 'other'.
+      while (apnSelector.length != 1)

[pneubeck (no reviews), 2015/01/23 14:53:28]

it's a rather fragile and harder to understand if this initialize code assumes
that 'other' is the last entry.
to more reliably (i.e. with less assumption) keep only the 'other' entry, you
could do:

var otherOption = $('select-apn-other-option');
while (apnSelector.length != 0) apnSelector.remove(0);
apnSelector.add(otherOption);

[stevenjb, 2015/01/23 20:25:33]

Again, I would re-write this very differently, this evolved from where it
started, I don't want to mess with the logic too much.

+        apnSelector.remove(0);
+
+      var apnList = onc.getActiveValue('Cellular.APNList');
+      if (apnList) {
+        // Populate the list with the existing APNs.
+        this.populateApnList_(apnList);
+      } else {
+        // Create a single 'default' entry.
+        var otherOption = apnSelector[0];

[pneubeck (no reviews), 2015/01/23 14:53:29]

a whole bunch of code has to be read to understand why this indeed is the
'other' entry. please see the next few comments to simplify this.
and please put a comment at the top of this function that explains what the
result will look like.

[stevenjb, 2015/01/23 20:25:33]

Ditto.

+        var activeOption = document.createElement('option');

[pneubeck (no reviews), 2015/01/23 14:53:29]

nit:
 activeOption -> defaultOption

[stevenjb, 2015/01/23 20:25:33]

Done.

+        activeOption.textContent =
+            loadTimeData.getString('cellularApnUseDefault');
+        activeOption.value = -1;
+        apnSelector.add(activeOption, otherOption);

[pneubeck (no reviews), 2015/01/23 14:53:28]

to prepend add the beginning of the list:
  apnSelector.add(activeOption, apnSelector[0])

[stevenjb, 2015/01/23 20:25:33]

I'm not convinced that is more clear (not that any of this is especially clear).

+        this.selectedApnIndex_ = apnSelector.length - 2;

[pneubeck (no reviews), 2015/01/23 14:53:29]

length is 2 here. could be simplified to
  this.selectedApnIndex_ = 0;

[stevenjb, 2015/01/23 20:25:33]

Yes... that is true here. Changed and added an assert.

+      }
       assert(this.selectedApnIndex_ >= 0);
       apnSelector.selectedIndex = this.selectedApnIndex_;
       updateHidden('.apn-list-view', false);
       updateHidden('.apn-details-view', true);
     },
 
     /**
+     * Helper function for setting APN properties.
+     * @param {Object} apnValue Dictionary of APN properties.
+     * @private
+     */
+    setActiveApn_: function(apnValue) {
+      var activeApn = {};
+      var apnName = apnValue['AccessPointName'];
+      if (apnName) {
+        activeApn['AccessPointName'] = apnName;
+        activeApn['Username'] = stringFromValue(apnValue['Username']);
+        activeApn['Password'] = stringFromValue(apnValue['Password']);
+      }
+      // Set the cached ONC data.
+      this.onc_.setProperty('Cellular.APN', activeApn);
+      // Set an ONC object with just the APN values.
+      var oncData = new OncData({});
+      oncData.setProperty('Cellular.APN', activeApn);
+      // TODO(stevenjb): chrome.networkingPrivate.setProperties
+      chrome.send('setProperties', [this.servicePath_, oncData.getData()]);
+    },
+
+    /**
      * Event Listener for the cellular-apn-use-default button.
      * @private
      */
     setDefaultApn_: function() {
-      var onc = this.onc_;
       var apnSelector = $('select-apn');
 
+      // Remove any 'user' entry.

[pneubeck (no reviews), 2015/01/23 14:53:28]

nit: can there be more than one 'user' entry? if not -> 'Remove the 'user' entry
if it exists.'

[stevenjb, 2015/01/23 20:25:33]

"any" can mean that, but that's kind of subtle in English, I'll reword.

       if (this.userApnIndex_ != -1) {
+        assert(this.userApnIndex_ < apnSelector.length - 1);
         apnSelector.remove(this.userApnIndex_);
         this.userApnIndex_ = -1;
       }
 
-      var iApn = -1;
-      var apnList = onc.getActiveValue('Cellular.APNList');
-      if (apnList != undefined && apnList.length > 0) {
-        iApn = 0;
-        var defaultApn = apnList[iApn];
-        var activeApn = {};
-        activeApn['AccessPointName'] =
-            stringFromValue(defaultApn['AccessPointName']);
-        activeApn['Username'] = stringFromValue(defaultApn['Username']);
-        activeApn['Password'] = stringFromValue(defaultApn['Password']);
-        onc.setProperty('Cellular.APN', activeApn);
-        chrome.send('setApn', [this.servicePath_,
-                               activeApn['AccessPointName'],
-                               activeApn['Username'],
-                               activeApn['Password']]);
-      }
+      var apnList = this.onc_.getActiveValue('Cellular.APNList');
+      var iApn = (apnList != undefined && apnList.length > 0) ? 0 : -1;
       apnSelector.selectedIndex = iApn;
       this.selectedApnIndex_ = iApn;
 
+      this.setActiveApn_({}); // Use default

[pneubeck (no reviews), 2015/01/23 14:53:28]

so with 'Use default' you mean the 'default' entry if apnList is empty
and the first item of apnList if its not empty.

please clarify that in a comment.

[stevenjb, 2015/01/23 20:25:33]

Done.

+
       updateHidden('.apn-list-view', false);
       updateHidden('.apn-details-view', true);
     },
 
     /**
      * Event Listener for the cellular-apn-set button.
      * @private
      */
     setApn_: function(apnValue) {
       if (apnValue == '')
         return;
 
-      var onc = this.onc_;
       var apnSelector = $('select-apn');
 
       var activeApn = {};
       activeApn['AccessPointName'] = stringFromValue(apnValue);
       activeApn['Username'] = stringFromValue($('cellular-apn-username').value);
       activeApn['Password'] = stringFromValue($('cellular-apn-password').value);
-      onc.setProperty('Cellular.APN', activeApn);
+      this.setActiveApn_(activeApn);
+      // Set the user selected APN.
       this.userApn_ = activeApn;
-      chrome.send('setApn', [this.servicePath_,
-                             activeApn['AccessPointName'],
-                             activeApn['Username'],
-                             activeApn['Password']]);
 
+      // Remove any existing 'user' entry.
       if (this.userApnIndex_ != -1) {
+        assert(this.userApnIndex_ < apnSelector.length - 1);
         apnSelector.remove(this.userApnIndex_);
         this.userApnIndex_ = -1;
       }
 
+      // Create a new 'user' entry with the new active apn.
       var option = document.createElement('option');
       option.textContent = activeApn['AccessPointName'];
       option.value = -1;
       option.selected = true;
       apnSelector.add(option, apnSelector[apnSelector.length - 1]);

[pneubeck (no reviews), 2015/01/23 14:53:29]

this is now putting the userOption in front of the last element.
please unify this code with the code in populateApnList_.

[stevenjb, 2015/01/23 20:25:33]

Done... we are now deep into the grey area of almost but not quite completely
re-factoring this...

       this.userApnIndex_ = apnSelector.length - 2;
       this.selectedApnIndex_ = this.userApnIndex_;
 
       updateHidden('.apn-list-view', false);
       updateHidden('.apn-details-view', true);
     },
 
     /**
      * Event Listener for the cellular-apn-cancel button.
      * @private
      */
     cancelApn_: function() {
-      $('select-apn').selectedIndex = this.selectedApnIndex_;
-      updateHidden('.apn-list-view', false);
-      updateHidden('.apn-details-view', true);
+      this.initializeApnList_();
     },
 
     /**
      * Event Listener for the select-apn button.
      * @private
      */
     selectApn_: function() {
       var onc = this.onc_;
       var apnSelector = $('select-apn');
-      var apnDict;
       if (apnSelector[apnSelector.selectedIndex].value != -1) {
         var apnList = onc.getActiveValue('Cellular.APNList');
         var apnIndex = apnSelector.selectedIndex;
         assert(apnIndex < apnList.length);
-        apnDict = apnList[apnIndex];
-        chrome.send('setApn', [this.servicePath_,
-                               stringFromValue(apnDict['AccessPointName']),
-                               stringFromValue(apnDict['Username']),
-                               stringFromValue(apnDict['Password'])]);
         this.selectedApnIndex_ = apnIndex;
+        this.setActiveApn_(apnList[apnIndex]);
       } else if (apnSelector.selectedIndex == this.userApnIndex_) {
-        apnDict = this.userApn_;
-        chrome.send('setApn', [this.servicePath_,
-                               stringFromValue(apnDict['AccessPointName']),
-                               stringFromValue(apnDict['Username']),
-                               stringFromValue(apnDict['Password'])]);
         this.selectedApnIndex_ = apnSelector.selectedIndex;
-      } else {
-        $('cellular-apn').value =
-            stringFromValue(onc.getActiveValue('Cellular.APN.AccessPointName'));
-        $('cellular-apn-username').value =
-            stringFromValue(onc.getActiveValue('Cellular.APN.Username'));
-        $('cellular-apn-password').value =
-            stringFromValue(onc.getActiveValue('Cellular.APN.Password'));
+        this.setActiveApn_(this.userApn_);
+      } else { // 'Other'
+        var apnDict;

[pneubeck (no reviews), 2015/01/23 14:53:28]

this code is only for pre-filling some edit fields at the moment you select the
'custom APN' option, i guess.

if so, lgtm.

[stevenjb, 2015/01/23 20:25:33]

Correct.

+        if (this.userApn_['AccessPointName']) {
+          apnDict = this.userApn_;

[pneubeck (no reviews), 2015/01/23 14:53:29]

this is rather confusing.
there seem to be two static entries in the selector list: 'user' and 'other'
(and 'default').
Why is now this.userApn_ relevant if 'other' is selected ?
or is just the comment in line 987 wrong?

[stevenjb, 2015/01/23 20:25:33]

I attempted to improve this. I'll add comments, but:
* If we select 'Other' and have defined a 'User' option, populate the custom
fields with 'User' under the assumption that the user wants to modify those.
* If we do not have a 'User' option, populate it with the 'Active' value.

[pneubeck (no reviews), 2015/01/24 09:33:13]

thanks for clarifying.

+        } else {
+          apnDict = {};
+          apnDict['AccessPointName'] =
+              onc.getActiveValue('Cellular.APN.AccessPointName');
+          apnDict['Username'] = onc.getActiveValue('Cellular.APN.Username');
+          apnDict['Password'] = onc.getActiveValue('Cellular.APN.Password');
+        }
+        $('cellular-apn').value = stringFromValue(apnDict['AccessPointName']);
+        $('cellular-apn-username').value = stringFromValue(apnDict['Username']);
+        $('cellular-apn-password').value = stringFromValue(apnDict['Password']);
         updateHidden('.apn-list-view', true);
         updateHidden('.apn-details-view', false);
       }
     }
   };
 
   /**
    * Enables or Disables all buttons that provide operations on the cellular
    * network.
    */
@@ skipping 675 matching lines @@
 
     // Don't show page name in address bar and in history to prevent people
     // navigate here by hand and solve issue with page session restore.
     PageManager.showPageByName('detailsInternetPage', false);
   };
 
   return {
     DetailsInternetPage: DetailsInternetPage
   };
 });