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 d8c139231c4264c1e15c1018597705e0e4a5d66b..fa573344d269c9e82c2707f8f104e252258178cc 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) {
       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];
       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,7 +815,7 @@ 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;
@@ -834,6 +833,7 @@ cr.define('options.internet', function() {
         }
       }
       if (this.selectedApnIndex_ == -1 && activeApn) {
+        // Create a 'user' entry for any active apn not in the list.
         var activeOption = document.createElement('option');
         activeOption.textContent = activeApn;
         activeOption.value = -1;
@@ -841,6 +841,36 @@ cr.define('options.internet', function() {
         this.selectedApnIndex_ = apnSelector.length - 2;
         this.userApnIndex_ = this.selectedApnIndex_;
       }
+    },
+
+    /**
+     * Helper method called from initializeDetailsPage to initialize the Apn
+     * list.
+     * @private
+     */
+    initializeApnList_: function() {
+      var onc = this.onc_;
+
+      var apnSelector = $('select-apn');
+
+      // Clear APN lists, keep only last element, 'other'.
+      while (apnSelector.length != 1)
+        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];
+        var activeOption = document.createElement('option');
+        activeOption.textContent =
+            loadTimeData.getString('cellularApnUseDefault');
+        activeOption.value = -1;
+        apnSelector.add(activeOption, otherOption);
+        this.selectedApnIndex_ = apnSelector.length - 2;
+      }
       assert(this.selectedApnIndex_ >= 0);
       apnSelector.selectedIndex = this.selectedApnIndex_;
       updateHidden('.apn-list-view', false);
@@ -848,33 +878,45 @@ 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) {

[pneubeck (no reviews), 2015/01/13 09:48:15]

is the 'else' case meant for clearing the APN settings?

I don't care so much about policy in this case, but I wonder how this goes
together with partial updates in 'setProperties'.

The semantics that you require from networkingPrivate.setProperties is that an
empty-dictionary assigned to a field X means clearing that field X (like Shill
does it in some cases).
That in turn means, if we ever want to merge the partial update with existing
user settings in ONC, that we can't use DictionaryValue::MergeDictionary
anymore.

That is doable but IMO rather delicate and might be easy to implement and use
incorrectly.

[stevenjb, 2015/01/13 17:13:06]

re: 'else': Correct. I can make that explicit or comment it better.

re: semantics: 

Yes, we would have to be careful about any future merging of Shill->ONC
dictionaries with partial updates, but that is already true to some extent with
IP config (the explicit types may mitigate that, but I'm not sure that will
'just work').

We could add another explicit type here, but that seems... tedious.

We could use 'null' to clear the property, but that didn't seem popular last
time :)

If you have a suggestion / preference I am happy to go with that.

+        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.
       if (this.userApnIndex_ != -1) {
         apnSelector.remove(this.userApnIndex_);
         this.userApnIndex_ = -1;
       }
 
       var iApn = -1;
-      var apnList = onc.getActiveValue('Cellular.APNList');
+      var apnList = this.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']]);
+        this.setActiveApn_(apnList[iApn]);
       }
       apnSelector.selectedIndex = iApn;
       this.selectedApnIndex_ = iApn;
@@ -891,25 +933,21 @@ 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']]);
 
-      if (this.userApnIndex_ != -1) {
+      // Remove any existing 'user' entry.
+      if (this.userApnIndex_ != -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;