Add ash::VPNDelegate and Chrome OS implementation

Add ash::VPNDelegate and Chrome OS implementation

This CL is the first step toward support for third-party VPNs in ash UI.
t adds a delegate interface that ash UI will use to retrieve the list
of available VPN providers and request that a VPN provider show its "add
network" dialog.

The CL was tested together with follow-up CLs which wire up the actual
UI.

BUG=460428
TEST=Manual, together with follow-ups that wire up the UI

Committed: https://crrev.com/99af3de35226ec378b60dbb89d69c331c8800b62
Cr-Commit-Position: refs/heads/master@{#320939}

[bartfab (slow), 2015-03-09 18:32:47]

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

[bartfab (slow), 2015-03-09 18:32:48]

Hi Daniel,

Could you please review?

Steven, FYI.

[Daniel Erat, 2015-03-09 19:46:19]

https://codereview.chromium.org/984863005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_delegate.h:20: 
nit: declare d'tor here and add empty implementation in .cc file

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:130: NotifyObservers();
do you want to call IsVPNProvider() here to avoid notifying observers
unnecessarily?

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:137: NotifyObservers();
same here

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
File chrome/browser/ui/ash/vpn_delegate_chromeos.h (right):

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
chrome/browser/ui/ash/vpn_delegate_chromeos.h:37: content::BrowserContext*
browser_context,
indent four spaces beyond previous line rather than two

[bartfab (slow), 2015-03-09 21:04:23]

https://codereview.chromium.org/984863005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_delegate.h:20: 
On 2015/03/09 19:46:19, Daniel Erat wrote:
> nit: declare d'tor here and add empty implementation in .cc file

Done.

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:130: NotifyObservers();
On 2015/03/09 19:46:19, Daniel Erat wrote:
> do you want to call IsVPNProvider() here to avoid notifying observers
> unnecessarily?

Excellent idea. Done.

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:137: NotifyObservers();
On 2015/03/09 19:46:19, Daniel Erat wrote:
> same here

Done.

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
File chrome/browser/ui/ash/vpn_delegate_chromeos.h (right):

https://codereview.chromium.org/984863005/diff/1/chrome/browser/ui/ash/vpn_de...
chrome/browser/ui/ash/vpn_delegate_chromeos.h:37: content::BrowserContext*
browser_context,
On 2015/03/09 19:46:19, Daniel Erat wrote:
> indent four spaces beyond previous line rather than two

Done.

[Daniel Erat, 2015-03-09 21:34:43]

lgtm

[stevenjb, 2015-03-09 23:20:02]

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

[stevenjb, 2015-03-09 23:20:03]

https://codereview.chromium.org/984863005/diff/20001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/20001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:86: }
I think we should cache these. Since this is going to be called from the UI, it
will be very easy for it to get called several times per UI frame. (It
shouldn't, and presumably won't initially, but could easily happen over time).

If a user has a large number of extensions installed (uncommon but not unheard
of), this could actually take a noticeable amount of time.

If we cache a vector of VPNProvider structs, and update it initially and in
OnExtension[Un]Loaded, we avoid this potential slowdown.

[bartfab (slow), 2015-03-10 17:54:54]

https://codereview.chromium.org/984863005/diff/20001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/20001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:86: }
On 2015/03/09 23:20:03, stevenjb wrote:
> I think we should cache these. Since this is going to be called from the UI,
it
> will be very easy for it to get called several times per UI frame. (It
> shouldn't, and presumably won't initially, but could easily happen over time).
> 
> If a user has a large number of extensions installed (uncommon but not unheard
> of), this could actually take a noticeable amount of time.
> 
> If we cache a vector of VPNProvider structs, and update it initially and in
> OnExtension[Un]Loaded, we avoid this potential slowdown.

Done. I explicitly re-fetch the entire vector in OnExtension[Un]Loaded() instead
of adding/removing a single entry as I want to make sure that the order of
extensions in that vector matches that in the extension system.

[stevenjb, 2015-03-10 18:06:33]

lgtm with a suggestion

https://codereview.chromium.org/984863005/diff/40001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/40001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:133: }
nit: Even though loading / unloading extensions is infrequent, we could build a
new vector here and compare it to the existing one, then only swap and notify if
they are different. The primary reason I think we should consider doing this is
that when logging in to a new machine with a large number of extensions, this
could create a bunch of unnecessary notifications.

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

https://codereview.chromium.org/984863005/diff/40001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/40001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:133: }
On 2015/03/10 18:06:33, stevenjb wrote:
> nit: Even though loading / unloading extensions is infrequent, we could build
a
> new vector here and compare it to the existing one, then only swap and notify
if
> they are different. The primary reason I think we should consider doing this
is
> that when logging in to a new machine with a large number of extensions, this
> could create a bunch of unnecessary notifications.

UpdateThirdPartyVPNProviders() will only be called in three cases:
1) VPN provider extension loaded.
2) VPN provider extension unloaded.
3) Initial list of VPN provider extensions available.

