[CrOS Tether] Add the notion of a tether DeviceState.

[CrOS Tether] Add the notion of a tether DeviceState.

This CL:
(1) Adds a tether DeviceState to NetworkStateHandler during initialization.
(2) Adds SetTetherTechnologyState() and SetTetherScanState() functions which can be called from the tether component.
(3) Updates the SetTechnologyEnabled(), SetProhibitedTechnologies(), and FormattedHardwareAddressForType() functions to deal properly with the tether DeviceState.
(4) Adds a new TetherDeviceStateManager class to the tether component which will handle setting the appropriate technology state. The class is mostly unimplemented for now.

BUG=672263
Review-Url: https://codereview.chromium.org/2819993002
Cr-Commit-Position: refs/heads/master@{#466185}
Committed: https://chromium.googlesource.com/chromium/src/+/5df3c973662600cb69c4d5e6ac8ae0d1870d10dc

[Kyle Horimoto, 2017-04-18 20:38:32]

Description was changed from

==========
[CrOS Tether] Add the notion of a tether DeviceState.

This CL is a work in progress - uploading only to share with hansberry@ while I
write tests.

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

to

==========
[CrOS Tether] Add the notion of a tether DeviceState.

This CL:
(1) Adds a tether DeviceState to NetworkStateHandler during initialization.
(2) Adds SetTetherTechnologyState() and SetTetherScanState() functions which can
be called from the tether component.
(3) Updates the SetTechnologyEnabled(), SetProhibitedTechnologies(), and
FormattedHardwareAddressForType() functions to deal properly with the tether
DeviceState.
(4) Adds a new TetherDeviceStateManager class to the tether component which will
handle setting the appropriate technology state. The class is mostly
unimplemented for now.

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

[Kyle Horimoto, 2017-04-18 21:06:26]

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

[Kyle Horimoto, 2017-04-18 21:06:26]

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

[Kyle Horimoto, 2017-04-18 21:06:26]

stevenjb: //chromeos/network changes
hansberry: //chromeos/components/tether changes

[Kyle Horimoto, 2017-04-18 21:06:27]

stevenjb: //chromeos/network changes
hansberry: //chromeos/components/tether changes

[stevenjb, 2017-04-19 21:21:45]

This looks like it includes changes from another CL?

To upload just the differences, use:

git cl upstream other_branch_name

That will also make this CL dependent on the other CL (assuming the other branch
was uploaded for review).

[Kyle Horimoto, 2017-04-19 23:11:33]

Rebased and set upstream properly. PTAL.

[Ryan Hansberry, 2017-04-20 01:38:36]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/devic...
File chromeos/network/device_state.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/devic...
chromeos/network/device_state.cc:12: #include
"chromeos/network/tether_constants.h"
Is this include necessary?

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/manag...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/manag...
chromeos/network/managed_state.h:115: friend class NetworkStateHandler;
Why is this necessary?

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:156: // Tether does not exist in
Shill, so call SetTetherTechnologyState() and
The comment suggests that SetTetherTechnologyState() is called here but I don't
see a call.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:209: // If tethering is prohibited,
set |tether_state_| and remove tethering
Tether*

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
Just return empty string here instead.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:115: // Sets the tether technology
state. Because tether networks do not represent
Please refer to as "Tether" throughout documentation for consistency.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:278: // TODO(khorimoto): Add network
update functionality for tether networks.
Why will we need this for Tether?

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504: TechnologyState tether_state_ =
TechnologyState::TECHNOLOGY_PROHIBITED;
nit: tether_technology_state_

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504: TechnologyState tether_state_ =
TechnologyState::TECHNOLOGY_PROHIBITED;
I think a better default may be unavailable.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/tethe...
File chromeos/network/tether_constants.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/tethe...
chromeos/network/tether_constants.cc:12: const char kTetherSignalStrength[] =
"Tether.SignalStrength";
This is my bad for not asking in a previous CL, but why are the naming
conventions different here?

[Kyle Horimoto, 2017-04-20 02:16:08]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/devic...
File chromeos/network/device_state.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/devic...
chromeos/network/device_state.cc:12: #include
"chromeos/network/tether_constants.h"
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> Is this include necessary?

Done.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/manag...
File chromeos/network/managed_state.h (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/manag...
chromeos/network/managed_state.h:115: friend class NetworkStateHandler;
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> Why is this necessary?

To call set_type().

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:156: // Tether does not exist in
Shill, so call SetTetherTechnologyState() and
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> The comment suggests that SetTetherTechnologyState() is called here but I
don't
> see a call.

Done.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:209: // If tethering is prohibited,
set |tether_state_| and remove tethering
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> Tether*

Done.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> Just return empty string here instead.

I don't think we should - we should return the MAC address corresponding to the
Wi-Fi network.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:115: // Sets the tether technology
state. Because tether networks do not represent
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> Please refer to as "Tether" throughout documentation for consistency.

Done.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:278: // TODO(khorimoto): Add network
update functionality for tether networks.
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> Why will we need this for Tether?

We don't actually - you're right.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504: TechnologyState tether_state_ =
TechnologyState::TECHNOLOGY_PROHIBITED;
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> nit: tether_technology_state_

Done.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504: TechnologyState tether_state_ =
TechnologyState::TECHNOLOGY_PROHIBITED;
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> I think a better default may be unavailable.

No, PROHIBITED makes more sense. This should only be changed if the the tether
Initializer starts up. If the policy disallows it from starting up, then it
should be PROHIBITED.

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/tethe...
File chromeos/network/tether_constants.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/tethe...
chromeos/network/tether_constants.cc:12: const char kTetherSignalStrength[] =
"Tether.SignalStrength";
On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> This is my bad for not asking in a previous CL, but why are the naming
> conventions different here?

Shill constants are all in the "<UppercaseDeviceStateType>.<UppercaseProperty>"
format, and the device paths/names don't have readable names, so I just chose
the "tether-device-path" naming convention. It really doesn't matter what the
non-Shill values are as long as they are constants.

[stevenjb, 2017-04-20 16:49:43]

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:159: enabled ? TECHNOLOGY_ENABLED :
TECHNOLOGY_AVAILABLE;
We need to ensure that this transition is valid:
if (tether_technology_state_ != avaialble or enabled) trigger error callback;
else set tether_technology_state_

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:487: DCHECK(tether_technology_state_
== TECHNOLOGY_ENABLED);
This probably shouldn't be a DCHECK. I can imagine a situation where one part of
the UI posts a task to add a network state (e.g. from JS) while another part of
the code (e.g. policy) disables tether. Instead we should just log an error and
return.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:520: DCHECK(tether_technology_state_
== TECHNOLOGY_ENABLED);
ditto

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:553: DCHECK(tether_technology_state_
== TECHNOLOGY_ENABLED);
ditto

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:763: }
This seems unnecessary, and placing the code at 790 inside the loop seems
awkward.

Instead, at line 807, managed_map should still contain a tether device if there
is one. At that point we should move it to managed_list and call
set_update_received/requested.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:268: // UI.
I don't think we should put that logic in the startup / UI code. Instead, this
method should trigger a scan request to the tether code which it is free to
ignore. i.e. this function is a 'request', not a directive.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:502: // up.
nit: s/is changed by the tether component when it starts up/is managed by the
tether component/

[Ryan Hansberry, 2017-04-20 16:53:54]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
On 2017/04/20 02:16:08, Kyle Horimoto wrote:
> On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> > Just return empty string here instead.
> 
> I don't think we should - we should return the MAC address corresponding to
the
> Wi-Fi network.

But we know at this point that the network does not have a MAC address, because
it's a Tether network. Perhaps I'm misunderstanding -- do Tether networks now do
have MAC addresses?

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/tethe...
File chromeos/network/tether_constants.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/tethe...
chromeos/network/tether_constants.cc:12: const char kTetherSignalStrength[] =
"Tether.SignalStrength";
On 2017/04/20 02:16:08, Kyle Horimoto wrote:
> On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> > This is my bad for not asking in a previous CL, but why are the naming
> > conventions different here?
> 
> Shill constants are all in the
"<UppercaseDeviceStateType>.<UppercaseProperty>"
> format, and the device paths/names don't have readable names, so I just chose
> the "tether-device-path" naming convention. It really doesn't matter what the
> non-Shill values are as long as they are constants.

I was sure you were following a convention, was just curious. Thanks!

[stevenjb, 2017-04-20 18:13:45]

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:179: tether_technology_state_ =
technology_state;
So, to go with my comments in UpdateManagedList(), we should probably add a
helper, EnsureTetherDeviceState(), and call that here and at the top of
SetTetherScanState.

Also, EnsureTetherDeviceState() should only add a device if the technology state
is not unavailable, and should remove it if the technology becomes unavailable.
i.e. there should not be a DeviceState entry when tether is not available.

[stevenjb, 2017-04-20 18:29:07]

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504:
TechnologyState::TECHNOLOGY_PROHIBITED;
This should be initialized to UNAVAILABLE which should also indicate that no
DeviceState for Tether exists. i.e. this is equivalent to NSH and Chrome not
having any awareness of Tether. We should rely on SetTetherTechnologyState()
getting called with something other than UNAVAILABLE to create a DeviceState.
(This should only be set to PROHIBITED by policy code).

[Kyle Horimoto, 2017-04-20 19:38:46]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
On 2017/04/20 16:53:54, Ryan Hansberry wrote:
> On 2017/04/20 02:16:08, Kyle Horimoto wrote:
> > On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> > > Just return empty string here instead.
> > 
> > I don't think we should - we should return the MAC address corresponding to
> the
> > Wi-Fi network.
> 
> But we know at this point that the network does not have a MAC address,
because
> it's a Tether network. Perhaps I'm misunderstanding -- do Tether networks now
do
> have MAC addresses?

stevenjb@, can you comment on this?

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:159: enabled ? TECHNOLOGY_ENABLED :
TECHNOLOGY_AVAILABLE;
On 2017/04/20 16:49:41, stevenjb wrote:
> We need to ensure that this transition is valid:
> if (tether_technology_state_ != avaialble or enabled) trigger error callback;
> else set tether_technology_state_

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:179: tether_technology_state_ =
technology_state;
On 2017/04/20 18:13:45, stevenjb wrote:
> So, to go with my comments in UpdateManagedList(), we should probably add a
> helper, EnsureTetherDeviceState(), and call that here and at the top of
> SetTetherScanState.
> 
> Also, EnsureTetherDeviceState() should only add a device if the technology
state
> is not unavailable, and should remove it if the technology becomes
unavailable.
> i.e. there should not be a DeviceState entry when tether is not available.
> 

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:487: DCHECK(tether_technology_state_
== TECHNOLOGY_ENABLED);
On 2017/04/20 16:49:41, stevenjb wrote:
> This probably shouldn't be a DCHECK. I can imagine a situation where one part
of
> the UI posts a task to add a network state (e.g. from JS) while another part
of
> the code (e.g. policy) disables tether. Instead we should just log an error
and
> return.

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:520: DCHECK(tether_technology_state_
== TECHNOLOGY_ENABLED);
On 2017/04/20 16:49:41, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:553: DCHECK(tether_technology_state_
== TECHNOLOGY_ENABLED);
On 2017/04/20 16:49:41, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:763: }
On 2017/04/20 16:49:41, stevenjb wrote:
> This seems unnecessary, and placing the code at 790 inside the loop seems
> awkward.
> 
> Instead, at line 807, managed_map should still contain a tether device if
there
> is one. At that point we should move it to managed_list and call
> set_update_received/requested.

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:268: // UI.
On 2017/04/20 16:49:42, stevenjb wrote:
> I don't think we should put that logic in the startup / UI code. Instead, this
> method should trigger a scan request to the tether code which it is free to
> ignore. i.e. this function is a 'request', not a directive.
> 

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:502: // up.
On 2017/04/20 16:49:42, stevenjb wrote:
> nit: s/is changed by the tether component when it starts up/is managed by the
> tether component/

