Rely on Service.Visible instead of Manager.Services

Rely on Service.Visible instead of Manager.Services

With this change we rely on Service.Visible and only use
Manager.Services to determine which services to observe.

BUG=384380
R=pneubeck@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278594

[pneubeck (no reviews), 2014-06-16 14:38:16]

https://codereview.chromium.org/330833003/diff/1/chromeos/dbus/fake_shill_man...
File chromeos/dbus/fake_shill_manager_client.h (right):

https://codereview.chromium.org/330833003/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.h:111: void SortServiceList(const
std::string& property);
oops. This function isn't defined (and is unused). Somehow we missed it before.

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

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:225: break;
did the ordering by Shill change in the meanwhile or is this a rebase artifact?
If it was never related to the ordering but to pending updates, this should
indeed be fine.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:655: UpdateNetworkList();
Just noting that networks are added at distributed over several loop iterations.
After these several iterations this is finally called.
In between there can already be access to the network list which wouldn't see
the sorting.

If we start sorting at all, it should be consistent over all accesses?
E.g. on each list access, we could ensure that the list is sorted / return it
sorted.
The list accesses have linear time anyways.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:677: // Sort networks as follows:
Better also mention that it's a stable sort that maintains the order of Shill's
list.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:685: ManagedStateList active,
wifi_visible, wifi_hidden;
is there an expected case, where this reordering actually changes order?

I thought at first that it's needed because of the visible property. But the
moment this is currently called, the visible property should already be
received.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:699: // List visible non-wifi networks
immediately after active networks.
is this the difference to Shill's order?
Note that this relies on Shill still listing the active ones before the
inactive, visible-non-wifi networks.
If you already using several target lists, could you add one more to remove that
dependency? (e.g. ManagedStateList visible_non_wifi)

(you could then remove the negation form the if-condition)

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:727: active.size() +
wifi_visible.size());
optional nit: use a visible counter

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.h:309: // Sort the network list and
update UMA stats.
Maybe either add a comment that this is only ever called if every NetworkState
has received properties before, or handle states with !update_received()
explicitly in the implementation.

[stevenjb, 2014-06-16 19:54:49]

https://codereview.chromium.org/330833003/diff/1/chromeos/dbus/fake_shill_man...
File chromeos/dbus/fake_shill_manager_client.h (right):

https://codereview.chromium.org/330833003/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.h:111: void SortServiceList(const
std::string& property);
On 2014/06/16 14:38:15, pneubeck wrote:
> oops. This function isn't defined (and is unused). Somehow we missed it
before.

Done.

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

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:225: break;
On 2014/06/16 14:38:15, pneubeck wrote:
> did the ordering by Shill change in the meanwhile or is this a rebase
artifact?
> If it was never related to the ordering but to pending updates, this should
> indeed be fine.

This was an ordering issue that is now addressed in SortNetworkList().

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:655: UpdateNetworkList();
On 2014/06/16 14:38:15, pneubeck wrote:
> Just noting that networks are added at distributed over several loop
iterations.
> After these several iterations this is finally called.
> In between there can already be access to the network list which wouldn't see
> the sorting.
> 
> If we start sorting at all, it should be consistent over all accesses?
> E.g. on each list access, we could ensure that the list is sorted / return it
> sorted.
> The list accesses have linear time anyways.

So, I thought about this a bunch, and while in practice I think this is OK, I
agree that it is imperfect and as-is FirstNetwork would at least have to be
changed back.

From a maintainability perspective having an imperfectly sorted list is
problematic though, so I think that the correct thing to do is to sort the list
every time a NetworkState update is received.

(The easiest thing to do would be to fix the sorting in Shill, but I don't like
to impose a sorting there that we mostly use for UI convenience).

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:677: // Sort networks as follows:
On 2014/06/16 14:38:15, pneubeck wrote:
> Better also mention that it's a stable sort that maintains the order of
Shill's
> list.

Done.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:685: ManagedStateList active,
wifi_visible, wifi_hidden;
On 2014/06/16 14:38:15, pneubeck wrote:
> is there an expected case, where this reordering actually changes order?
> 
> I thought at first that it's needed because of the visible property. But the
> moment this is currently called, the visible property should already be
> received.

