Observe property updates for all Network Services

chromeos/network/shill_property_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/shill_property_handler.h"
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
@@ skipping 187 matching lines @@
                    AsWeakPtr(), type, path));
   } else {
     NOTREACHED();
   }
 }
 
 void ShillPropertyHandler::OnPropertyChanged(const std::string& key,
                                              const base::Value& value) {
   if (ManagerPropertyChanged(key, value))
     listener_->ManagerPropertyChanged();
-  // If the service watch or device list changed and there are no pending
+  // If the service or device list changed and there are no pending
   // updates, signal the state list changed callback.
-  if ((key == flimflam::kServiceWatchListProperty) &&
+  if ((key == flimflam::kServicesProperty) &&
       pending_updates_[ManagedState::MANAGED_TYPE_NETWORK].size() == 0) {
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_NETWORK);
   }
   if (key == flimflam::kDevicesProperty &&
       pending_updates_[ManagedState::MANAGED_TYPE_DEVICE].size() == 0) {
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_DEVICE);
   }
 }
 
 //------------------------------------------------------------------------------
@@ skipping 30 matching lines @@
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_NETWORK);
   if (pending_updates_[ManagedState::MANAGED_TYPE_DEVICE].size() == 0)
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_DEVICE);
 }
 
 bool ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
                                                   const base::Value& value) {
   bool notify_manager_changed = false;
   if (key == flimflam::kServicesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist)
+    if (vlist) {
       listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
-  } else if (key == flimflam::kServiceWatchListProperty) {
-    const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist) {
+      // UpdateObserved used to use kServiceWatchListProperty for TYPE_NETWORK,

[pneubeck (no reviews), 2013/05/13 10:21:03]

shouldn't this be a method comment?

[stevenjb, 2013/05/13 21:31:40]

No, it's really an implementation detail. I'd rather have it here.

+      // however that prevents us from receiving Strength updates from inactive
+      // networks. The overhead for observing all services is not unreasonable
+      // (and we limit the max number of observed services to kMaxObserved).
       UpdateObserved(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
     }
   } else if (key == flimflam::kDevicesProperty) {
     const ListValue* vlist = GetListValue(key, value);
     if (vlist) {
       listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
       UpdateObserved(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
     }
   } else if (key == flimflam::kAvailableTechnologiesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
@@ skipping 38 matching lines @@
       // Request an update.
       RequestProperties(type, path);
       // Create an observer for future updates.
       new_observed[path] = new ShillPropertyObserver(
           type, path, base::Bind(
               &ShillPropertyHandler::PropertyChangedCallback, AsWeakPtr()));
       network_event_log::AddEntry(kLogModule, "StartObserving", path);
     }
     observer_map.erase(path);
     // Limit the number of observed services.
     if (new_observed.size() >= kMaxObserved)

[pneubeck (no reviews), 2013/05/13 10:21:03]

This is a serious restriction which should be documented at the
NetworkStateHandler class description.

I'm still not sure that this is the right thing to do.
Memory overhead cannot be the argument as Shill also stores these networks (thus
memory usage at most doubles).
The number of updates might already be diminishingly small (only triggered by
scans). If it's still too much work, how does Shill handle it?
Is your concern related to the number of DBus registrations? DBus provides a
solution to that.

On the other hand, we might wonder at some time why we don't see certain
networks in the UI.

[stevenjb, 2013/05/13 21:31:40]

I don't think this is a big of a restriction as you think it is.

We will still list all services and fetch their initial properties.

"Interesting" services such as connecting services will always be listed at the
top, see the comment for kMaxObserverd.

This is just a limit on:
 a) The number of dbus registrations (which, yes we could improve, but that gets
complicated quickly)
 b) The potential number of observer updates, which have non-trivial UI
overhead.

The only practical effect is that we will not see Strength updates for networks
we don't really care much about.

It's not an important limit, we could almost certainly remove it, I would just
prefer to avoid a situation where an edge case with 1000+ visible networks might
cause unrecoverable performance bottlenecks.

This seems like a simpler solution than optimizing for an edge case.

[pneubeck (no reviews), 2013/05/14 14:12:32]

Ok. Here is the conflict we run into with this:

- Configure a network XY.
- On Startup be within close range of XY's AP.
- NetworkStateHandler's initial update will fetch the high signalstrength for
XY.
- Move away from XY's AP => NetworkStateHandler stops observing XY.
- Go to the settings page and modify some property of XY that ends up in the
network state, like IP config.
- NetworkStateHandler will not be notified => XY's NetworkState is stale
- Because it's configured and visible, some UI may expose XY's stale
NetworkState

In general, any configurable property (e.g. security, guid) that is present in
NetworkState can potentially be inconsistent with the last configuration.

Depending on where we use which properties of NetworkState, this may or may not
end up in an actual bug; but at least it's a subtle unexpected behavior. If we
can avoid it now, we should do so.

I see three alternatives:
- Remove the limit
- Always observe configured networks independent of the limit
- Remove all configurable properties from NetworkState (and kNameProperty might
be one of them in the future).

       break;
   }
   // Delete network service observers still in observer_map.
   for (ShillPropertyObserverMap::iterator iter =  observer_map.begin();
        iter != observer_map.end(); ++iter) {
     network_event_log::AddEntry(kLogModule, "StopObserving", iter->first);
     delete iter->second;
   }
   observer_map.swap(new_observed);
 }
@@ skipping 150 matching lines @@
 
 void ShillPropertyHandler::NetworkDevicePropertyChangedCallback(
     const std::string& path,
     const std::string& key,
     const base::Value& value) {
   listener_->UpdateDeviceProperty(path, key, value);
 }
 
 }  // namespace internal
 }  // namespace chromeos