[CrOS Tether] Sort Tether network lists.

[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick settings) are sorted in the same order that the Tether component prioritizes them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful TetherAvailabilityResponse are next in the order, as long as they do not violate the first rule.
(3) All other devices are left in the order they are passed.

This CL also updates the GetNetworkListByType() logic in NetworkStateHandler; after this change, all connected/connecting networks are listed before non-active networks, and Wi-Fi networks associated with Tether networks are no longer returned since they are supposed to be obscured behind their associated Tether networks.

BUG=672263, 729796, 729801
Review-Url: https://codereview.chromium.org/2945643002
Cr-Commit-Position: refs/heads/master@{#481621}
Committed: https://chromium.googlesource.com/chromium/src/+/5a0eba4b0089554977fde98d3a30d3ca74a750a0

[Kyle Horimoto, 2017-06-16 20:51:48]

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

[Kyle Horimoto, 2017-06-16 20:51:48]

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

[Ryan Hansberry, 2017-06-19 19:17:49]

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

https://codereview.chromium.org/2945643002/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:59:
std::unique_ptr<ManagedState> Move(
Why ManagedState instead of NetworkState?

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

https://codereview.chromium.org/2945643002/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scanner_operation_unittest.cc:12: #include
"base/test/scoped_task_environment.h"
It's a shame that this and host_scanner's test need to take on additional (and
obscure) complexity because of the change to HostScanDevicePrioritizer.
Proposal: Make a general HostScanDevicePrioritizer interface that doesn't take a
NetworkStateHandler argument, and which is extended by
MockHostScanDevicePrioritizer, to allow for easy testing. Thoughts?

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:567: network_list_sorted_ = false;
I thought sorting was only determined by most recent TetherAvail and
ConnectTethering responses -- why would this affect sorting?

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:603: network_list_sorted_ = false;
Is this true? Once an element is removed, I would assume the list remains
sorted.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:750: network_list_sorted_ = false;
Again, I'm not sure how this relates to how HostScanDevicePrioritizer sorts
Tether networks.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:1231: if (tether_network_list_sorter_)
Check if network_list_sorted_ is false?

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:1232:
tether_network_list_sorter_->SortTetherNetworkList(&tether_network_list_);
set network_list_sorted_ = true

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.h:79: ManagedStateList* tether_networks)
const = 0;
Why ManagedState and not NetworkState?

[Kyle Horimoto, 2017-06-20 23:44:55]

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

https://codereview.chromium.org/2945643002/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:59:
std::unique_ptr<ManagedState> Move(
On 2017/06/19 19:17:48, Ryan Hansberry wrote:
> Why ManagedState instead of NetworkState?

The vector stored in NetworkStateHandler is a ManagedStateList.

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

https://codereview.chromium.org/2945643002/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scanner_operation_unittest.cc:12: #include
"base/test/scoped_task_environment.h"
On 2017/06/19 19:17:48, Ryan Hansberry wrote:
> It's a shame that this and host_scanner's test need to take on additional (and
> obscure) complexity because of the change to HostScanDevicePrioritizer.
> Proposal: Make a general HostScanDevicePrioritizer interface that doesn't take
a
> NetworkStateHandler argument, and which is extended by
> MockHostScanDevicePrioritizer, to allow for easy testing. Thoughts?

You're right - done.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:567: network_list_sorted_ = false;
On 2017/06/19 19:17:48, Ryan Hansberry wrote:
> I thought sorting was only determined by most recent TetherAvail and
> ConnectTethering responses -- why would this affect sorting?

The algorithm used to sort is an implementation detail of the sorting delegate.
Embedding that logic here violates encapsulation. Imagine, for example, that a
future update to HostScanDevicePrioritizer sorts according to connection
strength. Sorting would be broken in this case.

TL;DR: We shouldn't assume the sorting algorithm used from this class.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:603: network_list_sorted_ = false;
On 2017/06/19 19:17:49, Ryan Hansberry wrote:
> Is this true? Once an element is removed, I would assume the list remains
> sorted.

You're right - removed.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:750: network_list_sorted_ = false;
On 2017/06/19 19:17:48, Ryan Hansberry wrote:
> Again, I'm not sure how this relates to how HostScanDevicePrioritizer sorts
> Tether networks.

Connecting/connected networks should be prioritized over other networks. When a
network is connected, the list should need to be re-sorted.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:1231: if (tether_network_list_sorter_)
On 2017/06/19 19:17:49, Ryan Hansberry wrote:
> Check if network_list_sorted_ is false?

That's already done before SortNetworkList() is called.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:1232:
tether_network_list_sorter_->SortTetherNetworkList(&tether_network_list_);
On 2017/06/19 19:17:49, Ryan Hansberry wrote:
> set network_list_sorted_ = true

That's already done at the end of this function.

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2945643002/diff/40001/chromeos/network/networ...
chromeos/network/network_state_handler.h:79: ManagedStateList* tether_networks)
const = 0;
On 2017/06/19 19:17:49, Ryan Hansberry wrote:
> Why ManagedState and not NetworkState?

tether_network_list_ is a ManagedStateList.

[Ryan Hansberry, 2017-06-21 17:05:37]

lgtm. Have you tested this change with a combination of a cellular network and
tether networks to make sure they're sorted correctly within the Mobile Data
section?

[Kyle Horimoto, 2017-06-21 19:01:25]

Description was changed from

==========
[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick
settings) are sorted in the same order that the Tether component prioritizes
them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful
ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful
TetherAvailabilityResponse are next in the order, as long as they do not violate
the first rule.
(3) All other devices are left in the order they are passed.

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

to

==========
[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick
settings) are sorted in the same order that the Tether component prioritizes
them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful
ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful
TetherAvailabilityResponse are next in the order, as long as they do not violate
the first rule.
(3) All other devices are left in the order they are passed.

This CL also updates the GetNetworkListByType() logic in NetworkStateHandler;
after this change, all connected/connecting networks are listed before
non-active networks, and Wi-Fi networks associated with Tether networks are no
longer returned since they are supposed to be obscured behind their associated
Tether networks.

BUG=672263,729796
==========

[Kyle Horimoto, 2017-06-21 19:02:14]

On 2017/06/21 17:05:37, Ryan Hansberry wrote:
> lgtm. Have you tested this change with a combination of a cellular network and
> tether networks to make sure they're sorted correctly within the Mobile Data
> section?

I previously had not done that, but I updated the CL with this change. Note that
this CL now also fixes crbug.com/729796.

[stevenjb, 2017-06-21 20:59:04]

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer_impl.cc (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer_impl.cc:28:
network_state_handler_->SetTetherNetworkListSorter(this);
We shouldn't do this here. Have whatever code creates / destroys this class also
make it the NSH sorter. Also that way network_state_handler_ doesn't need to be
a member and this class doesn't need to depend on it.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer_impl.h (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer_impl.h:55:
std::unique_ptr<ManagedState>& tether_network_state) const;
All of the above is a lot of detail for a header (even for an Impl). Would it be
reasonable to create a NetworkSorter template class (defined entirely in the .cc
file) instead?

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:432: if (limit == 0 || count < limit)
{
Instead of indenting this, modify the test in lines 456/457 and move it to the
top of the for loop.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:482: }
Can we move this logic to a separate helper?

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.h:74: class TetherNetworkListSorter {
This should probably have Delegate in the name, maybe just TetherSortDelegate?

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.h:282: const TetherNetworkListSorter*
tether_network_list_sorter);
I wouldn't bother to check the tether technology state here, that really just
potentially creates initialization order issues which shouldn't matter (e.g. if
we set this delegate before initializing the technology; we should handle that
in the Sorter implementation instead, if at all).