IIUC, 1) and 2) will always cause the list of VPN providers to change. 3) will
cause a change iff there is at least one VPN provider installed. So the only
optimization I can see is to check whether the list went from empty to empty and
skip the notification then. However, this would be an optimization for the
one-off case 3) only. And since the only observer exists only if the ash VPN
menu is currently open, there is zero chance that we will ever hit this case in
practice.

[stevenjb, 2015-03-10 21:25:13]

Still lgtm

https://codereview.chromium.org/984863005/diff/40001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/40001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:133: }
On 2015/03/10 21:13:48, bartfab wrote:
> On 2015/03/10 18:06:33, stevenjb wrote:
> > nit: Even though loading / unloading extensions is infrequent, we could
build
> a
> > new vector here and compare it to the existing one, then only swap and
notify
> if
> > they are different. The primary reason I think we should consider doing this
> is
> > that when logging in to a new machine with a large number of extensions,
this
> > could create a bunch of unnecessary notifications.
> 
> UpdateThirdPartyVPNProviders() will only be called in three cases:
> 1) VPN provider extension loaded.
> 2) VPN provider extension unloaded.
> 3) Initial list of VPN provider extensions available.
> 
> IIUC, 1) and 2) will always cause the list of VPN providers to change. 3) will
> cause a change iff there is at least one VPN provider installed. So the only
> optimization I can see is to check whether the list went from empty to empty
and
> skip the notification then. However, this would be an optimization for the
> one-off case 3) only. And since the only observer exists only if the ash VPN
> menu is currently open, there is zero chance that we will ever hit this case
in
> practice.

Oh, I see, there is already an IsVPNProvider(extension) check in
OnExtension{Un}Loaded(). Nevermind then, this is fine.

[bartfab (slow), 2015-03-11 14:09:14]

Steven, Daniel, could you take another look? I moved the logic that adds the
built-in OpenVPN/L2TP provider to the list from ash UI into the delegate.

[Daniel Erat, 2015-03-11 14:20:33]

lgtm

[stevenjb, 2015-03-11 16:52:52]

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string is
also a valid ID.
The empty string is used to indicate a built-in VPN, right? We should indicate
that, 'valid ID' is a bit confusing.

https://codereview.chromium.org/984863005/diff/60001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/60001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:143: std::string()));
If we are including OpenVPN/L2TP here, I think it might be better to define a
string constant for that instead of using an empty string?

Also, we maybe shouldn't do it now, but in the future we might prefer treating
OpenVPN and L2TP over IPSec separately in the UI, in which case using a string
identifier would allow us to separate those.

[bartfab (slow), 2015-03-11 17:27:04]

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string is
also a valid ID.
On 2015/03/11 16:52:52, stevenjb wrote:
> The empty string is used to indicate a built-in VPN, right? We should indicate
> that, 'valid ID' is a bit confusing.

I wanted to make it as opaque as possible. I documented that the ID may be empty
but refrained from explaining what it stands for. In theory, the delegate's
consumers should not need to know or care. In practice, there is one spot in ash
where I explicitly look for an empty ID, but that is just for UMA logging. If we
moved the logging into the delegate, we could make the ID truly opaque. I would
still want to document that the ID may be empty though, to make sure it does not
trip up anyone's code.

https://codereview.chromium.org/984863005/diff/60001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/60001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:143: std::string()));
On 2015/03/11 16:52:52, stevenjb wrote:
> If we are including OpenVPN/L2TP here, I think it might be better to define a
> string constant for that instead of using an empty string?

This is how I had implemented it at first but I ran into a problem with this:
ash needs to be able to recognize the built-in provider for UMA purposes. If I
define a constant here, I will either have to duplicate it in ash (brittle) or
share it between this code and ash (no idea how, short of wrapping it in a whole
component of its own).

