Remove the configuration of Tether-associated Wi-Fi networks once connectivity is lost.

Remove the configuration of Tether-associated Wi-Fi networks once connectivity is lost.

Also, once a Wi-Fi network is removed (e.g. due to explicit config
removal), remove its associated Tether network.

BUG=672263
Review-Url: https://codereview.chromium.org/2821103003
Cr-Commit-Position: refs/heads/master@{#466838}
Committed: https://chromium.googlesource.com/chromium/src/+/84eea314227407de46fbfde376d082bd41faa438

[Ryan Hansberry, 2017-04-17 19:53:25]

Description was changed from

==========
Remove the configuration of Tether-associated Wi-Fi networks once connectivity
is lost.

Also, once a Wi-Fi network is removed (e.g. due to explicit config
removal), remove its associated Tether network.

BUG=672263
==========

to

==========
Remove the configuration of Tether-associated Wi-Fi networks once connectivity
is lost.

Also, once a Wi-Fi network is removed (e.g. due to explicit config
removal), remove its associated Tether network.

BUG=672263
==========

[Ryan Hansberry, 2017-04-17 19:53:25]

hansberry@chromium.org changed reviewers:
+ khorimoto@chromium.org, stevenjb@chromium.org

[Kyle Horimoto, 2017-04-17 20:34:53]

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/fake_managed_network_configuration_handler.cc
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.cc:47: //
last_removed_configuration_path_ = service_path;
Can't you just use this line? You should be able to assign a const string to a
non-const variable.

In any case, don't leave commented-out code in the CL.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/fake_managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.h:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
Should this file be added to //chromeos/network? I don't think it belongs here.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.h:76:
const std::string& last_removed_configuration_path() const;
Inline this.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/initializer_unittest.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/initializer_unittest.cc:167: base::WrapUnique(new
FakeManagedNetworkConfigurationHandler());
MakeUnique

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler.cc:20:
PA_LOG(INFO) << "Successfully removed Wi-Fi network.";
Add more information to this log. Since the callback doesn't accept a parameter,
just add one here and add pass the parameter to base::Bind() below.

Same for the failure callback.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler.cc:59:
managed_network_configuration_handler_->RemoveConfiguration(
Can you actually move this functionality to a new file and call it from here?
This logic needs to be shared with other parts of the tether component (e.g., it
should be called when the user manually disconnects or when the user logs out).

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler_unittest.cc
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler_unittest.cc:101:
EXPECT_EQ(wifi_service_path_, fake_managed_network_configuration_handler_
nit: Can you fit the second parameter on its own line? This looks weird.

https://codereview.chromium.org/2821103003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:634:
RemoveTetherNetworkState(tether_network_guid);
I don't think this should be a responsibility of NetworkStateHandler. Since the
tether component is adding the network, I think it should also remove the
network when it no longer needs it. NetworkStateHandler should just clear the
GUID as it did before this CL. Let's revert the changes to this file and move
the RemoveTetherNetworkState() call to the tether component.

[stevenjb, 2017-04-18 21:03:36]

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/fake_managed_network_configuration_handler.cc
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.cc:48:
const_cast<std::string&>(last_removed_configuration_path_) = service_path;
I'm pretty sure you can do the same thing with a mock using WillOnce or
WillByDefault().

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/fake_managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.h:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> Should this file be added to //chromeos/network? I don't think it belongs
here.
+1, although we do also already have a MockMNCH, and while I'm not a huge fan of
mocks it may be should for unit tests.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler.cc:59:
managed_network_configuration_handler_->RemoveConfiguration(
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> Can you actually move this functionality to a new file and call it from here?
> This logic needs to be shared with other parts of the tether component (e.g.,
it
> should be called when the user manually disconnects or when the user logs
out).

Do we really want to remove the configuration on any disconnect, e.g. every time
the user walks away with their phone? It means we would need to check for an
existing configuration before creating a new one, but that seems better than
regenerating the configuration each time?

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler_unittest.cc
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler_unittest.cc:101:
EXPECT_EQ(wifi_service_path_, fake_managed_network_configuration_handler_
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> nit: Can you fit the second parameter on its own line? This looks weird.

'git cl format'

https://codereview.chromium.org/2821103003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:634:
RemoveTetherNetworkState(tether_network_guid);
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> I don't think this should be a responsibility of NetworkStateHandler. Since
the
> tether component is adding the network, I think it should also remove the
> network when it no longer needs it. NetworkStateHandler should just clear the
> GUID as it did before this CL. Let's revert the changes to this file and move
> the RemoveTetherNetworkState() call to the tether component.

+1. That allows the Tether code to make the decision whether to remove a Tether
network when it's associated WiFi network goes out of range, or keep it around
and update it's state (e.g. to 'out of range').

[Kyle Horimoto, 2017-04-18 21:14:00]

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler.cc:59:
managed_network_configuration_handler_->RemoveConfiguration(
On 2017/04/18 21:03:36, stevenjb wrote:
> On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> > Can you actually move this functionality to a new file and call it from
here?
> > This logic needs to be shared with other parts of the tether component
(e.g.,
> it
> > should be called when the user manually disconnects or when the user logs
> out).
> 
> Do we really want to remove the configuration on any disconnect, e.g. every
time
> the user walks away with their phone? It means we would need to check for an
> existing configuration before creating a new one, but that seems better than
> regenerating the configuration each time?

Yes, we do want to remove the configuration on any disconnect. While it is
possible to lose a connection by physically moving out of range of the phone,
that is an edge case. The common cause of disconnections is the hotspot being
shut down on the phone side (due either to lack of Internet traffic or due to
the user explicitly shutting it down). Once the hotspot is shut down, a new SSID
and password are generated the next time that tethering starts up, so the old
configuration is no longer valid. We should assume that the configuration will
never be used again once a disconnection occurs.

[Ryan Hansberry, 2017-04-20 20:57:58]

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/fake_managed_network_configuration_handler.cc
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.cc:47: //
last_removed_configuration_path_ = service_path;
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> Can't you just use this line? You should be able to assign a const string to a
> non-const variable.
> 
> In any case, don't leave commented-out code in the CL.

Removed this file.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.cc:48:
const_cast<std::string&>(last_removed_configuration_path_) = service_path;
On 2017/04/18 21:03:35, stevenjb wrote:
> I'm pretty sure you can do the same thing with a mock using WillOnce or
> WillByDefault().

Removed this file.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/fake_managed_network_configuration_handler.h
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.h:1: //
Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/04/18 21:03:36, stevenjb wrote:
> On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> > Should this file be added to //chromeos/network? I don't think it belongs
> here.
> +1, although we do also already have a MockMNCH, and while I'm not a huge fan
of
> mocks it may be should for unit tests.
> 
> 

Removed this file.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/fake_managed_network_configuration_handler.h:76:
const std::string& last_removed_configuration_path() const;
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> Inline this.

Removed this file.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/initializer_unittest.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/initializer_unittest.cc:167: base::WrapUnique(new
FakeManagedNetworkConfigurationHandler());
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> MakeUnique

Done.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler.cc:20:
PA_LOG(INFO) << "Successfully removed Wi-Fi network.";
On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> Add more information to this log. Since the callback doesn't accept a
parameter,
> just add one here and add pass the parameter to base::Bind() below.
> 
> Same for the failure callback.

Done.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler.cc:59:
managed_network_configuration_handler_->RemoveConfiguration(
On 2017/04/18 21:14:00, Kyle Horimoto wrote:
> On 2017/04/18 21:03:36, stevenjb wrote:
> > On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> > > Can you actually move this functionality to a new file and call it from
> here?
> > > This logic needs to be shared with other parts of the tether component
> (e.g.,
> > it
> > > should be called when the user manually disconnects or when the user logs
> > out).
> > 
> > Do we really want to remove the configuration on any disconnect, e.g. every
> time
> > the user walks away with their phone? It means we would need to check for an
> > existing configuration before creating a new one, but that seems better than
> > regenerating the configuration each time?
> 
> Yes, we do want to remove the configuration on any disconnect. While it is
> possible to lose a connection by physically moving out of range of the phone,
> that is an edge case. The common cause of disconnections is the hotspot being
> shut down on the phone side (due either to lack of Internet traffic or due to
> the user explicitly shutting it down). Once the hotspot is shut down, a new
SSID
> and password are generated the next time that tethering starts up, so the old
> configuration is no longer valid. We should assume that the configuration will
> never be used again once a disconnection occurs.

Done.

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_network_disconnection_handler_unittest.cc
(right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_network_disconnection_handler_unittest.cc:101:
EXPECT_EQ(wifi_service_path_, fake_managed_network_configuration_handler_
On 2017/04/18 21:03:36, stevenjb wrote:
> On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> > nit: Can you fit the second parameter on its own line? This looks weird.
> 
> 'git cl format'
> 

This was the style given by git cl format :(

https://codereview.chromium.org/2821103003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2821103003/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:634:
RemoveTetherNetworkState(tether_network_guid);
On 2017/04/18 21:03:36, stevenjb wrote:
> On 2017/04/17 20:34:53, Kyle Horimoto wrote:
> > I don't think this should be a responsibility of NetworkStateHandler. Since
> the
> > tether component is adding the network, I think it should also remove the
> > network when it no longer needs it. NetworkStateHandler should just clear
the
> > GUID as it did before this CL. Let's revert the changes to this file and
move
> > the RemoveTetherNetworkState() call to the tether component.
> 
> +1. That allows the Tether code to make the decision whether to remove a
Tether
> network when it's associated WiFi network goes out of range, or keep it around
> and update it's state (e.g. to 'out of range').

Done.

[Kyle Horimoto, 2017-04-21 01:05:11]

lgtm

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
File chromeos/components/tether/network_configuration_remover.cc (right):

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
chromeos/components/tether/network_configuration_remover.cc:19: PA_LOG(INFO) <<
"Successfully removed Wi-Fi network " << guid << ".";
nit: "network with GUID " << guid

Same below.

[stevenjb, 2017-04-21 20:15:29]

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
File chromeos/components/tether/network_configuration_remover.h (right):

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
chromeos/components/tether/network_configuration_remover.h:28: virtual void
RemoveNetworkConfiguration(const std::string& wifi_network_guid);
Document this

[Ryan Hansberry, 2017-04-24 17:56:21]

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
File chromeos/components/tether/network_configuration_remover.h (right):

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
chromeos/components/tether/network_configuration_remover.h:28: virtual void
RemoveNetworkConfiguration(const std::string& wifi_network_guid);
On 2017/04/21 20:15:28, stevenjb wrote:
> Document this

Done.

[Ryan Hansberry, 2017-04-24 17:58:23]

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
File chromeos/components/tether/network_configuration_remover.cc (right):

https://codereview.chromium.org/2821103003/diff/40001/chromeos/components/tet...
chromeos/components/tether/network_configuration_remover.cc:19: PA_LOG(INFO) <<
"Successfully removed Wi-Fi network " << guid << ".";
On 2017/04/21 01:05:11, Kyle Horimoto wrote:
> nit: "network with GUID " << guid
> 
> Same below.

Done.

[stevenjb, 2017-04-24 21:22:16]

lgtm

[Ryan Hansberry, 2017-04-24 23:02:05]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-24 23:02:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2821103003/#ps80001
(title: "khorimoto@ comment.")

[commit-bot: I haz the power, 2017-04-24 23:02:43]

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-04-24 23:06:51]

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

[commit-bot: I haz the power, 2017-04-24 23:06:53]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Hansberry, 2017-04-24 23:29:44]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-24 23:29:45]

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

[commit-bot: I haz the power, 2017-04-24 23:30:26]

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-04-25 00:34:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493076584995710,
"parent_rev": "70d2a00a72c50c344436f139b2a3f5c09ec337bf", "commit_rev":
"84eea314227407de46fbfde376d082bd41faa438"}

[commit-bot: I haz the power, 2017-04-25 00:35:35]

Description was changed from

==========
Remove the configuration of Tether-associated Wi-Fi networks once connectivity
is lost.

Also, once a Wi-Fi network is removed (e.g. due to explicit config
removal), remove its associated Tether network.

BUG=672263
==========

to

==========
Remove the configuration of Tether-associated Wi-Fi networks once connectivity
is lost.

Also, once a Wi-Fi network is removed (e.g. due to explicit config
removal), remove its associated Tether network.

BUG=672263

Review-Url: https://codereview.chromium.org/2821103003
Cr-Commit-Position: refs/heads/master@{#466838}
Committed:
https://chromium.googlesource.com/chromium/src/+/84eea314227407de46fbfde376d0...
==========

[commit-bot: I haz the power, 2017-04-25 00:35:37]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/84eea314227407de46fbfde376d0...