Then we can just make this an in line set_tether_... method.

[Kyle Horimoto, 2017-06-21 21:27:16]

Description was changed from

==========
[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick
settings) are sorted in the same order that the Tether component prioritizes
them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful
ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful
TetherAvailabilityResponse are next in the order, as long as they do not violate
the first rule.
(3) All other devices are left in the order they are passed.

This CL also updates the GetNetworkListByType() logic in NetworkStateHandler;
after this change, all connected/connecting networks are listed before
non-active networks, and Wi-Fi networks associated with Tether networks are no
longer returned since they are supposed to be obscured behind their associated
Tether networks.

BUG=672263,729796
==========

to

==========
[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick
settings) are sorted in the same order that the Tether component prioritizes
them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful
ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful
TetherAvailabilityResponse are next in the order, as long as they do not violate
the first rule.
(3) All other devices are left in the order they are passed.

This CL also updates the GetNetworkListByType() logic in NetworkStateHandler;
after this change, all connected/connecting networks are listed before
non-active networks, and Wi-Fi networks associated with Tether networks are no
longer returned since they are supposed to be obscured behind their associated
Tether networks.

BUG=672263,729796,729801
==========

[Kyle Horimoto, 2017-06-21 22:17:07]

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer_impl.cc (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer_impl.cc:28:
network_state_handler_->SetTetherNetworkListSorter(this);
On 2017/06/21 20:59:03, stevenjb wrote:
> We shouldn't do this here. Have whatever code creates / destroys this class
also
> make it the NSH sorter. Also that way network_state_handler_ doesn't need to
be
> a member and this class doesn't need to depend on it.

Done.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer_impl.h (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer_impl.h:55:
std::unique_ptr<ManagedState>& tether_network_state) const;
On 2017/06/21 20:59:03, stevenjb wrote:
> All of the above is a lot of detail for a header (even for an Impl). Would it
be
> reasonable to create a NetworkSorter template class (defined entirely in the
.cc
> file) instead?

I moved both Move() implementations to an anonymous namespace in the .cc. I left
the GetDeviceId() implementations since they require instance fields.

I'm not exactly sure what you meant about the NetworkSorter template class, but
I think this is sufficient cleanup of the .h file.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:432: if (limit == 0 || count < limit)
{
On 2017/06/21 20:59:04, stevenjb wrote:
> Instead of indenting this, modify the test in lines 456/457 and move it to the
> top of the for loop.

Done.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:482: }
On 2017/06/21 20:59:04, stevenjb wrote:
> Can we move this logic to a separate helper?

Done.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.h:74: class TetherNetworkListSorter {
On 2017/06/21 20:59:04, stevenjb wrote:
> This should probably have Delegate in the name, maybe just TetherSortDelegate?

Done.

https://codereview.chromium.org/2945643002/diff/80001/chromeos/network/networ...
chromeos/network/network_state_handler.h:282: const TetherNetworkListSorter*
tether_network_list_sorter);
On 2017/06/21 20:59:04, stevenjb wrote:
> I wouldn't bother to check the tether technology state here, that really just
> potentially creates initialization order issues which shouldn't matter (e.g.
if
> we set this delegate before initializing the technology; we should handle that
> in the Sorter implementation instead, if at all).
> 
> Then we can just make this an in line set_tether_... method.
> 

