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 9b0371700a76ebe83602614c53c42d42fa20049f..1d8ad772cdc332e0520c4684b019fceb3a3e8824 100644
--- a/chrome/browser/resources/options/chromeos/internet_detail.js
+++ b/chrome/browser/resources/options/chromeos/internet_detail.js
@@ -793,18 +793,12 @@ 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() {
-      var onc = this.onc_;
-
+    populateApnList_: function(apnList) {
       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');
@@ -815,7 +809,7 @@ cr.define('options.internet', function() {
           onc.getActiveValue('Cellular.LastGoodAPN.Username');
       var lastGoodPassword =
           onc.getActiveValue('Cellular.LastGoodAPN.Password');
-      var apnList = onc.getActiveValue('Cellular.APNList');
+      var onc = this.onc_;

[pneubeck (no reviews), 2014/12/01 13:54:10]

used before declaration and not afterwards?

[stevenjb, 2015/01/09 20:10:59]

Rebase conflict. Fixed.

       for (var i = 0; i < apnList.length; i++) {
         var apnDict = apnList[i];
         var option = document.createElement('option');
@@ -848,20 +842,69 @@ cr.define('options.internet', function() {
         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 that "other".

[pneubeck (no reviews), 2014/12/01 13:54:10]

i don't understand this sentence. could you rephrase it?

[stevenjb, 2015/01/09 20:10:59]

s/that/,/

+      while (apnSelector.length != 1) {
+        apnSelector.remove(0);
+      }
+      var apnList = onc.getActiveValue('Cellular.APNList');
+      if (apnList) {
+        this.populateApnList_(apnList);
+      } else {
+        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);
+      this.userApnIndex_ = this.selectedApnIndex_;

[pneubeck (no reviews), 2014/12/01 13:54:10]

before this change, userApnIndex_ was not modified by initializeApnList_ if
this.selectedApnIndex_ != -1 || !activeApn.

Please check, whether you really wanted to do that. (I can't say as I don't
understand the code :-) )

[stevenjb, 2015/01/09 20:10:59]

Good catch, I'm not sure why/how this got moved here, but it belongs in
populateApnList_. Moved.

       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 = null;
+      var apnName = apnValue['AccessPointName'];
+      if (apnName) {
+        activeApn = {};
+        activeApn['AccessPointName'] = apnName;
+        activeApn['Username'] = stringFromValue(apnValue['Username']);
+        activeApn['Password'] = stringFromValue(apnValue['Password']);
+      }
+      // Set the cached ONC data.
+      this.onc_.setManagedProperty('Cellular.APN', activeApn);

[pneubeck (no reviews), 2014/12/01 13:54:10]

IIRC, this will change to setProperty after the next rebase.

[stevenjb, 2015/01/09 20:10:59]

Acknowledged.

+      // Set an ONC object with just the APN values.
+      var oncData = new OncData({});
+      oncData.setManagedProperty('Cellular.APN', activeApn);

[pneubeck (no reviews), 2014/12/01 13:54:10]

IIUC, you will change this from NULL to an 'Automatic APN' value?

[stevenjb, 2015/01/09 20:10:59]

I will need to test whether or not we can send an empty dictionary to Shill for
"Automatic" (and confirm that you can live with that), otherwise we may need to
add another Type property, set that, and add more functionality to the ONC
translation.

+      // 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');
 
       if (this.userApnIndex_ != -1) {
@@ -870,20 +913,10 @@ cr.define('options.internet', function() {
       }
 
       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.setManagedProperty('Cellular.APN', activeApn);
-        chrome.send('setApn', [this.servicePath_,
-                               activeApn['AccessPointName'],
-                               activeApn['Username'],
-                               activeApn['Password']]);
+        this.setActiveApn_(apnList[iApn]);
       }
       apnSelector.selectedIndex = iApn;
       this.selectedApnIndex_ = iApn;
@@ -900,19 +933,15 @@ 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.setManagedProperty('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) {

[pneubeck (no reviews), 2014/12/01 13:54:10]

Sorry. Even after staring at this for a while, I have no idea what's going on
here. (Not because of your change but because of the existing weirdness of the
old code).
E.g. why is setApn_ modifying the dom tree?! What for is 'userApnIndex_'
compared to 'selectedApnIndex_'... 

I think you will have to rely a lot on testing this (manually).

[stevenjb, 2015/01/09 20:10:59]

'userApnIndex_' points to a user defined entry in the dropdown. I agree that
this is awkward at best and plan to re-factor the UI in the redesign. For now I
will add comments to help clarify things.

         apnSelector.remove(this.userApnIndex_);