> Also, we maybe shouldn't do it now, but in the future we might prefer treating
> OpenVPN and L2TP over IPSec separately in the UI, in which case using a string
> identifier would allow us to separate those.

We could still do it then, once we solve the caveat noted above. The ID is never
persisted anywhere, so we can always change it in the code.

[stevenjb, 2015-03-11 18:23:02]

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string is
also a valid ID.
On 2015/03/11 17:27:04, bartfab wrote:
> On 2015/03/11 16:52:52, stevenjb wrote:
> > The empty string is used to indicate a built-in VPN, right? We should
indicate
> > that, 'valid ID' is a bit confusing.
> 
> I wanted to make it as opaque as possible. I documented that the ID may be
empty
> but refrained from explaining what it stands for. In theory, the delegate's
> consumers should not need to know or care. In practice, there is one spot in
ash
> where I explicitly look for an empty ID, but that is just for UMA logging. If
we
> moved the logging into the delegate, we could make the ID truly opaque. I
would
> still want to document that the ID may be empty though, to make sure it does
not
> trip up anyone's code.


I don't think "as opaque as possible" is a good objective :)

Seriously though, while I understand your point, I think we should define the
built-in type kOpenVpnOrL2TP here to make this as clear as possible and solve
the problem of where to define the shared constant. I don't see any problem with
defining that in the Delegate. That both makes this more clear and allows for an
easy transition to separating kOpenVpnOrL2TP into two types later.

[bartfab (slow), 2015-03-11 18:27:14]

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string is
also a valid ID.
On 2015/03/11 18:23:02, stevenjb wrote:
> On 2015/03/11 17:27:04, bartfab wrote:
> > On 2015/03/11 16:52:52, stevenjb wrote:
> > > The empty string is used to indicate a built-in VPN, right? We should
> indicate
> > > that, 'valid ID' is a bit confusing.
> > 
> > I wanted to make it as opaque as possible. I documented that the ID may be
> empty
> > but refrained from explaining what it stands for. In theory, the delegate's
> > consumers should not need to know or care. In practice, there is one spot in
> ash
> > where I explicitly look for an empty ID, but that is just for UMA logging.
If
> we
> > moved the logging into the delegate, we could make the ID truly opaque. I
> would
> > still want to document that the ID may be empty though, to make sure it does
> not
> > trip up anyone's code.
> 
> 
> I don't think "as opaque as possible" is a good objective :)
> 
> Seriously though, while I understand your point, I think we should define the
> built-in type kOpenVpnOrL2TP here to make this as clear as possible and solve
> the problem of where to define the shared constant. I don't see any problem
with
> defining that in the Delegate. That both makes this more clear and allows for
an
> easy transition to separating kOpenVpnOrL2TP into two types later.

The NetworkState annotates each VPN network with its provider ID. So we would
need to put a constant in a place where this code, ash and NetworkState can
access it. Since NetworkState lives in chromeos/network, I cannot think of any
such place.

[stevenjb, 2015-03-11 18:36:23]

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string is
also a valid ID.
On 2015/03/11 18:27:14, bartfab wrote:
> On 2015/03/11 18:23:02, stevenjb wrote:
> > On 2015/03/11 17:27:04, bartfab wrote:
> > > On 2015/03/11 16:52:52, stevenjb wrote:
> > > > The empty string is used to indicate a built-in VPN, right? We should
> > indicate
> > > > that, 'valid ID' is a bit confusing.
> > > 
> > > I wanted to make it as opaque as possible. I documented that the ID may be
> > empty
> > > but refrained from explaining what it stands for. In theory, the
delegate's
> > > consumers should not need to know or care. In practice, there is one spot
in
> > ash
> > > where I explicitly look for an empty ID, but that is just for UMA logging.
> If
> > we
> > > moved the logging into the delegate, we could make the ID truly opaque. I
> > would
> > > still want to document that the ID may be empty though, to make sure it
does
> > not
> > > trip up anyone's code.
> > 
> > 
> > I don't think "as opaque as possible" is a good objective :)
> > 
> > Seriously though, while I understand your point, I think we should define
the
> > built-in type kOpenVpnOrL2TP here to make this as clear as possible and
solve
> > the problem of where to define the shared constant. I don't see any problem
> with
> > defining that in the Delegate. That both makes this more clear and allows
for
> an
> > easy transition to separating kOpenVpnOrL2TP into two types later.
> 
> The NetworkState annotates each VPN network with its provider ID. So we would
> need to put a constant in a place where this code, ash and NetworkState can
> access it. Since NetworkState lives in chromeos/network, I cannot think of any
> such place.

