Add third-party VPN support to network details dialog

Add third-party VPN support to network details dialog

This CL adds support for third-party VPNs to the network details dialog:
* The title contains both the VPN provider name and the network name.
* Fields not applicable to third-party VPNs are hidden.
* A new field showing the provider name is added.
* The "Configure" button triggers the VPN provider's configuration UI.

BUG=460428
TEST=Manual

Committed: https://crrev.com/ad31894c146a95c366bd35e797dcd085411502e9
Cr-Commit-Position: refs/heads/master@{#321262}

[bartfab (slow), 2015-03-12 18:26:24]

bartfab@chromium.org changed reviewers:
+ stevenjb@chromium.org

[bartfab (slow), 2015-03-12 18:26:24]

Hi Steven,

Please take a look.

[bartfab (slow), 2015-03-13 17:04:08]

Hi Steven,

Could you have another look? I maintain a separate list of VPN networks now and
assemble provider name + network name in JS, removing the need to modify the
name property in GetManagedPropertiesResult().

[bartfab (slow), 2015-03-13 17:17:08]

On 2015/03/13 17:04:08, bartfab wrote:
> Hi Steven,
> 
> Could you have another look? I maintain a separate list of VPN networks now
and
> assemble provider name + network name in JS, removing the need to modify the
> name property in GetManagedPropertiesResult().

Screen shots are here:

http://crbug.com/460428#c9

[stevenjb, 2015-03-15 17:00:06]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[stevenjb, 2015-03-15 17:00:07]

stevenjb@chromium.org changed required reviewers:
+ michaelpg@chromium.org

[stevenjb, 2015-03-15 17:00:08]

+michaelpg@ - Could you look at the HTML/JS changes? This is a stopgap solution,
so it doesn't need to be perfect, but I am going to be out the next few days and
don't entirely trust my ability to review WebUI over the weekend. We would like
to get this feature in this week.

bartfab@ - I think we will want to be doing the provideId -> name lookup in the
third party VPN extension API, along with events to inform the UI when third
party providers are added. Adding a bunch of custom C++ code to a Settings UI we
are deprecating is a little frustratting, but if this is significantly easier
than providing it in the extension API then I guess we can live with that for
now.

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1322:
oncData.VPNProviderID,
Don't use oncData directly, use onc.getActiveValue(VPNProviderID)

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1324: }
else detailsPage.networkTitle = undefined;

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1655: var
providerID = oncData.VPNProviderID;
Use onc.getActiveValue

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:181: }
This should be done in network_state.cc, not here, since this is going away, but
I guess I can do that when I deprecate this.

(What we really want is to do this when we translate NetworkState to ONC and use
an ONC constant instead of 'built-in'.)

[stevenjb, 2015-03-15 17:02:03]

Modulo my comments and michael's review, this lgtm.
I will do my best to look in at the end of the day over the next few days.

[michaelpg, 2015-03-16 05:45:20]

lgtm

LGTM w/ nits

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:25: * @type
{Object.<string, string>}
nit (25,62,70): dot (.) no longer required for array/object parameters:
{Object<string, string>}

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:142: const
char kBuiltInVPNProviderID[] = "built-in";
nit: make this a @const in vpn_providers.js and add a similar comment there

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:230:
extensions::APIPermission::kVpnProvider);
nit: remove extra indentation

[bartfab (slow), 2015-03-16 10:03:26]

On 2015/03/15 17:00:08, stevenjb wrote:
> +michaelpg@ - Could you look at the HTML/JS changes? This is a stopgap
solution,
> so it doesn't need to be perfect, but I am going to be out the next few days
and
> don't entirely trust my ability to review WebUI over the weekend. We would
like
> to get this feature in this week.
> 
> bartfab@ - I think we will want to be doing the provideId -> name lookup in
the
> third party VPN extension API, along with events to inform the UI when third
> party providers are added. Adding a bunch of custom C++ code to a Settings UI
we
> are deprecating is a little frustratting, but if this is significantly easier
> than providing it in the extension API then I guess we can live with that for
> now.

The VPN extension API would not be the right place for this. It provides
functionality for implementing a VPN provider, not for managing/enumerating
other VPN providers installed on the system. What we need here is an API for
enumerating installed extensions. Such an API exists already, chrome.management.
However, like all other APIs, it only provides information for the *current*
profile while here, we need information from the *primary* profile. I would feel
very uneasy about breaching this barrier without very good security barrier.
IMHO, the best thing to do is to add the functionality to networkingPrivate,
which is accessible by privileged settings pages only.

In fact, I wrote this code so that it can trivially be switched over from
web_ui()->CallJavascriptFunction() to networkingPrivate APIs. Once all the
scaffolding is in place to use the networkingPrivate API throughout this
settings page, UpdateVPNProviders() can move into the API implementation file.
We will just have to replace its last line with the call to a callback that the
settings page provides to us.

> 
>
https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/options/chromeos/internet_detail.js (right):
> 
>
https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/options/chromeos/internet_detail.js:1322:
> oncData.VPNProviderID,
> Don't use oncData directly, use onc.getActiveValue(VPNProviderID)
> 
>
https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/options/chromeos/internet_detail.js:1324: }
> else detailsPage.networkTitle = undefined;
> 
>
https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/options/chromeos/internet_detail.js:1655: var
> providerID = oncData.VPNProviderID;
> Use onc.getActiveValue
> 
>
https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
> (right):
> 
>
https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:181: }
> This should be done in network_state.cc, not here, since this is going away,
but
> I guess I can do that when I deprecate this.
> 
> (What we really want is to do this when we translate NetworkState to ONC and
use
> an ONC constant instead of 'built-in'.)