Done.

https://codereview.chromium.org/2819993002/diff/160001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504:
TechnologyState::TECHNOLOGY_PROHIBITED;
On 2017/04/20 18:29:07, stevenjb wrote:
> This should be initialized to UNAVAILABLE which should also indicate that no
> DeviceState for Tether exists. i.e. this is equivalent to NSH and Chrome not
> having any awareness of Tether. We should rely on SetTetherTechnologyState()
> getting called with something other than UNAVAILABLE to create a DeviceState.
> (This should only be set to PROHIBITED by policy code).

Done.

[stevenjb, 2017-04-20 20:01:11]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
On 2017/04/20 19:38:46, Kyle Horimoto wrote:
> On 2017/04/20 16:53:54, Ryan Hansberry wrote:
> > On 2017/04/20 02:16:08, Kyle Horimoto wrote:
> > > On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> > > > Just return empty string here instead.
> > > 
> > > I don't think we should - we should return the MAC address corresponding
to
> > the
> > > Wi-Fi network.
> > 
> > But we know at this point that the network does not have a MAC address,
> because
> > it's a Tether network. Perhaps I'm misunderstanding -- do Tether networks
now
> do
> > have MAC addresses?
> 
> stevenjb@, can you comment on this?

Why wouldn't a tether network show the MAC address of the hotspot? When I set up
my phone as a hotspot and connect to it it has a valid MAC address. (I'm not
sure how useful that is, but for some purposes it may be, and from Chrome's
perspective that *is* the MAC address).

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_connection_handler.cc:122:
"enabled-or-disabled-when-not-available";
nit: I would just make this 'not-available'. The caller knows that it was
attempting to enable or disable.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:162: // this is not the case, invoke
the error callback.
nit: I think this is clear from the code (i..e the comment doesn't seem to add
anything)

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:197: // Signal Device/Technology state
changed.
I would actually call EnsureTetherDeviceState() explicitly here; this is the
only place that should affect whether or not a device state exists.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:667: }
nit: no {}

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:855: EnsureTetherDeviceState();
Rather than re-creating the tether state here, you could do:

