Allow users to add third-party VPNs from the settings page

chrome/browser/resources/options/chromeos/network_list.js

Index: chrome/browser/resources/options/chromeos/network_list.js
diff --git a/chrome/browser/resources/options/chromeos/network_list.js b/chrome/browser/resources/options/chromeos/network_list.js
index 09e1a6a10b6bba945a352ae0de0291353c0ddc99..e3629bb964c99bd3f38cec4c026bf72e8c4e0b78 100644
--- a/chrome/browser/resources/options/chromeos/network_list.js
+++ b/chrome/browser/resources/options/chromeos/network_list.js
@@ -375,11 +375,29 @@ cr.define('options.network', function() {
       return null;
     },
 
+    /**
+     * Determines if a menu can be updated on the fly. Menus that cannot be
+     * updated are fully regenerated using createMenu. The advantage of
+     * updating a menu is that it can preserve ordering of networks avoiding
+     * entries from jumping around after an update.
+     * @return {boolean} Whether the menu can be updated on the fly.
+     */
     canUpdateMenu: function() {
       return false;
     },
 
     /**
+     * Removes the current menu contents, causing it to be regenerated when the
+     * menu is shown the next time. If the menu is showing right now, its
+     * contents are regenerated immediately and the menu remains visible.
+     */
+    refreshMenu: function() {
+      this.menu_ = null;
+      if (activeMenu_ == this.getMenuName())
+        this.showMenu();
+    },
+
+    /**
      * Displays a popup menu.
      */
     showMenu: function() {
@@ -393,7 +411,7 @@ cr.define('options.network', function() {
         rebuild = true;
         var existing = $(this.getMenuName());
         if (existing) {
-          if (this.updateMenu())
+          if (this.canUpdateMenu() && this.updateMenu())
             return;
           closeMenu_();
         }
@@ -497,14 +515,14 @@ cr.define('options.network', function() {
       if (this.data_.key == 'WiFi') {
         addendum.push({
           label: loadTimeData.getString('joinOtherNetwork'),
-          command: createAddConnectionCallback_('WiFi'),
+          command: createAddNonVPNConnectionCallback_('WiFi'),
           data: {}
         });
       } else if (this.data_.key == 'Cellular') {
         if (cellularEnabled_ && cellularSupportsScan_) {
           addendum.push({
             label: loadTimeData.getString('otherCellularNetworks'),
-            command: createAddConnectionCallback_('Cellular'),
+            command: createAddNonVPNConnectionCallback_('Cellular'),
             addClass: ['other-cellulars'],
             data: {}
           });
@@ -531,11 +549,7 @@ cr.define('options.network', function() {
         }
         addendum.push(entry);
       } else if (this.data_.key == 'VPN') {
-        addendum.push({
-          label: loadTimeData.getString('joinOtherNetwork'),
-          command: createAddConnectionCallback_('VPN'),
-          data: {}
-        });
+        addendum = addendum.concat(createAddVPNConnectionEntries_());
       }
 
       var list = this.data.rememberedNetworks;
@@ -634,12 +648,7 @@ cr.define('options.network', function() {
       return menu;
     },
 
-    /**
-     * Determines if a menu can be updated on the fly. Menus that cannot be
-     * updated are fully regenerated using createMenu. The advantage of
-     * updating a menu is that it can preserve ordering of networks avoiding
-     * entries from jumping around after an update.
-     */
+    /** @override */
     canUpdateMenu: function() {
       return this.data_.key == 'WiFi' && activeMenu_ == this.getMenuName();
     },
@@ -650,12 +659,11 @@ cr.define('options.network', function() {
      * preferred ordering as determined by the network library.  If the
      * ordering becomes potentially out of sync, then the updated menu is
      * marked for disposal on close.  Reopening the menu will force a
-     * regeneration, which will in turn fix the ordering.
+     * regeneration, which will in turn fix the ordering. This method must only
+     * be called if canUpdateMenu() returned |true|.
      * @return {boolean} True if successfully updated.
      */
     updateMenu: function() {
-      if (!this.canUpdateMenu())
-        return false;
       var oldMenu = $(this.getMenuName());
       var group = oldMenu.getElementsByClassName('network-menu-group')[0];
       if (!group)
@@ -842,18 +850,7 @@ cr.define('options.network', function() {
       // Wi-Fi control is always visible.
       this.update({key: 'WiFi', networkList: []});
 
-      var entryAddWifi = {
-        label: loadTimeData.getString('addConnectionWifi'),
-        command: createAddConnectionCallback_('WiFi')
-      };
-      var entryAddVPN = {
-        label: loadTimeData.getString('addConnectionVPN'),
-        command: createAddConnectionCallback_('VPN')
-      };
-      this.update({key: 'addConnection',
-                   iconType: 'add-connection',
-                   menu: [entryAddWifi, entryAddVPN]
-                  });
+      this.updateAddConnectionMenuEntries_();
 
       var prefs = options.Preferences.getInstance();
       prefs.addEventListener('cros.signed.data_roaming_enabled',
@@ -861,6 +858,43 @@ cr.define('options.network', function() {
             enableDataRoaming_ = event.value.value;
           });
       this.endBatchUpdates();
+
+      options.VPNProviders.addObserver(this.onVPNProvidersChanged_.bind(this));
+    },
+
+    /**
+     * Called when the list of VPN providers changes. Refreshes the contents of
+     * menus that list VPN providers.
+     * @private
+     */
+    onVPNProvidersChanged_() {

[Dan Beam, 2015/03/19 19:01:52]

please try your code before you land it

[bartfab (slow), 2015/03/20 00:10:25]

This is valid JavaScript and works just fine. I did try it before landing the CL
of course.

The problem is that this is ECMAScript 6 syntax but our Closure bot is set up to
accept ECMAScript 5 only, causing the bot to complain.

[Dan Beam, 2015/03/20 01:27:58]

ah, bummer, my bad (I've just never seen it before)

maybe we can turn on ES6 in closure?

+      // Refresh the contents of the VPN menu.
+      var index = this.indexOf('VPN');
+      if (index != undefined)
+        this.getListItemByIndex(index).refreshMenu();
+
+      // Refresh the contents of the "add connection" menu.
+      this.updateAddConnectionMenuEntries_();
+      index = this.indexOf('addConnection');
+      if (index != undefined)
+        this.getListItemByIndex(index).refreshMenu();
+    },
+
+    /**
+     * Updates the entries in the "add connection" menu, based on the VPN
+     * providers currently enabled in the primary user's profile.
+     * @private
+     */
+    updateAddConnectionMenuEntries_() {
+      var entries = [{
+        label: loadTimeData.getString('addConnectionWifi'),
+        command: createAddNonVPNConnectionCallback_('WiFi')
+      }];
+      entries = entries.concat(createAddVPNConnectionEntries_());
+      this.update({key: 'addConnection',
+                   iconType: 'add-connection',
+                   menu: entries
+                  });
     },
 
     /**
@@ -1218,22 +1252,72 @@ cr.define('options.network', function() {
   }
 
   /**
-   * Create a callback function that adds a new connection of the given type.
+   * Creates a callback function that adds a new connection of the given type.
+   * This method may be used for all network types except VPN.
    * @param {string} type An ONC network type
    * @return {function()} The created callback.
    * @private
    */
-  function createAddConnectionCallback_(type) {
+  function createAddNonVPNConnectionCallback_(type) {
     return function() {
       if (type == 'WiFi')
         sendChromeMetricsAction('Options_NetworkJoinOtherWifi');
-      else if (type == 'VPN')
-        sendChromeMetricsAction('Options_NetworkJoinOtherVPN');
-      chrome.send('addConnection', [type]);
+      chrome.send('addNonVPNConnection', [type]);
+    };
+  }
+
+  /**
+   * Creates a callback function that shows the "add network" dialog for the
+   * built-in OpenVPN/L2TP VPN provider.
+   * @return {function()} The created callback.
+   * @private
+   */
+  function createVPNConnectionWithAddBuiltInProviderCallback_() {

[michaelpg, 2015/03/19 19:48:28]

Could these be simplified into one function? e.g.,

function createVPNConnectionCallbackForProvider_(opt_extensionID) {
  return function() {
    sendChromeMetricsAction(opt_extensionID ? 'Options_NetworkAddVPNThirdParty'
: 'Options_NetworkAddVPNBuiltIn');
    chrome.send('addVPNConnection', extensionID ? [extensionID] : undefined);
  };
}

[bartfab (slow), 2015/03/20 00:10:25]

Done.

[michaelpg, 2015/03/20 00:27:28]

Thanks!

+    return function() {
+      sendChromeMetricsAction('Options_NetworkAddVPNBuiltIn');
+      chrome.send('addVPNConnection');
     };
   }
 
   /**
+   * Creates a callback function that asks the third-party VPN provider
+   * identified by |extensionID| to show its "add network" dialog.
+   * @param {string} providerID The VPN provider's extension ID.
+   * @return {function()} The created callback.
+   * @private
+   */
+  function createAddVPNConnectionWithThirdPartyProviderCallback_(extensionID) {
+    return function() {
+      sendChromeMetricsAction('Options_NetworkAddVPNThirdParty');
+      chrome.send('addVPNConnection', [extensionID]);
+    };
+  }
+
+  /**
+   * Generates an "add network" entry for each VPN provider currently enabled in
+   * the primary user's profile.
+   * @return {!Array<{label: string, command: function(), data: !Object}>} The
+   *     list of entries.
+   * @private
+   */
+  function createAddVPNConnectionEntries_() {
+    var entries = [];
+    var providers = options.VPNProviders.getProviders();
+    for (var i = 0; i < providers.length; ++i) {
+      entries.push({
+        label: loadTimeData.getStringF('addConnectionVPNTemplate',
+                                       providers[i].name),
+        command: providers[i].extensionID ?

[michaelpg, 2015/03/19 19:48:28]

Then this would just be
createVPNConnectionCallbackForProvider(providers[i].extensionID);

Up to you whether that's preferred or not.

[bartfab (slow), 2015/03/20 00:10:25]

Done.

+            createAddVPNConnectionWithThirdPartyProviderCallback_(
+                providers[i].extensionID) :
+            createVPNConnectionWithAddBuiltInProviderCallback_(),
+        data: {}
+      });
+    }
+    return entries;
+  }
+
+  /**
    * Whether the Network list is disabled. Only used for display purpose.
    */
   cr.defineProperty(NetworkList, 'disabled', cr.PropertyKind.BOOL_ATTR);