Done.

[stevenjb, 2017-06-22 00:25:20]

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer_impl.h (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer_impl.h:55:
std::unique_ptr<ManagedState>& tether_network_state) const;
On 2017/06/21 22:17:07, Kyle Horimoto wrote:
> On 2017/06/21 20:59:03, stevenjb wrote:
> > All of the above is a lot of detail for a header (even for an Impl). Would
it
> be
> > reasonable to create a NetworkSorter template class (defined entirely in the
> .cc
> > file) instead?
> 
> I moved both Move() implementations to an anonymous namespace in the .cc. I
left
> the GetDeviceId() implementations since they require instance fields.
> 
> I'm not exactly sure what you meant about the NetworkSorter template class,
but
> I think this is sufficient cleanup of the .h file.

OK, I looked at this a little more closely and see what's going on, kind of...
it's a little odd using the same sorting method for two very different lists,
but as long as it is doing what it is supposed to, it seems fine.

If GetDeviceId(ManagedState*) didn't require the
device_id_tether_network_guid_map_ lookup (e.g. if it could get the device id
from the ManagedState), I would suggest making the GetDeviceId methods file
local, then pass tether_host_response_recorder_ to SortNetworks() which could
then be a file local template function which would be a little simpler, but this
is fine.

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

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72: size_t last_active_network_index =
-1;
Don't use size_t with negative values, it's generally unsigned.