// Move any existing Tether device to |managed_list|.
auto iter = managed_map.find(Tether);
if (iter != managed_map.end()) {
  managed_list->push_back(std::move(iter->second));
  managed_map.erase(iter);
}

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504: // UNAVAILABLE; this value is
managed by the tether component.
nit: (I didn't notice this before): No need to mention what this is initialized
to since that is clear in the next line.

[Kyle Horimoto, 2017-04-20 20:49:20]

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_connection_handler.cc:122:
"enabled-or-disabled-when-not-available";
On 2017/04/20 20:01:10, stevenjb wrote:
> nit: I would just make this 'not-available'. The caller knows that it was
> attempting to enable or disable.

Done.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:162: // this is not the case, invoke
the error callback.
On 2017/04/20 20:01:11, stevenjb wrote:
> nit: I think this is clear from the code (i..e the comment doesn't seem to add
> anything)

Done.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:197: // Signal Device/Technology state
changed.
On 2017/04/20 20:01:11, stevenjb wrote:
> I would actually call EnsureTetherDeviceState() explicitly here; this is the
> only place that should affect whether or not a device state exists.

Done.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:667: }
On 2017/04/20 20:01:11, stevenjb wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:855: EnsureTetherDeviceState();
On 2017/04/20 20:01:11, stevenjb wrote:
> Rather than re-creating the tether state here, you could do:
> 
> // Move any existing Tether device to |managed_list|.
> auto iter = managed_map.find(Tether);
> if (iter != managed_map.end()) {
>   managed_list->push_back(std::move(iter->second));
>   managed_map.erase(iter);
> }