I don't understand. id is populated in vpn_delegate_chromeos.cc:

 vpn_providers_.push_back(ash::VPNProvider(
        l10n_util::GetStringUTF8(IDS_NETWORK_VPN_BUILT_IN_PROVIDER),
        std::string()));

You would just need to replace std::string with kOpenVpnOrL2TP, it doesn't need
to have anything to do with NetworkState.

[stevenjb, 2015-03-11 18:39:48]

On 2015/03/11 18:36:23, stevenjb wrote:
>
https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
> File ash/system/chromeos/network/vpn_delegate.h (right):
> 
>
https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
> ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string
is
> also a valid ID.
> On 2015/03/11 18:27:14, bartfab wrote:
> > On 2015/03/11 18:23:02, stevenjb wrote:
> > > On 2015/03/11 17:27:04, bartfab wrote:
> > > > On 2015/03/11 16:52:52, stevenjb wrote:
> > > > > The empty string is used to indicate a built-in VPN, right? We should
> > > indicate
> > > > > that, 'valid ID' is a bit confusing.
> > > > 
> > > > I wanted to make it as opaque as possible. I documented that the ID may
be
> > > empty
> > > > but refrained from explaining what it stands for. In theory, the
> delegate's
> > > > consumers should not need to know or care. In practice, there is one
spot
> in
> > > ash
> > > > where I explicitly look for an empty ID, but that is just for UMA
logging.
> > If
> > > we
> > > > moved the logging into the delegate, we could make the ID truly opaque.
I
> > > would
> > > > still want to document that the ID may be empty though, to make sure it
> does
> > > not
> > > > trip up anyone's code.
> > > 
> > > 
> > > I don't think "as opaque as possible" is a good objective :)
> > > 
> > > Seriously though, while I understand your point, I think we should define
> the
> > > built-in type kOpenVpnOrL2TP here to make this as clear as possible and
> solve
> > > the problem of where to define the shared constant. I don't see any
problem
> > with
> > > defining that in the Delegate. That both makes this more clear and allows
> for
> > an
> > > easy transition to separating kOpenVpnOrL2TP into two types later.
> > 
> > The NetworkState annotates each VPN network with its provider ID. So we
would
> > need to put a constant in a place where this code, ash and NetworkState can
> > access it. Since NetworkState lives in chromeos/network, I cannot think of
any
> > such place.
> 
> I don't understand. id is populated in vpn_delegate_chromeos.cc:
> 
>  vpn_providers_.push_back(ash::VPNProvider(
>         l10n_util::GetStringUTF8(IDS_NETWORK_VPN_BUILT_IN_PROVIDER),
>         std::string()));
> 
> You would just need to replace std::string with kOpenVpnOrL2TP, it doesn't
need
> to have anything to do with NetworkState.

To be clear: I am explicitly suggesting that we don't use this property for the
provider id in NetworkState, that would be redundant, we already have the
Provider.Type property for identifying OpenVPN and L2TP/IPSec VPNs. This
constant would only exist for populating the list of available providers in the
UI.