You can use std::numeric_limits<size_t>::max() (include <limits>), but then will
need to change the logic... see below...

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:75: if (network->IsConnectingState()
|| network->IsConnectedState()) {
I started to make some suggestions here, but this is a) confusing and b) not
super efficient (inserting into a vector is slow), see alternate suggestion
below.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:447: GetTetherNetworkList(limit,
list);
So, rather than using the kind of confusing + kind of inefficient function
above, what if we modify GetTetherNetworkList() to take an 'active' boolean that
would return just the active tether networks here, then...

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:453: iter != network_list_.end() &&
(limit == 0 || count < limit); ++iter) {
Now that we've changed the logic, this is probably more clear without |count|,
just use list->size() (which arguably would have been more clear before also).

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:468: }
Move all of the above logic into helper method (e.g.
IncludeNetworkInList(network, configured_only, visible_only)), then...

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:474: }
break if this is not an active network (note: Ethernet will *always* be active)

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:484:
EnsureActiveNetworksInFrontOfList(list);
Append GetTetherNetworkList(limit, list, active=false) here

Then continue iterating network_list and append inactive networks.

So the code will look like:

  GetTetherNetworkList(limit, list, true /* active */);

  for ((auto iter = network_list_.begin();
       iter != network_list_.end() && (limit == 0 || list->size() < limit);
       ++iter) {
    const NetworkState* network = (*iter)->AsNetworkState();
    if (!IncludeNetworkInList(network, configured_only, visible_only)
      continue;
    if (!IsActiveNetwork(network))
      break;
    if (network->type() == shill::kTypeEthernet) {
      // Ethernet networks should always be in front.
      list->insert(list->begin(), network);
    } else {
      list->push_back(network);
    }
  }

  GetTetherNetworkList(limit, list, false /* active */);

  for ((auto iter = network_list_.begin();
       iter != network_list_.end() && (limit == 0 || list->size() < limit);
       ++iter) {
    const NetworkState* network = (*iter)->AsNetworkState();
    if (!IncludeNetworkInList(network, configured_only, visible_only)
      continue;
    list->push_back(network);
  }

The advantage to this approach is that it is immediately clear that the order
is:
Active Tether
Active Shill networks
Inactive Tether
Inactive Shill networks

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

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:73: // component, a delegate is
required to sort the list.
This isn't really necessary/helpful, the name is clear and the method comment is
sufficient.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:77: // the list.
This should be more generic::

// Sorts |tether_networks| according to the Tether component rules.
// |tether_networks| contains only networks of type Tether.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:280: // Sets the sorting delegate for
Tether networks.
No need for a comment for a trivial setter.

[Kyle Horimoto, 2017-06-22 02:08:01]

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer_impl.h (right):

https://codereview.chromium.org/2945643002/diff/80001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer_impl.h:55:
std::unique_ptr<ManagedState>& tether_network_state) const;
On 2017/06/22 00:25:19, stevenjb wrote:
> On 2017/06/21 22:17:07, Kyle Horimoto wrote:
> > On 2017/06/21 20:59:03, stevenjb wrote:
> > > All of the above is a lot of detail for a header (even for an Impl). Would
> it
> > be
> > > reasonable to create a NetworkSorter template class (defined entirely in
the
> > .cc
> > > file) instead?
> > 
> > I moved both Move() implementations to an anonymous namespace in the .cc. I
> left
> > the GetDeviceId() implementations since they require instance fields.
> > 
> > I'm not exactly sure what you meant about the NetworkSorter template class,
> but
> > I think this is sufficient cleanup of the .h file.
> 
> OK, I looked at this a little more closely and see what's going on, kind of...
> it's a little odd using the same sorting method for two very different lists,
> but as long as it is doing what it is supposed to, it seems fine.
> 
> If GetDeviceId(ManagedState*) didn't require the
> device_id_tether_network_guid_map_ lookup (e.g. if it could get the device id
> from the ManagedState), I would suggest making the GetDeviceId methods file
> local, then pass tether_host_response_recorder_ to SortNetworks() which could
> then be a file local template function which would be a little simpler, but
this
> is fine.

Acknowledged.

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

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72: size_t last_active_network_index =
-1;
On 2017/06/22 00:25:19, stevenjb wrote:
> Don't use size_t with negative values, it's generally unsigned.
> 
> You can use std::numeric_limits<size_t>::max() (include <limits>), but then
will
> need to change the logic... see below...

Acknowledged.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:75: if (network->IsConnectingState()
|| network->IsConnectedState()) {
On 2017/06/22 00:25:19, stevenjb wrote:
> I started to make some suggestions here, but this is a) confusing and b) not
> super efficient (inserting into a vector is slow), see alternate suggestion
> below.

Acknowledged.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:447: GetTetherNetworkList(limit,
list);
On 2017/06/22 00:25:19, stevenjb wrote:
> So, rather than using the kind of confusing + kind of inefficient function
> above, what if we modify GetTetherNetworkList() to take an 'active' boolean
that
> would return just the active tether networks here, then...

Done.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:453: iter != network_list_.end() &&
(limit == 0 || count < limit); ++iter) {
On 2017/06/22 00:25:19, stevenjb wrote:
> Now that we've changed the logic, this is probably more clear without |count|,
> just use list->size() (which arguably would have been more clear before also).

Done.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:468: }
On 2017/06/22 00:25:19, stevenjb wrote:
> Move all of the above logic into helper method (e.g.
> IncludeNetworkInList(network, configured_only, visible_only)), then...

Done.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:474: }
On 2017/06/22 00:25:19, stevenjb wrote:
> break if this is not an active network (note: Ethernet will *always* be
active)

