Use setProperties for Cellular APN

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 8477dd8b179b6810931f34f014cc63471472154a..310e7d6e59226a6da0a0b8d70e48ba9ba9727846 100644
--- a/chrome/browser/resources/options/chromeos/internet_detail.js
+++ b/chrome/browser/resources/options/chromeos/internet_detail.js
@@ -179,10 +179,15 @@ cr.define('options.internet', function() {
    * @extends {cr.ui.pageManager.Page}
    */
   function DetailsInternetPage() {
-    // Cached Apn properties
+    // If non-negative, indicates a custom entry in select-apn.
     this.userApnIndex_ = -1;
-    this.selectedApnIndex_ = -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;
+
     // We show the Proxy configuration tab for remembered networks and when
     // configuring a proxy from the login screen.
     this.showProxy_ = false;
@@ -784,18 +789,13 @@ cr.define('options.internet', function() {
     },
 
     /**
-     * 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');
@@ -806,7 +806,6 @@ cr.define('options.internet', function() {
           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');
@@ -816,31 +815,60 @@ cr.define('options.internet', function() {
         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);
@@ -848,37 +876,48 @@ cr.define('options.internet', function() {
     },
 
     /**
+     * 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);
     },
@@ -891,25 +930,24 @@ cr.define('options.internet', function() {
       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;
@@ -927,9 +965,7 @@ cr.define('options.internet', function() {
      * @private
      */
     cancelApn_: function() {
-      $('select-apn').selectedIndex = this.selectedApnIndex_;
-      updateHidden('.apn-list-view', false);
-      updateHidden('.apn-details-view', true);
+      this.initializeApnList_();
     },
 
     /**
@@ -939,31 +975,29 @@ cr.define('options.internet', function() {
     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);
       }