[bartfab (slow), 2015-03-11 22:36:26]

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string is
also a valid ID.
On 2015/03/11 18:36:23, stevenjb wrote:
> On 2015/03/11 18:27:14, bartfab wrote:
> > On 2015/03/11 18:23:02, stevenjb wrote:
> > > On 2015/03/11 17:27:04, bartfab wrote:
> > > > On 2015/03/11 16:52:52, stevenjb wrote:
> > > > > The empty string is used to indicate a built-in VPN, right? We should
> > > indicate
> > > > > that, 'valid ID' is a bit confusing.
> > > > 
> > > > I wanted to make it as opaque as possible. I documented that the ID may
be
> > > empty
> > > > but refrained from explaining what it stands for. In theory, the
> delegate's
> > > > consumers should not need to know or care. In practice, there is one
spot
> in
> > > ash
> > > > where I explicitly look for an empty ID, but that is just for UMA
logging.
> > If
> > > we
> > > > moved the logging into the delegate, we could make the ID truly opaque.
I
> > > would
> > > > still want to document that the ID may be empty though, to make sure it
> does
> > > not
> > > > trip up anyone's code.
> > > 
> > > 
> > > I don't think "as opaque as possible" is a good objective :)
> > > 
> > > Seriously though, while I understand your point, I think we should define
> the
> > > built-in type kOpenVpnOrL2TP here to make this as clear as possible and
> solve
> > > the problem of where to define the shared constant. I don't see any
problem
> > with
> > > defining that in the Delegate. That both makes this more clear and allows
> for
> > an
> > > easy transition to separating kOpenVpnOrL2TP into two types later.
> > 
> > The NetworkState annotates each VPN network with its provider ID. So we
would
> > need to put a constant in a place where this code, ash and NetworkState can
> > access it. Since NetworkState lives in chromeos/network, I cannot think of
any
> > such place.
> 
> I don't understand. id is populated in vpn_delegate_chromeos.cc:
> 
>  vpn_providers_.push_back(ash::VPNProvider(
>         l10n_util::GetStringUTF8(IDS_NETWORK_VPN_BUILT_IN_PROVIDER),
>         std::string()));
> 
> You would just need to replace std::string with kOpenVpnOrL2TP, it doesn't
need
> to have anything to do with NetworkState.

For networks using a third-party VPN provider, I need to correlate the network
with the corresponding provider, based on the ID. So VPNDelegate and
NetworkState need to be using the same ID schema. In my current implementation,
I do the same thing for networks using the built-in VPN support. I simply set
the ID to "" in VPNDelegate and NetworkState, creating a correlation between the
two.

I can change it so that for the built-in VPN support, I look at the network type
instead. This will complicate the UI code somewhat as it will no longer be able
to just compare IDs. But it is doable if you feel strongly.

[bartfab (slow), 2015-03-12 11:11:07]

On 2015/03/11 22:36:26, bartfab wrote:
>
https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
> File ash/system/chromeos/network/vpn_delegate.h (right):
> 
>
https://codereview.chromium.org/984863005/diff/60001/ash/system/chromeos/netw...
> ash/system/chromeos/network/vpn_delegate.h:24: // Opaque ID. The empty string
is
> also a valid ID.
> On 2015/03/11 18:36:23, stevenjb wrote:
> > On 2015/03/11 18:27:14, bartfab wrote:
> > > On 2015/03/11 18:23:02, stevenjb wrote:
> > > > On 2015/03/11 17:27:04, bartfab wrote:
> > > > > On 2015/03/11 16:52:52, stevenjb wrote:
> > > > > > The empty string is used to indicate a built-in VPN, right? We
should
> > > > indicate
> > > > > > that, 'valid ID' is a bit confusing.
> > > > > 
> > > > > I wanted to make it as opaque as possible. I documented that the ID
may
> be
> > > > empty
> > > > > but refrained from explaining what it stands for. In theory, the
> > delegate's
> > > > > consumers should not need to know or care. In practice, there is one
> spot
> > in
> > > > ash
> > > > > where I explicitly look for an empty ID, but that is just for UMA
> logging.
> > > If
> > > > we
> > > > > moved the logging into the delegate, we could make the ID truly
opaque.
> I
> > > > would
> > > > > still want to document that the ID may be empty though, to make sure
it
> > does
> > > > not
> > > > > trip up anyone's code.
> > > > 
> > > > 
> > > > I don't think "as opaque as possible" is a good objective :)
> > > > 
> > > > Seriously though, while I understand your point, I think we should
define
> > the
> > > > built-in type kOpenVpnOrL2TP here to make this as clear as possible and
> > solve
> > > > the problem of where to define the shared constant. I don't see any
> problem
> > > with
> > > > defining that in the Delegate. That both makes this more clear and
allows
> > for
> > > an
> > > > easy transition to separating kOpenVpnOrL2TP into two types later.
> > > 
> > > The NetworkState annotates each VPN network with its provider ID. So we
> would
> > > need to put a constant in a place where this code, ash and NetworkState
can
> > > access it. Since NetworkState lives in chromeos/network, I cannot think of
> any
> > > such place.
> > 
> > I don't understand. id is populated in vpn_delegate_chromeos.cc:
> > 
> >  vpn_providers_.push_back(ash::VPNProvider(
> >         l10n_util::GetStringUTF8(IDS_NETWORK_VPN_BUILT_IN_PROVIDER),
> >         std::string()));
> > 
> > You would just need to replace std::string with kOpenVpnOrL2TP, it doesn't
> need
> > to have anything to do with NetworkState.
> 
> For networks using a third-party VPN provider, I need to correlate the network
> with the corresponding provider, based on the ID. So VPNDelegate and
> NetworkState need to be using the same ID schema. In my current
implementation,
> I do the same thing for networks using the built-in VPN support. I simply set
> the ID to "" in VPNDelegate and NetworkState, creating a correlation between
the
> two.
> 
> I can change it so that for the built-in VPN support, I look at the network
type
> instead. This will complicate the UI code somewhat as it will no longer be
able
> to just compare IDs. But it is doable if you feel strongly.