Shill is sorting by type then by state, type, visibility, so hidden wifi
networks show up before disconnected (but "visible") Cellular networks.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:699: // List visible non-wifi networks
immediately after active networks.
On 2014/06/16 14:38:15, pneubeck wrote:
> is this the difference to Shill's order?
> Note that this relies on Shill still listing the active ones before the
> inactive, visible-non-wifi networks.
> If you already using several target lists, could you add one more to remove
that
> dependency? (e.g. ManagedStateList visible_non_wifi)
> 
> (you could then remove the negation form the if-condition)
Done.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.cc:727: active.size() +
wifi_visible.size());
On 2014/06/16 14:38:15, pneubeck wrote:
> optional nit: use a visible counter

Done.

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/330833003/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler.h:309: // Sort the network list and
update UMA stats.
On 2014/06/16 14:38:15, pneubeck wrote:
> Maybe either add a comment that this is only ever called if every NetworkState
> has received properties before, or handle states with !update_received()
> explicitly in the implementation.

This is called directly by ManagedStateListChanged, which already has a comment
to that effect.

[pneubeck (no reviews), 2014-06-17 15:30:33]

I have slight concerns about the regular sorting of the whole network list.
If you're super confident about it, then lgtm, otherwise I'd like to chat with
you about this at first.

https://codereview.chromium.org/330833003/diff/20001/chromeos/dbus/shill_serv...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/330833003/diff/20001/chromeos/dbus/shill_serv...
chromeos/dbus/shill_service_client.h:60: // or Profile. Returns the properties
for the service.
for clarification, maybe add a sentence:
"Afterwards, the caller is responsible to [or "should"] add the service using
the Manager's AddManagerService method."

https://codereview.chromium.org/330833003/diff/20001/chromeos/network/network...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/330833003/diff/20001/chromeos/network/network...
chromeos/network/network_state_handler.cc:521: SortNetworkList();
Couldn't this lead to a to huge performance impact?
This will be called on every network update. E.g. if the user logs in, we call
this up to a 100 times. Similarly for other triggers like after a network scan
(if several networks disappeared and reappear; doesn't shill regularly forget
wifi services, on purpose?).

Every of these calls, takes linear time in the length of the list (the list can
be longer than 100; but should be safe to assume <1000 in the typical case). And
that on the UI thread.



I have the strong impression, that this can't be solved with a simple solution
"correctly" if we don't have an idea on the frequency of function calls.


Can we chat/VC about this later?

[pneubeck (no reviews), 2014-06-17 15:30:33]

I have slight concerns about the regular sorting of the whole network list.
If you're super confident about it, then lgtm, otherwise I'd like to chat with
you about this at first.

https://codereview.chromium.org/330833003/diff/20001/chromeos/dbus/shill_serv...
File chromeos/dbus/shill_service_client.h (right):

https://codereview.chromium.org/330833003/diff/20001/chromeos/dbus/shill_serv...
chromeos/dbus/shill_service_client.h:60: // or Profile. Returns the properties
for the service.
for clarification, maybe add a sentence:
"Afterwards, the caller is responsible to [or "should"] add the service using
the Manager's AddManagerService method."

https://codereview.chromium.org/330833003/diff/20001/chromeos/network/network...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/330833003/diff/20001/chromeos/network/network...
chromeos/network/network_state_handler.cc:521: SortNetworkList();
Couldn't this lead to a to huge performance impact?
This will be called on every network update. E.g. if the user logs in, we call
this up to a 100 times. Similarly for other triggers like after a network scan
(if several networks disappeared and reappear; doesn't shill regularly forget
wifi services, on purpose?).

Every of these calls, takes linear time in the length of the list (the list can
be longer than 100; but should be safe to assume <1000 in the typical case). And
that on the UI thread.



I have the strong impression, that this can't be solved with a simple solution
"correctly" if we don't have an idea on the frequency of function calls.


Can we chat/VC about this later?

[stevenjb, 2014-06-17 17:14:08]

