Fix watch list logic for ShillPropertyHandler

Also adds verbose(2) logging to network_event_log

BUG=none


Review URL: https://chromiumcodereview.appspot.com/11280199

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@169942 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 27 insertions, 21 deletions

diff --git a/chromeos/network/network_event_log.cc b/chromeos/network/network_event_log.cc
index a7d7147..bf1669d 100644
--- a/chromeos/network/network_event_log.cc
+++ b/chromeos/network/network_event_log.cc
@@ -88,7 +88,9 @@ void NetworkEventLog::AddEntry(const std::string& module,
   }
   if (entries_.size() >= kMaxNetworkEventLogEntries)
     entries_.pop_front();
-  entries_.push_back(LogEntry(module, event, description));
+  LogEntry entry(module, event, description);
+  entries_.push_back(entry);
+  VLOG(2) << entry.ToString();
 }
 
 std::string NetworkEventLog::GetAsString(StringOrder order,
diff --git a/chromeos/network/shill_property_handler.cc b/chromeos/network/shill_property_handler.cc
index e905089..7781367 100644
--- a/chromeos/network/shill_property_handler.cc
+++ b/chromeos/network/shill_property_handler.cc
@@ -93,7 +93,10 @@ void ShillPropertyHandler::RequestScan() const {
 
 void ShillPropertyHandler::RequestProperties(ManagedState::ManagedType type,
                                              const std::string& path) {
-  ++pending_updates_[type];
+  if (pending_updates_[type].find(path) != pending_updates_[type].end())
+    return;  // Update already requested.
+
+  pending_updates_[type].insert(path);
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
         dbus::ObjectPath(path),
@@ -155,9 +158,8 @@ bool ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
       UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
   } else if (key == flimflam::kServiceWatchListProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist) {
+    if (vlist)
       UpdateObservedNetworkServices(*vlist);
-    }
   } else if (key == flimflam::kDevicesProperty) {
     const ListValue* vlist = GetListValue(key, value);
     if (vlist)
@@ -186,7 +188,7 @@ void ShillPropertyHandler::UpdateManagedList(ManagedState::ManagedType type,
   // called when the update requests have completed). If no requests
   // have been made, call ManagedStateListChanged to indicate that the
   // order of the list has changed.
-  if (pending_updates_[type] == 0)
+  if (pending_updates_[type].size() == 0)
     listener_->ManagedStateListChanged(type);
 }
 
@@ -204,22 +206,20 @@ void ShillPropertyHandler::UpdateObservedNetworkServices(
     if (iter2 != observed_networks_.end()) {
       new_observed[path] = iter2->second;
     } else {
+      // Request an update for the network.
+      RequestProperties(ManagedState::MANAGED_TYPE_NETWORK, path);
+      // Create an observer for future updates.
       new_observed[path] = new ShillServiceObserver(
           path, base::Bind(
               &ShillPropertyHandler::NetworkServicePropertyChangedCallback,
               weak_ptr_factory_.GetWeakPtr()));
+      network_event_log::AddEntry(kLogModule, "StartObserving", path);
     }
     observed_networks_.erase(path);
     // Limit the number of observed services.
     if (new_observed.size() >= kMaxObservedServices)
       break;
   }
-  network_event_log::AddEntry(
-      kLogModule, "ObservedListChanged",
-      StringPrintf("Entries: %"PRIuS " New: %"PRIuS,
-                   entries.GetSize(), new_observed.size()));
-  VLOG(2) << "UpdateObservedNetworkServices, new observed: "
-          << new_observed.size();
   // Delete network service observers still in observed_networks_.
   for (ShillServiceObserverMap::iterator iter =  observed_networks_.begin();
        iter != observed_networks_.end(); ++iter) {
@@ -235,7 +235,7 @@ void ShillPropertyHandler::GetPropertiesCallback(
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& properties) {
   VLOG(2) << "GetPropertiesCallback: " << type << " : " << path;
-  --pending_updates_[type];
+  pending_updates_[type].erase(path);
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
     LOG(ERROR) << "Failed to get properties for: " << path
                << ": " << call_status;
@@ -243,7 +243,7 @@ void ShillPropertyHandler::GetPropertiesCallback(
   }
   listener_->UpdateManagedStateProperties(type, path, properties);
   // Notify the listener only when all updates for that type have completed.
-  if (pending_updates_[type] == 0)
+  if (pending_updates_[type].size() == 0)
     listener_->ManagedStateListChanged(type);
 }
 
diff --git a/chromeos/network/shill_property_handler.h b/chromeos/network/shill_property_handler.h
index 63d2337..6b10f79 100644
--- a/chromeos/network/shill_property_handler.h
+++ b/chromeos/network/shill_property_handler.h
@@ -158,8 +158,8 @@ class CHROMEOS_EXPORT ShillPropertyHandler
   // Convenience pointer for ShillManagerClient
   ShillManagerClient* shill_manager_;
 
-  // Pending update count for each managed state type
-  std::map<ManagedState::ManagedType, int> pending_updates_;
+  // Pending update list for each managed state type
+  std::map<ManagedState::ManagedType, std::set<std::string> > pending_updates_;
 
   // List of network services with Shill property changed observers
   ShillServiceObserverMap observed_networks_;
diff --git a/chromeos/network/shill_property_handler_unittest.cc b/chromeos/network/shill_property_handler_unittest.cc
index f9954d6..def23f9 100644
--- a/chromeos/network/shill_property_handler_unittest.cc
+++ b/chromeos/network/shill_property_handler_unittest.cc
@@ -337,11 +337,15 @@ TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerServicePropertyChanged) {
   AddService(flimflam::kTypeWifi, kTestServicePath,
              flimflam::kStateIdle, true);
   message_loop_.RunUntilIdle();
-  // No new updates or services:
-  EXPECT_EQ(1, listener_->manager_updates());
-  EXPECT_EQ(2, listener_->list_updates(flimflam::kServicesProperty));
+  // Service list update should be received when watch list changes.
+  EXPECT_EQ(3, listener_->list_updates(flimflam::kServicesProperty));
+  // Number of services shouldn't change.
   EXPECT_EQ(kNumShillManagerClientStubImplServices + 1,
             listener_->entries(flimflam::kServicesProperty).size());
+  // Property update should be received when watched service is added.
+  EXPECT_EQ(1, listener_->
+            property_updates(flimflam::kServicesProperty)[kTestServicePath]);
+
   // Change a property.
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
       dbus::ObjectPath(kTestServicePath),
@@ -349,14 +353,14 @@ TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerServicePropertyChanged) {
       scan_interval,
       base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
   message_loop_.RunUntilIdle();
-  // Property change SHOULD trigger an update.
-  EXPECT_EQ(1, listener_->
+  // Property change should trigger another update.
+  EXPECT_EQ(2, listener_->
             property_updates(flimflam::kServicesProperty)[kTestServicePath]);
 
   // Remove a service
   RemoveService(kTestServicePath);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(3, listener_->list_updates(flimflam::kServicesProperty));
+  EXPECT_EQ(4, listener_->list_updates(flimflam::kServicesProperty));
   EXPECT_EQ(kNumShillManagerClientStubImplServices,
             listener_->entries(flimflam::kServicesProperty).size());