[ash-md] Restores handler for VPN Disconnect button in system menu

[ash-md] Restores handler for VPN Disconnect button in system menu

The handler was unintentionally lost in MD work which got exposed after
turning the MD to be on by default in M-56.
Breaking CL - https://codereview.chromium.org/2463163002

BUG=704299
Review-Url: https://codereview.chromium.org/2768973002
Cr-Commit-Position: refs/heads/master@{#459182}
Committed: https://chromium.googlesource.com/chromium/src/+/c2637be18c2e52da142d6cbf646ab82045e13218

[varkha, 2017-03-22 21:56:27]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[varkha, 2017-03-22 21:56:36]

varkha@chromium.org changed reviewers:
+ tdanderson@chromium.org

[varkha, 2017-03-22 21:57:10]

tdanderson@, can you please take a look? Thanks!

[commit-bot: I haz the power, 2017-03-22 21:57:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 22:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 22:33:36]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-03-22 23:03:54]

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

https://codereview.chromium.org/2768973002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/vpn_list_view.cc:203:
->GetNetworkStateFromGuid(guid_));
In the breaking CL, this query is instead performed via
GetNetworkState(service_path_). Currently this is the only place |guid_| is
used. Can |guid_| be removed, and can we instead call
GetNetworkState(service_path_) here? It is unclear to me whether that will make
a functional difference.

[varkha, 2017-03-23 00:00:31]

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

https://codereview.chromium.org/2768973002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/vpn_list_view.cc:203:
->GetNetworkStateFromGuid(guid_));
On 2017/03/22 23:03:54, tdanderson wrote:
> In the breaking CL, this query is instead performed via
> GetNetworkState(service_path_). Currently this is the only place |guid_| is
> used. Can |guid_| be removed, and can we instead call
> GetNetworkState(service_path_) here? It is unclear to me whether that will
make
> a functional difference.

Removing service_path_ was conscious choice - see
https://codereview.chromium.org/2698473007. I have changed the code to not cache
the service_path and fetch it instead from guid. It would probably be better to
plumb through NetworkConnect::DisconnectFromNetworkId or something like that but
it would make possible merge more difficult. Adding stevenjb@ as a reviewer to
take a look if that is OK and maybe consider adding that interface for network
group.

[varkha, 2017-03-23 00:01:08]

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

[varkha, 2017-03-23 00:01:09]

stevenjb@, please take a look - see a comment about switching from service_path
to guid.

[varkha, 2017-03-23 00:50:13]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 00:50:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 01:38:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 01:38:42]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-03-23 18:33:13]

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:34: #include
"chromeos/network/network_connection_handler.h"
We should actually remove this and depend on chromeos/network/network_connect.h
instead, see below.

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:219:
base::Bind(&IgnoreDisconnectError));
Yeah, so, what we really want is:

chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(network->guid())

That will require adding NetworkConnect::DisconectFromNetworkId(guid), but I
think it is worth it for the extra clarity and to avoid the |network| lookup and
direct NetworkConnectionHandler dependency here.

NetworkConnect::DisconectFromNetworkId would include all of the code above,
including IgnoreDisconnectError.

Thanks!

[varkha, 2017-03-23 19:03:26]

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:219:
base::Bind(&IgnoreDisconnectError));
On 2017/03/23 18:33:13, stevenjb wrote:
> Yeah, so, what we really want is:
> 
> chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(network->guid())
> 
> That will require adding NetworkConnect::DisconectFromNetworkId(guid), but I
> think it is worth it for the extra clarity and to avoid the |network| lookup
and
> direct NetworkConnectionHandler dependency here.
> 
> NetworkConnect::DisconectFromNetworkId would include all of the code above,
> including IgnoreDisconnectError.
> 
> Thanks!
> 

Agreed. It seems like NetworkConnect::ConnectToNetworkId uses a string ID rather
than guid and then the impl takes that and looks up a path, then uses the path
to connect. Is that how we want the DisconnectFromNetworkId to work or did you
really mean guid and not a string ID there?

[stevenjb, 2017-03-23 19:14:06]

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:219:
base::Bind(&IgnoreDisconnectError));
On 2017/03/23 19:03:26, varkha wrote:
> On 2017/03/23 18:33:13, stevenjb wrote:
> > Yeah, so, what we really want is:
> > 
> > chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(network->guid())
> > 
> > That will require adding NetworkConnect::DisconectFromNetworkId(guid), but I
> > think it is worth it for the extra clarity and to avoid the |network| lookup
> and
> > direct NetworkConnectionHandler dependency here.
> > 
> > NetworkConnect::DisconectFromNetworkId would include all of the code above,
> > including IgnoreDisconnectError.
> > 
> > Thanks!
> > 
> 
> Agreed. It seems like NetworkConnect::ConnectToNetworkId uses a string ID
rather
> than guid and then the impl takes that and looks up a path, then uses the path
> to connect. Is that how we want the DisconnectFromNetworkId to work or did you
> really mean guid and not a string ID there?

"string ID" == guid.

