Add ash UI for third-party VPNs

Add ash UI for third-party VPNs

This CL replaces the list of VPN networks in the ash tray bubble with
a new implementation that shows VPN providers and networks in a
hierarchical layout, allowing the user to see at a glance which
provider a network belongs to. The only exception is the currently
connected or connecting network, which is detached from its provider
and moved to the top. If there is a connected network, a disconnect
button is shown next to its name.

Disconnected networks are arranged in shill's priority order within
each provider and the providers are arranged in the order of their
highest priority network. Clicking on a disconnected network triggers
a connection attempt. Clicking on the currently connected or
connecting network shows its configuration dialog. Clicking on a
provider shows the provider's "add network" dialog.

BUG=460428
TEST=Manual

Committed: https://crrev.com/1c7a3f84d059301936aee2678061632e99209e27
Cr-Commit-Position: refs/heads/master@{#321108}

[bartfab (slow), 2015-03-09 21:00:34]

bartfab@chromium.org changed reviewers:
+ asvitkine@chromium.org, oshima@chromium.org, stevenjb@chromium.org

[bartfab (slow), 2015-03-09 21:00:35]

Hi Mitsuru,

Could you please review:
  ash/ash.gyp
  ash/ash_chromeos_strings.grdp
  ash/metrics/*
  ash/resources/*
  ui/chromeos/*

Hi Steven,

Could you please review:
  ash/system/*
  chromeos/network/network_state.*

Hi Alexei,

Could you please review:
  tools/metrics/actions/actions.xml

[oshima, 2015-03-10 22:42:06]

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

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/network_state_list_detailed_view.h:17: #include
"ui/chromeos/network/network_list_view_base.h"
forward decl should be enough.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:62: explicit
VPNListEntryBase(VPNListView* parent);
virtual dtor

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:71:
VPNListProviderEntry(VPNListView* parent, const std::string& name);
ditto

[stevenjb, 2015-03-10 23:41:12]

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/tray_vpn.cc:60: return
handler->FirstNetworkByType(NetworkTypePattern::VPN());
nit: I'm not a huge fan of the implicit cast to bool here, how about (to also be
consistent with the patter above):
if
(NetworkHandler::Get()->network_state_handler()->FirstNetworkByType(NetworkTypePattern::VPN())
  return true;
return false;

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:142:
ui::network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);
Adding/removing the animation observer should probably be done in
UpdateFromNetworkState() (which should handle nullptr).

We should also observe NetworkStateHandler and call UpdateFromNetworkState()
from NetworkConnectionStateChanged for |network|. (NetworkIconChanged is only
triggered for animation updates, not state updates).

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:274: }
This can be simplified. If there is a connected or connecting VPN it is
guaranteed to be the first entry in the list.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:281: new
VPNDisconnectButtonEntry(this));
I think the scoped local here is more confusing than helpful. Views are owned by
the view hierarchy, that should be understood by anyone viewing / reviewing
Views code. I would just assign the new button directly to disconnect_button_
and pass that to AddChildView.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:297: const auto& provider =
id_provider_map[id];
nit: A lot of devs object to the use of auto for non iterators. I don't mind too
much myself, but it does obscure what |provider| is (here and in the for loops
above, |network| also). I am OK with it as is, but I do think this code would be
clearer using types in these for loops, and no more verbose, e.g:

for (const NetworkState* network : networks) {
  ...
  const VPNProvider* provider = id_provider_map[id];

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:315: // disconnect button, it
should be scrolled into view.
I think the comment above scroll_to_show_view is sufficient, these additional
comments are redundant (i.e. the code is simpler to read than the comment).

Aslo, maybe use if {} else if {} ... to make it more clear that we should only
have one scroll_to_show_view candidate.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:353: const auto& entry =
network_view_service_path_map_.find(view);
This is a better use of auto :)

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:384: Shell* const shell =
Shell::GetInstance();
I think the const here is more confusing than helpful (we rarely do this).

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:403: scoped_ptr<views::View>
provider(new VPNListProviderEntry(this, name));
Again, I don't think the scoped_ptr makes this more clear, I would just move the
AddChildView immediately after the assignment.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:406: // Add the belonging to this
provider, in the priority order returned by
Add the networks

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:410: scoped_ptr<views::View>
entry(new VPNListNetworkEntry(this, network));
ditto

[oshima, 2015-03-10 23:48:17]

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/tray_vpn.cc:60: return
handler->FirstNetworkByType(NetworkTypePattern::VPN());
On 2015/03/10 23:41:11, stevenjb wrote:
> nit: I'm not a huge fan of the implicit cast to bool here, how about (to also
be
> consistent with the patter above):
> if
>
(NetworkHandler::Get()->network_state_handler()->FirstNetworkByType(NetworkTypePattern::VPN())
>   return true;
> return false;

Yes, this is discouraged, although the following pattern is common in chromium.

 return !!handler->FirstNetworkByType(NetworkTypePattern::VPN());

I don't have strong opinion and will leave this to you guys.

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

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

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/network_state_list_detailed_view.h:17: #include
"ui/chromeos/network/network_list_view_base.h"
On 2015/03/10 22:42:06, oshima wrote:
> forward decl should be enough.

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/tray_vpn.cc:60: return
handler->FirstNetworkByType(NetworkTypePattern::VPN());
On 2015/03/10 23:48:17, oshima wrote:
> On 2015/03/10 23:41:11, stevenjb wrote:
> > nit: I'm not a huge fan of the implicit cast to bool here, how about (to
also
> be
> > consistent with the patter above):
> > if
> >
>
(NetworkHandler::Get()->network_state_handler()->FirstNetworkByType(NetworkTypePattern::VPN())
> >   return true;
> > return false;
> 
> Yes, this is discouraged, although the following pattern is common in
chromium.
> 
>  return !!handler->FirstNetworkByType(NetworkTypePattern::VPN());
> 
> I don't have strong opinion and will leave this to you guys.

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:62: explicit
VPNListEntryBase(VPNListView* parent);
On 2015/03/10 22:42:06, oshima wrote:
> virtual dtor

The virtual destructor is inherited from the base class. There is no technical
reason to override it and I cannot see any in the style guide either.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:71:
VPNListProviderEntry(VPNListView* parent, const std::string& name);
On 2015/03/10 22:42:06, oshima wrote:
> ditto

As above.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:142:
ui::network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);
On 2015/03/10 23:41:12, stevenjb wrote:
> Adding/removing the animation observer should probably be done in
> UpdateFromNetworkState() (which should handle nullptr).

Done.

> 
> We should also observe NetworkStateHandler and call UpdateFromNetworkState()
> from NetworkConnectionStateChanged for |network|. (NetworkIconChanged is only
> triggered for animation updates, not state updates).
> 

I do not think we need to worry about network state changes here. TrayVPN owns a
TrayNetworkStateObserver which will inform us of network state changes via the
following chain:

TrayNetworkStateObserver::NetworkConnectionStateChanged() ->
TrayVPN::NetworkStateChanged() ->
NetworkStateListDetailedView::Update() ->
NetworkStateListDetailedView::UpdateNetworkList() ->
VPNListView::Update()

This is also in line with what the current ui::NetworkListView works. It does
not subscribe to network change events itself. It gets notified via the above
chain.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:274: }
On 2015/03/10 23:41:12, stevenjb wrote:
> This can be simplified. If there is a connected or connecting VPN it is
> guaranteed to be the first entry in the list.

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:281: new
VPNDisconnectButtonEntry(this));
On 2015/03/10 23:41:12, stevenjb wrote:
> I think the scoped local here is more confusing than helpful. Views are owned
by
> the view hierarchy, that should be understood by anyone viewing / reviewing
> Views code. I would just assign the new button directly to disconnect_button_
> and pass that to AddChildView.

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:297: const auto& provider =
id_provider_map[id];
On 2015/03/10 23:41:12, stevenjb wrote:
> nit: A lot of devs object to the use of auto for non iterators. I don't mind
too
> much myself, but it does obscure what |provider| is (here and in the for loops
> above, |network| also). I am OK with it as is, but I do think this code would
be
> clearer using types in these for loops, and no more verbose, e.g:
> 
> for (const NetworkState* network : networks) {
>   ...
>   const VPNProvider* provider = id_provider_map[id];

I changed all non-iterator autos to explicit types. However, I think that this
made the code less readable due to the long, verbose types such as |const
std::pair<const views::View*, std::string>&|.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:315: // disconnect button, it
should be scrolled into view.
On 2015/03/10 23:41:11, stevenjb wrote:
> I think the comment above scroll_to_show_view is sufficient, these additional
> comments are redundant (i.e. the code is simpler to read than the comment).
> 
> Aslo, maybe use if {} else if {} ... to make it more clear that we should only
> have one scroll_to_show_view candidate.

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:353: const auto& entry =
network_view_service_path_map_.find(view);
On 2015/03/10 23:41:12, stevenjb wrote:
> This is a better use of auto :)

Acknowledged.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:384: Shell* const shell =
Shell::GetInstance();
On 2015/03/10 23:41:12, stevenjb wrote:
> I think the const here is more confusing than helpful (we rarely do this).
> 

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:403: scoped_ptr<views::View>
provider(new VPNListProviderEntry(this, name));
On 2015/03/10 23:41:12, stevenjb wrote:
> Again, I don't think the scoped_ptr makes this more clear, I would just move
the
> AddChildView immediately after the assignment.
> 

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:406: // Add the belonging to this
provider, in the priority order returned by
On 2015/03/10 23:41:12, stevenjb wrote:
> Add the networks

Done.

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:410: scoped_ptr<views::View>
entry(new VPNListNetworkEntry(this, network));
On 2015/03/10 23:41:12, stevenjb wrote:
> ditto

Done.

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

I made two changes to this CL:
* I moved the logic that adds the built-in OpenVPN/L2TP
  provider to the list from ash UI into the delegate.
* I updated the UI and CL description to match the newest
  mocks that are currently under review. See
  http://crbug.com/460428#c8 for screen shots.

[Alexei Svitkine (slow), 2015-03-11 19:05:58]

metrics lgtm

[stevenjb, 2015-03-12 18:29:24]

I'm not thrilled about the new design (I think that hiding the list of available
networks when connected/connecting is a mistake), but I will discuss that
separately with Tom.

Otherwise the code lgtm

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

https://codereview.chromium.org/980943005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_list_view.cc:155: return;
This generally shouldn't happen, at least not for long, the item should get
removed, right? In that case this is fine, but we should add a comment since
it's not obvious why doing nothing in that case is OK.

[bartfab (slow), 2015-03-12 18:31:50]

On 2015/03/12 18:29:24, stevenjb wrote:
> I'm not thrilled about the new design (I think that hiding the list of
available
> networks when connected/connecting is a mistake), but I will discuss that
> separately with Tom.
> 
> Otherwise the code lgtm
> 
>
https://codereview.chromium.org/980943005/diff/80001/ash/system/chromeos/netw...
> File ash/system/chromeos/network/vpn_list_view.cc (right):
> 
>
https://codereview.chromium.org/980943005/diff/80001/ash/system/chromeos/netw...
> ash/system/chromeos/network/vpn_list_view.cc:155: return;
> This generally shouldn't happen, at least not for long, the item should get
> removed, right? In that case this is fine, but we should add a comment since
> it's not obvious why doing nothing in that case is OK.

Hiding the list of other networks was something Saswat really wanted and finally
convinced Jenn of :). I am happy to land either version.

[oshima, 2015-03-13 17:30:55]

lgtm

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
File ash/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/980943005/diff/1/ash/system/chromeos/network/...
ash/system/chromeos/network/vpn_list_view.cc:62: explicit
VPNListEntryBase(VPNListView* parent);
On 2015/03/11 11:14:09, bartfab wrote:
> On 2015/03/10 22:42:06, oshima wrote:
> > virtual dtor
> 
> The virtual destructor is inherited from the base class. There is no technical
> reason to override it and I cannot see any in the style guide either.

There used to be, but this seems to be gone now.

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

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

https://codereview.chromium.org/980943005/diff/80001/ash/system/chromeos/netw...
ash/system/chromeos/network/vpn_list_view.cc:155: return;
On 2015/03/12 18:29:24, stevenjb wrote:
> This generally shouldn't happen, at least not for long, the item should get
> removed, right? In that case this is fine, but we should add a comment since
> it's not obvious why doing nothing in that case is OK.

Done.

[bartfab (slow), 2015-03-18 10:38:09]

The CQ bit was checked by bartfab@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, oshima@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/980943005/#ps140001
(title: "Updated after latest round of UI reviews.")

[commit-bot: I haz the power, 2015-03-18 10:38:40]

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

[commit-bot: I haz the power, 2015-03-18 11:28:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-18 11:28:58]

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