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..e52c59357e6bc0cffdc86e13ce0351c1cc926a27 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/format_macros.h"
+#include "base/json/json_writer.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
@@ -20,8 +21,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 +49,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 +56,13 @@ bool ConnectionStateChanged(chromeos::NetworkState* network,
           !prev_connection_state.empty());
 }
 
+std::string GetManagedStateLogName(const chromeos::ManagedState* state) {
+  if (!state)
+    return "None";
+  return base::StringPrintf("%s (%s)", state->name().c_str(),
+                            state->path().c_str());
+}
+
 }  // namespace
 
 namespace chromeos {
@@ -160,12 +145,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 +277,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 +289,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 +298,11 @@ void NetworkStateHandler::ConnectToBestWifiNetwork() {
 void NetworkStateHandler::SetConnectingNetwork(
     const std::string& service_path) {
   connecting_network_ = service_path;
-  network_event_log::AddEntry(
-      kLogModule, "SetConnectingNetwork", service_path);
+  const NetworkState* network = GetNetworkState(service_path);
+  if (network)
+    NET_LOG_EVENT("SetConnectingNetwork", GetManagedStateLogName(network));
+  else
+    NET_LOG_ERROR("SetConnectingNetwork to unknown network", service_path);
 }
 
 void NetworkStateHandler::GetNetworkStatePropertiesForTest(
@@ -375,8 +363,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 +394,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", GetManagedStateLogName(managed));
   // Notify observers.
   if (network_property_updated) {
     NetworkState* network = managed->AsNetworkState();
@@ -443,10 +428,15 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
       OnDefaultNetworkChanged();
     }
     std::string detail = network->name() + "." + key;
-    std::string vstr = ValueAsString(value);
+    std::string vstr;
+    base::JSONWriter::Write(&value, &vstr);
     if (!vstr.empty())

[pneubeck (no reviews), 2013/05/16 08:21:07]

write should never fail. empty should be a valid value, hiding it may lead to
confusion.
IIRC, write accepts an argument to use quotes for strings, which might be handy
here.

[stevenjb, 2013/05/16 17:09:53]

It could be an empty string, and I did not see that option (except 'pretty'
which could be multi-line).
I added '' for empty strings and wrapped this back in a utility function, but
using JSONWriter.

       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 +451,11 @@ void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
     return;
 
   std::string detail = device->name() + "." + key;
-  std::string vstr = ValueAsString(value);
+  std::string vstr;
+  base::JSONWriter::Write(&value, &vstr);
   if (!vstr.empty())

[pneubeck (no reviews), 2013/05/16 08:21:07]

same here.

     detail += " = " + vstr;
-  network_event_log::AddEntry(kLogModule, "DevicePropertyUpdated", detail);
+  NET_LOG_EVENT("DevicePropertyUpdated", detail);
 
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DeviceListChanged());
@@ -473,8 +464,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 +474,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 +538,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 +560,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 +576,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];