[varkha, 2017-03-23 19:20:50]

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:219:
base::Bind(&IgnoreDisconnectError));
On 2017/03/23 19:14:05, stevenjb wrote:
> On 2017/03/23 19:03:26, varkha wrote:
> > On 2017/03/23 18:33:13, stevenjb wrote:
> > > Yeah, so, what we really want is:
> > > 
> > > chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(network->guid())
> > > 
> > > That will require adding NetworkConnect::DisconectFromNetworkId(guid), but
I
> > > think it is worth it for the extra clarity and to avoid the |network|
lookup
> > and
> > > direct NetworkConnectionHandler dependency here.
> > > 
> > > NetworkConnect::DisconectFromNetworkId would include all of the code
above,
> > > including IgnoreDisconnectError.
> > > 
> > > Thanks!
> > > 
> > 
> > Agreed. It seems like NetworkConnect::ConnectToNetworkId uses a string ID
> rather
> > than guid and then the impl takes that and looks up a path, then uses the
path
> > to connect. Is that how we want the DisconnectFromNetworkId to work or did
you
> > really mean guid and not a string ID there?
> 
> "string ID" == guid.

Oh I see. I think I misread the code in
NetworkStateListDetailedView::HandleViewClicked() because it uses both *FromGuid
and *Id suffixes and because it uses |network->guid()| where it could just use
|guid|.

[stevenjb, 2017-03-23 19:25:21]

On 2017/03/23 19:20:50, varkha wrote:
>
https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
> File ash/common/system/chromeos/network/vpn_list_view.cc (right):
> 
>
https://codereview.chromium.org/2768973002/diff/20001/ash/common/system/chrom...
> ash/common/system/chromeos/network/vpn_list_view.cc:219:
> base::Bind(&IgnoreDisconnectError));
> On 2017/03/23 19:14:05, stevenjb wrote:
> > On 2017/03/23 19:03:26, varkha wrote:
> > > On 2017/03/23 18:33:13, stevenjb wrote:
> > > > Yeah, so, what we really want is:
> > > > 
> > > >
chromeos::NetworkConnect::Get()->DisconnectFromNetworkId(network->guid())
> > > > 
> > > > That will require adding NetworkConnect::DisconectFromNetworkId(guid),
but
> I
> > > > think it is worth it for the extra clarity and to avoid the |network|
> lookup
> > > and
> > > > direct NetworkConnectionHandler dependency here.
> > > > 
> > > > NetworkConnect::DisconectFromNetworkId would include all of the code
> above,
> > > > including IgnoreDisconnectError.
> > > > 
> > > > Thanks!
> > > > 
> > > 
> > > Agreed. It seems like NetworkConnect::ConnectToNetworkId uses a string ID
> > rather
> > > than guid and then the impl takes that and looks up a path, then uses the
> path
> > > to connect. Is that how we want the DisconnectFromNetworkId to work or did
> you
> > > really mean guid and not a string ID there?
> > 
> > "string ID" == guid.
> 
> Oh I see. I think I misread the code in
> NetworkStateListDetailedView::HandleViewClicked() because it uses both
*FromGuid
> and *Id suffixes and because it uses |network->guid()| where it could just use
> |guid|.

Yeah... there's some history there that I won't get into, but in the networking
code 'id' will always refer to a guid. My next project is to convert all of this
to a Mojo API using ONC which will hopefully eliminate a lot of confusion...

[stevenjb, 2017-03-23 19:28:05]

This LGTM as-is for merging into 58 as long as we follow up with the suggested
cleanup later.

[varkha, 2017-03-23 19:30:59]

In interest of easing merging, I plan to do the refactoring in a separate CL.
Would that be OK?

[varkha, 2017-03-23 19:32:10]

The CQ bit was checked by varkha@chromium.org

[varkha, 2017-03-23 19:32:40]

The CQ bit was unchecked by varkha@chromium.org

[varkha, 2017-03-23 19:33:00]

Description was changed from

==========
[ash-md] Restores handler for VPN Disconnect button in system menu

The handler was unintentionally lost in MD work which got exposed after
turning the MD to be on by default in M-56.
Braking CL - https://codereview.chromium.org/2463163002

BUG=704299
==========

to

==========
[ash-md] Restores handler for VPN Disconnect button in system menu

The handler was unintentionally lost in MD work which got exposed after
turning the MD to be on by default in M-56.
Breaking CL - https://codereview.chromium.org/2463163002

BUG=704299
==========

[varkha, 2017-03-23 19:33:13]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2017-03-23 19:34:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 19:40:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490297593100770,
"parent_rev": "91d04b6f3dd86851c91fd82107ee1c6c1adce5e5", "commit_rev":
"c2637be18c2e52da142d6cbf646ab82045e13218"}

[commit-bot: I haz the power, 2017-03-23 19:41:20]

Description was changed from

==========
[ash-md] Restores handler for VPN Disconnect button in system menu

The handler was unintentionally lost in MD work which got exposed after
turning the MD to be on by default in M-56.
Breaking CL - https://codereview.chromium.org/2463163002

BUG=704299
==========

to

==========
[ash-md] Restores handler for VPN Disconnect button in system menu

The handler was unintentionally lost in MD work which got exposed after
turning the MD to be on by default in M-56.
Breaking CL - https://codereview.chromium.org/2463163002

BUG=704299

Review-Url: https://codereview.chromium.org/2768973002
Cr-Commit-Position: refs/heads/master@{#459182}
Committed:
https://chromium.googlesource.com/chromium/src/+/c2637be18c2e52da142d6cbf646a...
==========

[commit-bot: I haz the power, 2017-03-23 19:41:21]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c2637be18c2e52da142d6cbf646a...