[bartfab (slow), 2015-03-16 10:04:51]

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1322:
oncData.VPNProviderID,
On 2015/03/15 17:00:08, stevenjb wrote:
> Don't use oncData directly, use onc.getActiveValue(VPNProviderID)

I had based this on existing code. It seemed to me that you were using
onc.getActiveValue(X) for real ONC properties and oncData.X for injected, fake
properties. I guess it makes sense to move to getActiveValue(X) for everything
now that we are trying to move over to real ONC properties throughout. Done.

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1324: }
On 2015/03/15 17:00:08, stevenjb wrote:
> else detailsPage.networkTitle = undefined;

Done.

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/internet_detail.js:1655: var
providerID = oncData.VPNProviderID;
On 2015/03/15 17:00:08, stevenjb wrote:
> Use onc.getActiveValue

Done.

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/vpn_providers.js:25: * @type
{Object.<string, string>}
On 2015/03/16 05:45:20, michaelpg wrote:
> nit (25,62,70): dot (.) no longer required for array/object parameters:
> {Object<string, string>}

Done.

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

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:142: const
char kBuiltInVPNProviderID[] = "built-in";
On 2015/03/16 05:45:20, michaelpg wrote:
> nit: make this a @const in vpn_providers.js and add a similar comment there

Done.

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:181: }
On 2015/03/15 17:00:08, stevenjb wrote:
> This should be done in network_state.cc, not here, since this is going away,
but
> I guess I can do that when I deprecate this.
> 
> (What we really want is to do this when we translate NetworkState to ONC and
use
> an ONC constant instead of 'built-in'.)
> 

Fully ack. My goal was to prepare things so that we can switch over in two
steps:
1) Replace |kTagVPNProviderID| with a real ONC property.
2) Populate the property in network_state.cc instead of here.

https://codereview.chromium.org/1006553002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:230:
extensions::APIPermission::kVpnProvider);
On 2015/03/16 05:45:20, michaelpg wrote:
> nit: remove extra indentation

Done.

[bartfab (slow), 2015-03-18 20:12:51]

Hi Philipp,

Could you take a look while Steven is out?

[bartfab (slow), 2015-03-18 20:13:08]

bartfab@chromium.org changed reviewers:
+ pneubeck@chromium.org

[bartfab (slow), 2015-03-18 20:13:08]

Hi Philipp,

Could you take a look while Steven is out?

[pneubeck (no reviews), 2015-03-18 20:41:30]

diff 5 to 6 lgtm with a few comments.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1322:
detailsPage.networkTitle_ = undefined;
I think it's more idiomatic to do

delete detailsPage.networkTitle_;
or
... = none;

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1657: var
thirdPartyVPN = providerType == 'ThirdPartyVPN';
nit: rename to isThirdPartyVPN

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1668: for (var i =
0; i < providers.length; ++i) {
if this is the only use of getProviders(), then you could as well move this loop
into VPNProviders (where you have it at another place) and expose something like
'getProviderName(extensionId)'

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/vpn_providers.js:23: * @type
{!Array<{name: string, extensionID: ?string>}}
was there a specific reason why you changed from object to array?

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/vpn_providers.js:64: *     ofVPN
providers enabled in the primary user's profile.
typo: ofVPN -> of VPN

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/vpn_providers.js:75: * @private
why is this private? it's called from external

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

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:472: if
(IsVPNProvider(extension.get()))
nit: missing {}

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:478:
BuildVPNProviderDictionary(
still required?

[bartfab (slow), 2015-03-18 21:10:48]

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1322:
detailsPage.networkTitle_ = undefined;
On 2015/03/18 20:41:30, pneubeck wrote:
> I think it's more idiomatic to do
> 
> delete detailsPage.networkTitle_;
> or
> ... = none;

Done.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1657: var
thirdPartyVPN = providerType == 'ThirdPartyVPN';
On 2015/03/18 20:41:30, pneubeck wrote:
> nit: rename to isThirdPartyVPN

Done.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:1668: for (var i =
0; i < providers.length; ++i) {
On 2015/03/18 20:41:30, pneubeck wrote:
> if this is the only use of getProviders(), then you could as well move this
loop
> into VPNProviders (where you have it at another place) and expose something
like
> 'getProviderName(extensionId)'

CL 1017443002 will add another use which needs to access al the information
about each provider.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/vpn_providers.js (right):

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/vpn_providers.js:23: * @type
{!Array<{name: string, extensionID: ?string>}}
On 2015/03/18 20:41:30, pneubeck wrote:
> was there a specific reason why you changed from object to array?

Yes. In CL 1017443002, I will be showing a list of VPN providers. I want to
preserve their order and an object cannot do that.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/vpn_providers.js:64: *     ofVPN
providers enabled in the primary user's profile.
On 2015/03/18 20:41:30, pneubeck wrote:
> typo: ofVPN -> of VPN

Done.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/vpn_providers.js:75: * @private
On 2015/03/18 20:41:30, pneubeck wrote:
> why is this private? it's called from external

Copypasta.

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

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:472: if
(IsVPNProvider(extension.get()))
On 2015/03/18 20:41:30, pneubeck wrote:
> nit: missing {}

Done.

https://codereview.chromium.org/1006553002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:478:
BuildVPNProviderDictionary(
On 2015/03/18 20:41:30, pneubeck wrote:
> still required?

Yes, CL 1017443002 will require the full list of all providers.

[bartfab (slow), 2015-03-19 00:15:42]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-19 00:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, michaelpg@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1006553002/#ps160001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-03-19 00:17:02]

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

[commit-bot: I haz the power, 2015-03-19 01:18:10]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-19 01:18:48]

Patchset 9 (id:??) landed as
https://crrev.com/ad31894c146a95c366bd35e797dcd085411502e9
Cr-Commit-Position: refs/heads/master@{#321262}