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 775 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() {
-      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');
       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');
+      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');
         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.
@@ skipping 12 matching lines @@
              lastGoodPassword == apnDict['Password'])) {
           this.selectedApnIndex_ = i;
         }
       }
       if (this.selectedApnIndex_ == -1 && activeApn) {
         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 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) {
         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.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;
 
       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.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_);
         this.userApnIndex_ = -1;
       }
 
       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;
@@ skipping 722 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
   };
 });