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

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

Committed: https://crrev.com/86c012e8accd3fbd31b626d416c9f558e9f30c60
Cr-Commit-Position: refs/heads/master@{#321490}

[bartfab (slow), 2015-03-16 18:23:01]

bartfab@chromium.org changed reviewers:
+ isherman@chromium.org, michaelpg@chromium.org, stevenjb@chromium.org

[bartfab (slow), 2015-03-16 18:23:02]

Hi Michael,

Could you review while Steve is busy?

Hi Steve,

Could you give an owner stamp?

Hi Ilya,

Could you review tools/metrics/actions/actions.xml?

[Ilya Sherman, 2015-03-17 01:48:22]

actions.xml lgtm

[michaelpg, 2015-03-17 19:28:16]

https://codereview.chromium.org/1017443002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1017443002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:3787: <message
name="IDS_STATUSBAR_NETWORK_ADD_VPN_TEMPLATE" desc="Template for constructing a
menu item that will ask a given VPN provider to show its 'add network' dialog.">
nit: since this isn't used in the status area (on the login screen), can we give
it a name like the other internet options strings and place it near them? e.g.:
IDS_OPTIONS_SETTINGS_ADD_VPN_TEMPLATE

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/network_list.js (left):

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/network_list.js:659: if
(!this.canUpdateMenu())
why does this need to be moved out to line 416?

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/network_list.js:394: * contents is
regenerated immediately and the menu remains visible.
nit: s/is/are

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/network_list.js:1293: * @return
{!Array<Object<{label: string,
The Object<> notation designates a set. To just designate a "record" type, use:
    @return {!Array<{label: string, ...}>}

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/vpn_providers.js:45: get providers() {
Why do we need these get/setters?

[bartfab (slow), 2015-03-18 21:27:15]

Hi Michael,

Could you take another look?

I switched from an object/dictionary of VPN providers to an array by the way so
that the order gets preserved.

https://codereview.chromium.org/1017443002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1017443002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:3787: <message
name="IDS_STATUSBAR_NETWORK_ADD_VPN_TEMPLATE" desc="Template for constructing a
menu item that will ask a given VPN provider to show its 'add network' dialog.">
On 2015/03/17 19:28:16, michaelpg wrote:
> nit: since this isn't used in the status area (on the login screen), can we
give
> it a name like the other internet options strings and place it near them?
e.g.:
> IDS_OPTIONS_SETTINGS_ADD_VPN_TEMPLATE

Done.

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/network_list.js (left):

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/network_list.js:659: if
(!this.canUpdateMenu())
On 2015/03/17 19:28:16, michaelpg wrote:
> why does this need to be moved out to line 416?

showMenu() needs to be compatible with the base class as well, which has
canUpdateMenu() but does not have updateMenu().

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/network_list.js:394: * contents is
regenerated immediately and the menu remains visible.
On 2015/03/17 19:28:16, michaelpg wrote:
> nit: s/is/are

Done.

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/network_list.js:1293: * @return
{!Array<Object<{label: string,
On 2015/03/17 19:28:16, michaelpg wrote:
> The Object<> notation designates a set. To just designate a "record" type,
use:
>     @return {!Array<{label: string, ...}>}

Done.

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1017443002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/vpn_providers.js:45: get providers() {
On 2015/03/17 19:28:16, michaelpg wrote:
> Why do we need these get/setters?

I had to convert |providers_| from a simple member to a get/set pair because
setting has a side effect now - it triggers observers.

[stevenjb, 2015-03-19 16:02:02]

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:53: },
We should make this more easily compatible with extension event listeners by
passing a function callback instead of an object that is expected to have an
'onVPNProvidersChanged' function.

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:126: };
If we use listener functions instead of observer objects, we don't need this at
all.

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:592: void
InternetOptionsHandler::AddNonVPNConnection(const base::ListValue* args) {
I think I would have preferred leaving this function as it was and adding a
separate AddThirdPartyVpnConnection function, but I guess this is fine.

[bartfab (slow), 2015-03-19 16:22:05]

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:53: },
On 2015/03/19 16:02:02, stevenjb wrote:
> We should make this more easily compatible with extension event listeners by
> passing a function callback instead of an object that is expected to have an
> 'onVPNProvidersChanged' function.

Done.

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:126: };
On 2015/03/19 16:02:02, stevenjb wrote:
> If we use listener functions instead of observer objects, we don't need this
at
> all.

Done.

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/1017443002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:592: void
InternetOptionsHandler::AddNonVPNConnection(const base::ListValue* args) {
On 2015/03/19 16:02:02, stevenjb wrote:
> I think I would have preferred leaving this function as it was and adding a
> separate AddThirdPartyVpnConnection function, but I guess this is fine.

Acknowledged.

[stevenjb, 2015-03-19 17:31:10]

lgtm

[bartfab (slow), 2015-03-19 17:32:29]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-19 17:32:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1017443002/#ps80001
(title: "Finished renaming method.")

[commit-bot: I haz the power, 2015-03-19 17:35:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017443002/80001

[commit-bot: I haz the power, 2015-03-19 18:34:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-19 18:35:31]

Patchset 5 (id:??) landed as
https://crrev.com/4f6e1a2ded3a56e7c2d1360b94990b025a309ecb
Cr-Commit-Position: refs/heads/master@{#321395}

[Dan Beam, 2015-03-19 19:01:51]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

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

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:870:
onVPNProvidersChanged_() {
please try your code before you land it

[Dan Beam, 2015-03-19 19:03:20]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1026463003/ by dbeam@chromium.org.

The reason for reverting is: Invalid JS caught by Closure bots:
http://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Lin....

[michaelpg, 2015-03-19 19:48:29]

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:1275: function
createVPNConnectionWithAddBuiltInProviderCallback_() {
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);
  };
}

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:1310: command:
providers[i].extensionID ?
Then this would just be
createVPNConnectionCallbackForProvider(providers[i].extensionID);

Up to you whether that's preferred or not.

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:44:
this.observers_[i]();
What would happen if an observer tried to set providers?

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:80: * Adds an
observer to be notified when the list of VPN providers changes.
nit: s/notified/called

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

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:870:
onVPNProvidersChanged_() {
On 2015/03/19 19:01:52, Dan Beam wrote:
> please try your code before you land it

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.

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:1275: function
createVPNConnectionWithAddBuiltInProviderCallback_() {
On 2015/03/19 19:48:28, michaelpg wrote:
> 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);
>   };
> }

Done.

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:1310: command:
providers[i].extensionID ?
On 2015/03/19 19:48:28, michaelpg wrote:
> Then this would just be
> createVPNConnectionCallbackForProvider(providers[i].extensionID);
> 
> Up to you whether that's preferred or not.

Done.

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:44:
this.observers_[i]();
On 2015/03/19 19:48:28, michaelpg wrote:
> What would happen if an observer tried to set providers?

As discussed, I added a comment warning about this.

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:80: * Adds an
observer to be notified when the list of VPN providers changes.
On 2015/03/19 19:48:28, michaelpg wrote:
> nit: s/notified/called

Done.

[bartfab (slow), 2015-03-20 00:10:33]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-20 00:10:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1017443002/#ps100001
(title: "Fixed closure errors. Addressed comments.")

[bartfab (slow), 2015-03-20 00:10:55]

The CQ bit was unchecked by bartfab@chromium.org

[bartfab (slow), 2015-03-20 00:16:01]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2015-03-20 00:16:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1017443002/100001

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

lgtm

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:1275: function
createVPNConnectionWithAddBuiltInProviderCallback_() {
On 2015/03/20 00:10:25, bartfab wrote:
> On 2015/03/19 19:48:28, michaelpg wrote:
> > 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);
> >   };
> > }
> 
> Done.

Thanks!

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

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://codereview.chromium.org/1017443002/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/network_list.js:870:
onVPNProvidersChanged_() {
On 2015/03/20 00:10:25, bartfab wrote:
> On 2015/03/19 19:01:52, Dan Beam wrote:
> > please try your code before you land it
> 
> 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.

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

maybe we can turn on ES6 in closure?

[commit-bot: I haz the power, 2015-03-20 01:44:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-20 01:44:51]

Patchset 6 (id:??) landed as
https://crrev.com/86c012e8accd3fbd31b626d416c9f558e9f30c60
Cr-Commit-Position: refs/heads/master@{#321490}