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 161 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) {
       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');
       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'])) {
           this.selectedApnIndex_ = i;
         }
       }
       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;
         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() {
+      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);
       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) {

[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;
 
       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']]);
 
-      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;
       option.selected = true;
       apnSelector.add(option, apnSelector[apnSelector.length - 1]);
       this.userApnIndex_ = apnSelector.length - 2;
       this.selectedApnIndex_ = this.userApnIndex_;
 
       updateHidden('.apn-list-view', false);
       updateHidden('.apn-details-view', true);
@@ skipping 729 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
   };
 });