Re-factor network_event_log

chromeos/network/network_state_handler.cc

Index: chromeos/network/network_state_handler.cc
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index a1909a4f485fa7273aa77bafae0e973ea2af98d8..5fa609f8b1f753583e1a6ee35a633a2335eac9cf 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -20,8 +20,6 @@
 
 namespace {
 
-const char kLogModule[] = "NetworkStateHandler";
-
 // Returns true if |network->type()| == |match_type|, or it matches one of the
 // following special match types:
 // * kMatchTypeDefault matches any network (i.e. the first instance)
@@ -50,27 +48,6 @@ bool ManagedStateMatchesType(const chromeos::ManagedState* managed,
   return false;
 }
 
-std::string ValueAsString(const base::Value& value) {
-  if (value.GetType() == base::Value::TYPE_BOOLEAN) {
-    bool bval = false;
-    value.GetAsBoolean(&bval);
-    return bval ? "true" : "false";
-  } else if (value.GetType() == base::Value::TYPE_INTEGER) {
-    int intval = 0;
-    value.GetAsInteger(&intval);
-    return base::StringPrintf("%d", intval);
-  } else if (value.GetType() == base::Value::TYPE_DOUBLE) {
-    double dval = 0;
-    value.GetAsDouble(&dval);
-    return base::StringPrintf("%g", dval);
-  } else if (value.GetType() == base::Value::TYPE_STRING) {
-    std::string vstr;
-    value.GetAsString(&vstr);
-    return vstr;
-  }
-  return "";
-}
-
 bool ConnectionStateChanged(chromeos::NetworkState* network,
                             const std::string& prev_connection_state) {
   return (network->connection_state() != prev_connection_state) &&
@@ -78,6 +55,13 @@ bool ConnectionStateChanged(chromeos::NetworkState* network,
           !prev_connection_state.empty());
 }
 
+std::string GetManagedStateLogName(const chromeos::ManagedState* network) {

[pneubeck (no reviews), 2013/05/13 13:16:25]

either the function name and argument type are wrong or the argument-name.

rename 'network' -> 'state'

[stevenjb, 2013/05/13 19:36:39]

Done.

+  if (!network)
+    return "None";
+  return base::StringPrintf("%s (%s)", network->name().c_str(),
+                            network->path().c_str());
+}
+
 }  // namespace
 
 namespace chromeos {
@@ -160,12 +144,12 @@ void NetworkStateHandler::SetTechnologyEnabled(
     bool enabled,
     const network_handler::ErrorCallback& error_callback) {
   std::string technology = GetTechnologyForType(type);
-  network_event_log::AddEntry(
-      kLogModule, "SetTechnologyEnabled",
-      base::StringPrintf("%s:%d", technology.c_str(), enabled));
+  NET_LOG_EVENT("SetTechnologyEnabled",
+                base::StringPrintf("%s:%d", technology.c_str(), enabled));
   shill_property_handler_->SetTechnologyEnabled(
       technology, enabled, error_callback);
-  ManagerPropertyChanged();  // Technology state changed -> ENABLING
+  // Signal Technology state changed -> ENABLING
+  NotifyManagerPropertyChanged();
 }
 
 const DeviceState* NetworkStateHandler::GetDeviceState(
@@ -292,7 +276,7 @@ void NetworkStateHandler::GetNetworkList(NetworkStateList* list) const {
 }
 
 void NetworkStateHandler::RequestScan() const {
-  network_event_log::AddEntry(kLogModule, "RequestScan", "");
+  NET_LOG_EVENT("RequestScan", "");
   shill_property_handler_->RequestScan();
 }
 
@@ -304,7 +288,7 @@ void NetworkStateHandler::WaitForScan(const std::string& type,
 }
 
 void NetworkStateHandler::ConnectToBestWifiNetwork() {
-  network_event_log::AddEntry(kLogModule, "ConnectToBestWifiNetwork", "");
+  NET_LOG_EVENT("ConnectToBestWifiNetwork", "");
   WaitForScan(flimflam::kTypeWifi,
               base::Bind(&internal::ShillPropertyHandler::ConnectToBestServices,
                          shill_property_handler_->AsWeakPtr()));
@@ -313,8 +297,7 @@ void NetworkStateHandler::ConnectToBestWifiNetwork() {
 void NetworkStateHandler::SetConnectingNetwork(
     const std::string& service_path) {
   connecting_network_ = service_path;
-  network_event_log::AddEntry(
-      kLogModule, "SetConnectingNetwork", service_path);
+  NET_LOG_EVENT("SetConnectingNetwork", GetNameFromPath(service_path));

[pneubeck (no reviews), 2013/05/13 13:16:25]

Does GetNameFromPath really have to exposed in the header? This is the only
place where it's used (after fixing my other comment)? Do we need it at all?

If so, restrict it at least to Networks, so that the ambiguity of service paths
("is it a network or a device?") is gone.

[stevenjb, 2013/05/13 19:36:39]

Eliminated GetNameFromPath

 }
 
 void NetworkStateHandler::GetNetworkStatePropertiesForTest(
@@ -375,8 +358,7 @@ void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
 }
 
 void NetworkStateHandler::ProfileListChanged() {
-  network_event_log::AddEntry(
-      kLogModule, "ProfileListChanged", "Re-Requesting Network Properties");
+  NET_LOG_EVENT("ProfileListChanged", "Re-Requesting Network Properties");
   for (ManagedStateList::iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     shill_property_handler_->RequestProperties(
@@ -407,9 +389,7 @@ void NetworkStateHandler::UpdateManagedStateProperties(
     }
   }
   managed->InitialPropertiesReceived();
-  network_event_log::AddEntry(
-      kLogModule, "PropertiesReceived",
-      base::StringPrintf("%s (%s)", path.c_str(), managed->name().c_str()));
+  NET_LOG_DEBUG("PropertiesReceived", GetNameFromPath(path));

[pneubeck (no reviews), 2013/05/13 13:16:25]

should use GetManagedStateLogName(managed) instead.

[stevenjb, 2013/05/13 19:36:39]

Done.

   // Notify observers.
   if (network_property_updated) {
     NetworkState* network = managed->AsNetworkState();
@@ -443,10 +423,14 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
       OnDefaultNetworkChanged();
     }
     std::string detail = network->name() + "." + key;
-    std::string vstr = ValueAsString(value);
+    std::string vstr = network_event_log::DBusValueAsString(value);
     if (!vstr.empty())
       detail += " = " + vstr;
-    network_event_log::AddEntry(kLogModule, "NetworkPropertyUpdated", detail);
+    network_event_log::LogLevel log_level =
+        (key == flimflam::kSignalStrengthProperty)
+        ? network_event_log::LOG_LEVEL_DEBUG
+        : network_event_log::LOG_LEVEL_EVENT;
+    NET_LOG_LEVEL(log_level, "NetworkPropertyUpdated", detail);
   }
   NetworkPropertiesUpdated(network);
 }
@@ -461,10 +445,10 @@ void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
     return;
 
   std::string detail = device->name() + "." + key;
-  std::string vstr = ValueAsString(value);
+  std::string vstr = network_event_log::DBusValueAsString(value);
   if (!vstr.empty())
     detail += " = " + vstr;
-  network_event_log::AddEntry(kLogModule, "DevicePropertyUpdated", detail);
+  NET_LOG_EVENT("DevicePropertyUpdated", detail);
 
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DeviceListChanged());
@@ -473,8 +457,8 @@ void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
     ScanCompleted(device->type());
 }
 
-void NetworkStateHandler::ManagerPropertyChanged() {
-  network_event_log::AddEntry(kLogModule, "NetworkManagerChanged", "");
+void NetworkStateHandler::NotifyManagerPropertyChanged() {
+  NET_LOG_DEBUG("NotifyManagerPropertyChanged", "");
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkManagerChanged());
 }
@@ -483,18 +467,16 @@ void NetworkStateHandler::ManagedStateListChanged(
     ManagedState::ManagedType type) {
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     // Notify observers that the list of networks has changed.
-    network_event_log::AddEntry(
-        kLogModule, "NetworkListChanged",
-        base::StringPrintf("Size:%"PRIuS, network_list_.size()));
+    NET_LOG_EVENT("NetworkListChanged",
+                  base::StringPrintf("Size:%"PRIuS, network_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       NetworkListChanged());
     // The list order may have changed, so check if the default network changed.
     if (CheckDefaultNetworkChanged())
       OnDefaultNetworkChanged();
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
-    network_event_log::AddEntry(
-        kLogModule, "DeviceListChanged",
-        base::StringPrintf("Size:%"PRIuS, device_list_.size()));
+    NET_LOG_DEBUG("DeviceListChanged",
+                  base::StringPrintf("Size:%"PRIuS, device_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       DeviceListChanged());
   } else {
@@ -549,10 +531,9 @@ NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
 void NetworkStateHandler::OnNetworkConnectionStateChanged(
     NetworkState* network) {
   DCHECK(network);
-  network_event_log::AddEntry(
-      kLogModule, "NetworkConnectionStateChanged",
-      base::StringPrintf("%s:%s", network->path().c_str(),
-                         network->connection_state().c_str()));
+  NET_LOG_EVENT("NetworkConnectionStateChanged", base::StringPrintf(
+      "%s:%s", GetManagedStateLogName(network).c_str(),
+      network->connection_state().c_str()));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkConnectionStateChanged(network));
   if (CheckDefaultNetworkChanged() || network->path() == default_network_path_)
@@ -572,9 +553,8 @@ bool NetworkStateHandler::CheckDefaultNetworkChanged() {
 
 void NetworkStateHandler::OnDefaultNetworkChanged() {
   const NetworkState* default_network = DefaultNetwork();
-  network_event_log::AddEntry(
-      kLogModule, "DefaultNetworkChanged",
-      default_network ? default_network->path() : "None");
+  NET_LOG_EVENT("DefaultNetworkChanged",
+                GetManagedStateLogName(default_network));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DefaultNetworkChanged(default_network));
 }
@@ -589,18 +569,16 @@ void NetworkStateHandler::NetworkPropertiesUpdated(
       !network->IsConnectingState() &&
       network->connection_state() != flimflam::kStateIdle) {
     connecting_network_.clear();
-    network_event_log::AddEntry(
-        kLogModule, "ClearConnectingNetwork",
-        base::StringPrintf("%s:%s", network->path().c_str(),
-                           network->connection_state().c_str()));
+    NET_LOG_EVENT("ClearConnectingNetwork", base::StringPrintf(
+        "%s:%s", GetManagedStateLogName(network).c_str(),
+        network->connection_state().c_str()));
   }
 }
 
 void NetworkStateHandler::ScanCompleted(const std::string& type) {
   size_t num_callbacks = scan_complete_callbacks_.count(type);
-  network_event_log::AddEntry(
-      kLogModule, "ScanCompleted",
-      base::StringPrintf("%s:%"PRIuS, type.c_str(), num_callbacks));
+  NET_LOG_EVENT("ScanCompleted",
+                base::StringPrintf("%s:%"PRIuS, type.c_str(), num_callbacks));
   if (num_callbacks == 0)
     return;
   ScanCallbackList& callback_list = scan_complete_callbacks_[type];
@@ -627,4 +605,14 @@ std::string NetworkStateHandler::GetTechnologyForType(
   return type;
 }
 
+std::string NetworkStateHandler::GetNameFromPath(
+    const std::string& path) const {

[pneubeck (no reviews), 2013/05/13 13:16:25]

after fixing my other comment, this is only used for networks. no need to guess.

[stevenjb, 2013/05/13 19:36:39]

Eliminated GetNameFromPath

+  ManagedState* managed = GetModifiableManagedState(&network_list_, path);
+  if (!managed)
+    managed = GetModifiableManagedState(&device_list_, path);
+  if (managed)
+    return GetManagedStateLogName(managed);
+  return path;
+}
+
 }  // namespace chromeos