https://codereview.chromium.org/330833003/diff/20001/chromeos/network/network...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/330833003/diff/20001/chromeos/network/network...
chromeos/network/network_state_handler.cc:521: SortNetworkList();
On 2014/06/17 15:30:33, pneubeck wrote:
> Couldn't this lead to a to huge performance impact?
> This will be called on every network update. E.g. if the user logs in, we call
> this up to a 100 times. Similarly for other triggers like after a network scan
> (if several networks disappeared and reappear; doesn't shill regularly forget
> wifi services, on purpose?).
> 
> Every of these calls, takes linear time in the length of the list (the list
can
> be longer than 100; but should be safe to assume <1000 in the typical case).
And
> that on the UI thread.
> 
> 
> 
> I have the strong impression, that this can't be solved with a simple solution
> "correctly" if we don't have an idea on the frequency of function calls.
> 
> 
> Can we chat/VC about this later?


Yeah, you're right, technically after a scan this could be an N^2 operation.
Even though N is likely to be small in practice, it could lead to noticeable lag
and should be avoided. This is why I didn't do this initially, I'm not sure how
I talked myself into doing it, except that I was distracted by other things
yesterday.

We could sort on demand. I'm not a huge fan of that either, mostly because it
adds complexity from the calling side of things, but if we use a dirty flag we
can at least ensure that sorting doesn't happen often in that case.

Or we could go back to only sorting when all updates have been received and fix
FirstNetworkByType to scan the entire list. In practice there shouldn't be any
visible artifacts so long as the UI only refreshes the list in response to a
NetworkListChanged event.

Feel free to ping me when you're available if you want to chat about this over
VC.

[stevenjb, 2014-06-17 22:02:49]

PTAL

[pneubeck (no reviews), 2014-06-18 09:45:49]

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

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:224: if (!network_list_sorted_)
optional nit: 
you could move network_list_sorted_ into SortNetworkList.

(If you find the function name unfitting, could be renamed then to
EnsureNetworkListSorted)

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:225: SortNetworkList();  // Sort to
ensure visible networks are listed fist.
fist -> first

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:719: hidden.push_back(network);
do networks with !update_received() always end up in these list?
since sort is now called not only after all updates are received, you should
handle this case explicitly (or at least comment it)

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

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.h:161: // called if necessary to prove
the states in the a convenient order (see
prove? or should it be 'provide'?

'the a' -> 'a'

[stevenjb, 2014-06-18 22:33:26]

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

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:224: if (!network_list_sorted_)
On 2014/06/18 09:45:48, pneubeck wrote:
> optional nit: 
> you could move network_list_sorted_ into SortNetworkList.
> 
> (If you find the function name unfitting, could be renamed then to
> EnsureNetworkListSorted)
Yeah, I considered that, but I kind of want to force a sort after a complete
update just in case I missed an edge case, and don't want to add a parameter
'force'. Since we only do this in a couple of places I thought this was more
readable.

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:225: SortNetworkList();  // Sort to
ensure visible networks are listed fist.
On 2014/06/18 09:45:49, pneubeck wrote:
> fist -> first

Done.

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:719: hidden.push_back(network);
On 2014/06/18 09:45:48, pneubeck wrote:
> do networks with !update_received() always end up in these list?
> since sort is now called not only after all updates are received, you should
> handle this case explicitly (or at least comment it)

They would be treated as "hidden", which would be fine, but I went ahead and
handled them explicitly for clarity.

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

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.h:161: // called if necessary to prove
the states in the a convenient order (see
On 2014/06/18 09:45:49, pneubeck wrote:
> prove? or should it be 'provide'?
> 
> 'the a' -> 'a'

Done.

[pneubeck (no reviews), 2014-06-18 22:35:29]

lgtm

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

https://codereview.chromium.org/330833003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:224: if (!network_list_sorted_)
On 2014/06/18 22:33:26, stevenjb wrote:
> On 2014/06/18 09:45:48, pneubeck wrote:
> > optional nit: 
> > you could move network_list_sorted_ into SortNetworkList.
> > 
> > (If you find the function name unfitting, could be renamed then to
> > EnsureNetworkListSorted)
> Yeah, I considered that, but I kind of want to force a sort after a complete
> update just in case I missed an edge case, and don't want to add a parameter
> 'force'. Since we only do this in a couple of places I thought this was more
> readable.
> 

I see. that's fine.

[stevenjb, 2014-06-18 22:36:52]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-18 22:37:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/330833003/100001

[commit-bot: I haz the power, 2014-06-19 07:48:36]

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

[commit-bot: I haz the power, 2014-06-19 07:48:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[stevenjb, 2014-06-20 04:40:44]

Committed patchset #6 manually as r278594 (presubmit successful).