From 86c012e8accd3fbd31b626d416c9f558e9f30c60 Mon Sep 17 00:00:00 2001 From: bartfab Date: Thu, 19 Mar 2015 18:44:01 -0700 Subject: [PATCH] Allow users to add third-party VPNs from the settings page This CL adds "Add $provider..." entries to the "Private network" and "Add connection" drop-down menus on the Chrome OS settings page. The contents of these menus updates dynamically as VPN providers are added/removed. BUG=460428 TEST=Manual Review URL: https://codereview.chromium.org/1017443002 Cr-Commit-Position: refs/heads/master@{#321490} --- chrome/app/chromeos_strings.grdp | 3 + .../resources/options/chromeos/network_list.js | 142 ++++++++++++++++----- .../resources/options/chromeos/vpn_providers.js | 43 ++++++- .../options/chromeos/internet_options_handler.cc | 34 ++++- .../options/chromeos/internet_options_handler.h | 3 +- .../chromeos/internet_options_handler_strings.cc | 2 +- tools/metrics/actions/actions.xml | 25 +++- 7 files changed, 202 insertions(+), 50 deletions(-) diff --git a/chrome/app/chromeos_strings.grdp b/chrome/app/chromeos_strings.grdp index bb7958b01e41..d92603f50f98 100644 --- a/chrome/app/chromeos_strings.grdp +++ b/chrome/app/chromeos_strings.grdp @@ -1888,6 +1888,9 @@ Press any key to continue exploring. Remembered networks + + Add $1OpenVPN... + Not connected diff --git a/chrome/browser/resources/options/chromeos/network_list.js b/chrome/browser/resources/options/chromeos/network_list.js index 09e1a6a10b6b..ab2bd3f79aa9 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_: function() { + // 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_: function() { + 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,62 @@ 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 a VPN + * provider. If |opt_extensionID| is omitted, the dialog for the built-in + * OpenVPN/L2TP provider is shown. Otherwise, |opt_extensionID| identifies the + * third-party provider for which the dialog should be shown. + * @param {string=} opt_extensionID Extension ID identifying the third-party + * VPN provider for which the dialog should be shown. + * @return {function()} The created callback. + * @private + */ + function createVPNConnectionCallback_(opt_extensionID) { + return function() { + sendChromeMetricsAction(opt_extensionID ? + 'Options_NetworkAddVPNThirdParty' : + 'Options_NetworkAddVPNBuiltIn'); + chrome.send('addVPNConnection', + opt_extensionID ? [opt_extensionID] : undefined); + }; + } + + /** + * 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: createVPNConnectionCallback_(providers[i].extensionID), + data: {} + }); + } + return entries; + } + + /** * Whether the Network list is disabled. Only used for display purpose. */ cr.defineProperty(NetworkList, 'disabled', cr.PropertyKind.BOOL_ATTR); diff --git a/chrome/browser/resources/options/chromeos/vpn_providers.js b/chrome/browser/resources/options/chromeos/vpn_providers.js index 911c68487fbf..06da41b53399 100644 --- a/chrome/browser/resources/options/chromeos/vpn_providers.js +++ b/chrome/browser/resources/options/chromeos/vpn_providers.js @@ -26,6 +26,34 @@ cr.define('options', function() { providers_: [], /** + * Observers who will be called when the list of VPN providers changes. + * @type {!Array} + */ + observers_: [], + + /** + * The VPN providers enabled in the primary user's profile. + * @type {!Array<{name: string, extensionID: ?string}>} + */ + get providers() { + return this.providers_; + }, + set providers(providers) { + this.providers_ = providers; + for (var i = 0; i < this.observers_.length; ++i) + this.observers_[i](); + }, + + /** + * Adds an observer to be called when the list of VPN providers changes. + * @param {!function()} observer The observer to add. + * @private + */ + addObserver_: function(observer) { + this.observers_.push(observer); + }, + + /** * Formats a network name for display purposes. If the network belongs to * a third-party VPN provider, the provider name is added to the network * name. @@ -50,12 +78,23 @@ cr.define('options', function() { }; /** + * Adds an observer to be called when the list of VPN providers changes. Note + * that an observer may in turn call setProviders() but should be careful not + * to get stuck in an infinite loop as every change to the list of VPN + * providers will cause the observers to be called again. + * @param {!function()} observer The observer to add. + */ + VPNProviders.addObserver = function(observer) { + VPNProviders.getInstance().addObserver_(observer); + }; + + /** * Returns the list of VPN providers enabled in the primary user's profile. * @return {!Array<{name: string, extensionID: ?string}>} The list of VPN * providers enabled in the primary user's profile. */ VPNProviders.getProviders = function() { - return VPNProviders.getInstance().providers_; + return VPNProviders.getInstance().providers; }; /** @@ -64,7 +103,7 @@ cr.define('options', function() { * of VPN providers enabled in the primary user's profile. */ VPNProviders.setProviders = function(providers) { - VPNProviders.getInstance().providers_ = providers; + VPNProviders.getInstance().providers = providers; }; /** diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc index 403125c093aa..7171b089e0b4 100644 --- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc +++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc @@ -106,7 +106,8 @@ const char kSetNetworkGuidMessage[] = "setNetworkGuid"; const char kRemoveNetworkMessage[] = "removeNetwork"; // TODO(stevenjb): Deprecate these and integrate with settings Web UI. -const char kAddConnectionMessage[] = "addConnection"; +const char kAddVPNConnectionMessage[] = "addVPNConnection"; +const char kAddNonVPNConnectionMessage[] = "addNonVPNConnection"; const char kConfigureNetworkMessage[] = "configureNetwork"; const char kActivateNetworkMessage[] = "activateNetwork"; @@ -289,8 +290,11 @@ void InternetOptionsHandler::InitializePage() { } void InternetOptionsHandler::RegisterMessages() { - web_ui()->RegisterMessageCallback(kAddConnectionMessage, - base::Bind(&InternetOptionsHandler::AddConnection, + web_ui()->RegisterMessageCallback(kAddVPNConnectionMessage, + base::Bind(&InternetOptionsHandler::AddVPNConnection, + base::Unretained(this))); + web_ui()->RegisterMessageCallback(kAddNonVPNConnectionMessage, + base::Bind(&InternetOptionsHandler::AddNonVPNConnection, base::Unretained(this))); web_ui()->RegisterMessageCallback(kRemoveNetworkMessage, base::Bind(&InternetOptionsHandler::RemoveNetwork, @@ -565,7 +569,27 @@ const PrefService* InternetOptionsHandler::GetPrefs() const { return Profile::FromWebUI(web_ui())->GetPrefs(); } -void InternetOptionsHandler::AddConnection(const base::ListValue* args) { + +void InternetOptionsHandler::AddVPNConnection(const base::ListValue* args) { + if (args->empty()) { + // Show the "add network" dialog for the built-in OpenVPN/L2TP provider. + NetworkConfigView::ShowForType(shill::kTypeVPN, GetNativeWindow()); + return; + } + + std::string extension_id; + if (args->GetSize() != 1 || !args->GetString(0, &extension_id)) { + NOTREACHED(); + return; + } + + // Request that the third-party VPN provider identified by |provider_id| + // show its "add network" dialog. + chromeos::VpnServiceFactory::GetForBrowserContext( + GetProfileForPrimaryUser())->SendShowAddDialogToExtension(extension_id); +} + +void InternetOptionsHandler::AddNonVPNConnection(const base::ListValue* args) { std::string onc_type; if (args->GetSize() != 1 || !args->GetString(0, &onc_type)) { NOTREACHED(); @@ -573,8 +597,6 @@ void InternetOptionsHandler::AddConnection(const base::ListValue* args) { } if (onc_type == ::onc::network_type::kWiFi) { NetworkConfigView::ShowForType(shill::kTypeWifi, GetNativeWindow()); - } else if (onc_type == ::onc::network_type::kVPN) { - NetworkConfigView::ShowForType(shill::kTypeVPN, GetNativeWindow()); } else if (onc_type == ::onc::network_type::kCellular) { ChooseMobileNetworkDialog::ShowDialog(GetNativeWindow()); } else { diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h index e09ff0931a7d..9ee4b8bebdcf 100644 --- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h +++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.h @@ -110,7 +110,8 @@ class InternetOptionsHandler : public ::options::OptionsPageUIHandler, const PrefService* GetPrefs() const; // Additional callbacks for managing networks. - void AddConnection(const base::ListValue* args); + void AddVPNConnection(const base::ListValue* args); + void AddNonVPNConnection(const base::ListValue* args); void ConfigureNetwork(const base::ListValue* args); void ActivateNetwork(const base::ListValue* args); void RemoveNetwork(const base::ListValue* args); diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler_strings.cc b/chrome/browser/ui/webui/options/chromeos/internet_options_handler_strings.cc index 70b4cf21f06c..9ff9e1330e7b 100644 --- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler_strings.cc +++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler_strings.cc @@ -40,7 +40,7 @@ StringResource kStringResources[] = { {"useSharedProxies", IDS_OPTIONS_SETTINGS_USE_SHARED_PROXIES}, {"addConnectionTitle", IDS_OPTIONS_SETTINGS_SECTION_TITLE_ADD_CONNECTION}, {"addConnectionWifi", IDS_OPTIONS_SETTINGS_ADD_CONNECTION_WIFI}, - {"addConnectionVPN", IDS_STATUSBAR_NETWORK_ADD_VPN}, + {"addConnectionVPNTemplate", IDS_OPTIONS_SETTINGS_ADD_VPN_TEMPLATE}, {"otherCellularNetworks", IDS_OPTIONS_SETTINGS_OTHER_CELLULAR_NETWORKS}, {"enableDataRoaming", IDS_OPTIONS_SETTINGS_ENABLE_DATA_ROAMING}, {"disableDataRoaming", IDS_OPTIONS_SETTINGS_DISABLE_DATA_ROAMING}, diff --git a/tools/metrics/actions/actions.xml b/tools/metrics/actions/actions.xml index 2731167484b8..1701e70c9b56 100644 --- a/tools/metrics/actions/actions.xml +++ b/tools/metrics/actions/actions.xml @@ -8506,9 +8506,7 @@ should be able to be added at any place in this file. Captive portal authorization bypass proxy is disabled. - - Deprecated as of 03/2015. Option is removed. - + Deprecated as of 03/2015. Option is removed. @@ -8516,9 +8514,7 @@ should be able to be added at any place in this file. Captive portal authorization bypass proxy is enabled. - - Deprecated as of 03/2015. Option is removed. - + Deprecated as of 03/2015. Option is removed. @@ -9077,6 +9073,22 @@ should be able to be added at any place in this file. Please enter the description of this user action. + + stevenjb@chromium.org + + Settings: Internet connection: VPN dropdown: Show built-in dialog for adding + an OpenVPN or L2TP VPN. + + + + + stevenjb@chromium.org + + Settings: Internet connection: VPN dropdown: Show "add network" + dialog for a third-party VPN provider. + + + stevenjb@chromium.org @@ -9117,6 +9129,7 @@ should be able to be added at any place in this file. Settings: Internet connection: VPN dropdown: Join other + Replaced by Options_NetworkAddVPNBuiltIn. -- 2.11.4.GIT