Done.

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2819993002/diff/180001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:504: // UNAVAILABLE; this value is
managed by the tether component.
On 2017/04/20 20:01:11, stevenjb wrote:
> nit: (I didn't notice this before): No need to mention what this is
initialized
> to since that is clear in the next line.

Done.

[Ryan Hansberry, 2017-04-20 21:27:02]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
On 2017/04/20 20:01:10, stevenjb wrote:
> On 2017/04/20 19:38:46, Kyle Horimoto wrote:
> > On 2017/04/20 16:53:54, Ryan Hansberry wrote:
> > > On 2017/04/20 02:16:08, Kyle Horimoto wrote:
> > > > On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> > > > > Just return empty string here instead.
> > > > 
> > > > I don't think we should - we should return the MAC address corresponding
> to
> > > the
> > > > Wi-Fi network.
> > > 
> > > But we know at this point that the network does not have a MAC address,
> > because
> > > it's a Tether network. Perhaps I'm misunderstanding -- do Tether networks
> now
> > do
> > > have MAC addresses?
> > 
> > stevenjb@, can you comment on this?
> 
> Why wouldn't a tether network show the MAC address of the hotspot? When I set
up
> my phone as a hotspot and connect to it it has a valid MAC address. (I'm not
> sure how useful that is, but for some purposes it may be, and from Chrome's
> perspective that *is* the MAC address).

If that's the case, then we should return the mac_address() of the WiFi network,
not the Tether network (presumably an empty string).

[Kyle Horimoto, 2017-04-20 21:28:45]

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2819993002/diff/140001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:376: network =
GetNetworkStateFromGuid(network->tether_guid());
On 2017/04/20 21:27:02, Ryan Hansberry wrote:
> On 2017/04/20 20:01:10, stevenjb wrote:
> > On 2017/04/20 19:38:46, Kyle Horimoto wrote:
> > > On 2017/04/20 16:53:54, Ryan Hansberry wrote:
> > > > On 2017/04/20 02:16:08, Kyle Horimoto wrote:
> > > > > On 2017/04/20 01:38:35, Ryan Hansberry wrote:
> > > > > > Just return empty string here instead.
> > > > > 
> > > > > I don't think we should - we should return the MAC address
corresponding
> > to
> > > > the
> > > > > Wi-Fi network.
> > > > 
> > > > But we know at this point that the network does not have a MAC address,
> > > because
> > > > it's a Tether network. Perhaps I'm misunderstanding -- do Tether
networks
> > now
> > > do
> > > > have MAC addresses?
> > > 
> > > stevenjb@, can you comment on this?
> > 
> > Why wouldn't a tether network show the MAC address of the hotspot? When I
set
> up
> > my phone as a hotspot and connect to it it has a valid MAC address. (I'm not
> > sure how useful that is, but for some purposes it may be, and from Chrome's
> > perspective that *is* the MAC address).
> 
> If that's the case, then we should return the mac_address() of the WiFi
network,
> not the Tether network (presumably an empty string).

That's what I'm already doing ;)