Done.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:484:
EnsureActiveNetworksInFrontOfList(list);
On 2017/06/22 00:25:19, stevenjb wrote:
> Append GetTetherNetworkList(limit, list, active=false) here
> 
> Then continue iterating network_list and append inactive networks.
> 
> So the code will look like:
> 
>   GetTetherNetworkList(limit, list, true /* active */);
> 
>   for ((auto iter = network_list_.begin();
>        iter != network_list_.end() && (limit == 0 || list->size() < limit);
>        ++iter) {
>     const NetworkState* network = (*iter)->AsNetworkState();
>     if (!IncludeNetworkInList(network, configured_only, visible_only)
>       continue;
>     if (!IsActiveNetwork(network))
>       break;
>     if (network->type() == shill::kTypeEthernet) {
>       // Ethernet networks should always be in front.
>       list->insert(list->begin(), network);
>     } else {
>       list->push_back(network);
>     }
>   }
> 
>   GetTetherNetworkList(limit, list, false /* active */);
> 
>   for ((auto iter = network_list_.begin();
>        iter != network_list_.end() && (limit == 0 || list->size() < limit);
>        ++iter) {
>     const NetworkState* network = (*iter)->AsNetworkState();
>     if (!IncludeNetworkInList(network, configured_only, visible_only)
>       continue;
>     list->push_back(network);
>   }
> 
> The advantage to this approach is that it is immediately clear that the order
> is:
> Active Tether
> Active Shill networks
> Inactive Tether
> Inactive Shill networks

Done.

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

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:73: // component, a delegate is
required to sort the list.
On 2017/06/22 00:25:20, stevenjb wrote:
> This isn't really necessary/helpful, the name is clear and the method comment
is
> sufficient.

Done.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:77: // the list.
On 2017/06/22 00:25:20, stevenjb wrote:
> This should be more generic::
> 
> // Sorts |tether_networks| according to the Tether component rules.
> // |tether_networks| contains only networks of type Tether.
> 
> 

Done.

https://codereview.chromium.org/2945643002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.h:280: // Sets the sorting delegate for
Tether networks.
On 2017/06/22 00:25:20, stevenjb wrote:
> No need for a comment for a trivial setter.

Done.

[stevenjb, 2017-06-22 16:44:38]

Thanks, I think this is a lot cleaner than what we had before. lgtm w/ nits.

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

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:73: bool active) {
nit: get_active might be more clear?

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:82: if (active != is_network_active)
nit: Inverted order is a little more clear IMHO

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:457: iter != network_list_.end() &&
(limit == 0 || list->size() < limit);
nit: (Sorry, just thought of this), since we're doing this same compare several
times now, it might be simpler to add:
if (limit == 0)
  limit = std::numeric_limits<size_t>::max();

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:495: void
NetworkStateHandler::GetTetherNetworkList(bool active,
nit: We should probably rename this to something like AppendTetherNetworksToList
now.

[Kyle Horimoto, 2017-06-22 17:27:22]

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

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:73: bool active) {
On 2017/06/22 16:44:38, stevenjb wrote:
> nit: get_active might be more clear?

Done.

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:82: if (active != is_network_active)
On 2017/06/22 16:44:38, stevenjb wrote:
> nit: Inverted order is a little more clear IMHO

Done.

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:457: iter != network_list_.end() &&
(limit == 0 || list->size() < limit);
On 2017/06/22 16:44:38, stevenjb wrote:
> nit: (Sorry, just thought of this), since we're doing this same compare
several
> times now, it might be simpler to add:
> if (limit == 0)
>   limit = std::numeric_limits<size_t>::max();

Done.

https://codereview.chromium.org/2945643002/diff/120001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:495: void
NetworkStateHandler::GetTetherNetworkList(bool active,
On 2017/06/22 16:44:38, stevenjb wrote:
> nit: We should probably rename this to something like
AppendTetherNetworksToList
> now.

Done.

[Kyle Horimoto, 2017-06-22 17:27:31]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-06-22 17:27:31]

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

[commit-bot: I haz the power, 2017-06-22 17:27:58]

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-06-22 19:17:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498152451220260,
"parent_rev": "acdd2a4349f0316bcc5f6c09a5fce64d92e82aaa", "commit_rev":
"5a0eba4b0089554977fde98d3a30d3ca74a750a0"}

[commit-bot: I haz the power, 2017-06-22 19:17:18]

Description was changed from

==========
[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick
settings) are sorted in the same order that the Tether component prioritizes
them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful
ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful
TetherAvailabilityResponse are next in the order, as long as they do not violate
the first rule.
(3) All other devices are left in the order they are passed.

This CL also updates the GetNetworkListByType() logic in NetworkStateHandler;
after this change, all connected/connecting networks are listed before
non-active networks, and Wi-Fi networks associated with Tether networks are no
longer returned since they are supposed to be obscured behind their associated
Tether networks.

BUG=672263,729796,729801
==========

to

==========
[CrOS Tether] Sort Tether network lists.

With this change, Tether networks in the networking UI (i.e., settings and quick
settings) are sorted in the same order that the Tether component prioritizes
them when performing a host scan.

The sorting rules are as follows:
(1) The device which has most recently sent a successful
ConnectTetheringResponse is always at the front of the queue.
(2) Devices which have most recently sent a successful
TetherAvailabilityResponse are next in the order, as long as they do not violate
the first rule.
(3) All other devices are left in the order they are passed.

This CL also updates the GetNetworkListByType() logic in NetworkStateHandler;
after this change, all connected/connecting networks are listed before
non-active networks, and Wi-Fi networks associated with Tether networks are no
longer returned since they are supposed to be obscured behind their associated
Tether networks.

BUG=672263,729796,729801

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

[commit-bot: I haz the power, 2017-06-22 19:17:21]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5a0eba4b0089554977fde98d3a30...