As usual with a lengthy discussion, I think the end result was worth it. The key
insight is that I ended up using |id| for two different purposes: telling
built-in from third-party providers and storing a third-party provider's
extension ID. I added a VPNProvider::Key struct now that stores these two pieces
of information in separate fields. If we add more built-in VPN providers in the
future, we can trivially replace |third_party| with a |type| field. The ash code
uses VPNProvider::Key::MatchesNetwork() to match providers and networks, so it
will not care if the internal format changes.

[bartfab (slow), 2015-03-12 11:13:43]

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
File ash/system/chromeos/network/DEPS (right):

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/DEPS:2: "+third_party/cros_system_api",
This was a transitive dependency already anyway as
ash/system/chromeos/network/DEPS allows "chromeos," which in turn allows
"third_party/cros_system_api."

[stevenjb, 2015-03-12 17:40:45]

lgtm with nits.

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.cc:29: return
network.vpn_provider_extension_id().empty();
We should comment this. It seems at some point we might want to store
Provider.Type here to and compare that, but for now I think a comment will
suffice, e.g. something like 'If no provider extension id is specified, match
any built-in VPN type'.

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:81: // Requests that the VPN provider
identified by |id| show its "add network"
s/id/key/

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:67: return vpn_providers_.size()
> 1;
I don't like relying on this, it seems fragile. I think we should iterate
through the list and return true when an entry has third_party == true.

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:131:
extension_registry_->enabled_extensions();
nit: Don't really need this intermediate.

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:141: ash::VPNProvider::Key(),
nit: Either comment 'Use an empty key for built-in' or add a BuiltInKey() method
to VPNProvider::Key

[bartfab (slow), 2015-03-17 12:38:22]

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.cc:29: return
network.vpn_provider_extension_id().empty();
On 2015/03/12 17:40:45, stevenjb wrote:
> We should comment this. It seems at some point we might want to store
> Provider.Type here to and compare that, but for now I think a comment will
> suffice, e.g. something like 'If no provider extension id is specified, match
> any built-in VPN type'.

Done.

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
File ash/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/984863005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_delegate.h:81: // Requests that the VPN provider
identified by |id| show its "add network"
On 2015/03/12 17:40:45, stevenjb wrote:
> s/id/key/

Done.

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
File chrome/browser/ui/ash/vpn_delegate_chromeos.cc (right):

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:67: return vpn_providers_.size()
> 1;
On 2015/03/12 17:40:45, stevenjb wrote:
> I don't like relying on this, it seems fragile. I think we should iterate
> through the list and return true when an entry has third_party == true.

Done.

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:131:
extension_registry_->enabled_extensions();
On 2015/03/12 17:40:45, stevenjb wrote:
> nit: Don't really need this intermediate.

Done.

https://codereview.chromium.org/984863005/diff/80001/chrome/browser/ui/ash/vp...
chrome/browser/ui/ash/vpn_delegate_chromeos.cc:141: ash::VPNProvider::Key(),
On 2015/03/12 17:40:45, stevenjb wrote:
> nit: Either comment 'Use an empty key for built-in' or add a BuiltInKey()
method
> to VPNProvider::Key

Done.

[bartfab (slow), 2015-03-17 12:38:26]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-17 12:38:27]

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

[commit-bot: I haz the power, 2015-03-17 12:39:03]

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

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

The CQ bit was checked by bartfab@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/984863005/#ps140001
(title: "Fixed crash during device-local account login.")

[commit-bot: I haz the power, 2015-03-17 17:08:54]

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

[commit-bot: I haz the power, 2015-03-17 18:02:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-17 18:02:44]

Patchset 8 (id:??) landed as
https://crrev.com/99af3de35226ec378b60dbb89d69c331c8800b62
Cr-Commit-Position: refs/heads/master@{#320939}