[stevenjb, 2017-04-20 21:54:37]

lgtm

[Kyle Horimoto, 2017-04-20 21:56:13]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:57:02]

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

[Ryan Hansberry, 2017-04-20 22:29:21]

lgtm

[commit-bot: I haz the power, 2017-04-20 23:19:59]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492725373567060,
"parent_rev": "a6c3346f39d2e05d591d3719f4756b55916ad313", "commit_rev":
"5df3c973662600cb69c4d5e6ac8ae0d1870d10dc"}

[commit-bot: I haz the power, 2017-04-20 23:20:09]

Description was changed from

==========
[CrOS Tether] Add the notion of a tether DeviceState.

This CL:
(1) Adds a tether DeviceState to NetworkStateHandler during initialization.
(2) Adds SetTetherTechnologyState() and SetTetherScanState() functions which can
be called from the tether component.
(3) Updates the SetTechnologyEnabled(), SetProhibitedTechnologies(), and
FormattedHardwareAddressForType() functions to deal properly with the tether
DeviceState.
(4) Adds a new TetherDeviceStateManager class to the tether component which will
handle setting the appropriate technology state. The class is mostly
unimplemented for now.

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

to

==========
[CrOS Tether] Add the notion of a tether DeviceState.

This CL:
(1) Adds a tether DeviceState to NetworkStateHandler during initialization.
(2) Adds SetTetherTechnologyState() and SetTetherScanState() functions which can
be called from the tether component.
(3) Updates the SetTechnologyEnabled(), SetProhibitedTechnologies(), and
FormattedHardwareAddressForType() functions to deal properly with the tether
DeviceState.
(4) Adds a new TetherDeviceStateManager class to the tether component which will
handle setting the appropriate technology state. The class is mostly
unimplemented for now.

BUG=672263

Review-Url: https://codereview.chromium.org/2819993002
Cr-Commit-Position: refs/heads/master@{#466185}
Committed:
https://chromium.googlesource.com/chromium/src/+/5df3c973662600cb69c4d5e6ac8a...
==========

[commit-bot: I haz the power, 2017-04-20 23:20:10]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/5df3c973662600cb69c4d5e6ac8a...

[keishi, 2017-04-21 01:19:48]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2836543002/ by keishi@chromium.org.

The reason for reverting is: NetworkStateHandlerTest.TetherTechnologyState
started failing on Linux ChromiumOS Tests (dbg)

https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C....