Re-factor network_event_log

chromeos/network/network_state_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/network_state_handler.h"
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/managed_state.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler_observer.h"
 #include "chromeos/network/shill_property_handler.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 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)
 // * kMatchTypeNonVirtual matches non virtual networks
 // * kMatchTypeWireless matches wireless networks
 // * kMatchTypeMobile matches cellular or wimax networks
 bool ManagedStateMatchesType(const chromeos::ManagedState* managed,
                              const std::string& match_type) {
   const std::string& type = managed->type();
   if (match_type == chromeos::NetworkStateHandler::kMatchTypeDefault)
     return true;
   if (match_type == type)
     return true;
   if (match_type == chromeos::NetworkStateHandler::kMatchTypeNonVirtual &&
       type != flimflam::kTypeVPN) {
     return true;
   }
   if (match_type == chromeos::NetworkStateHandler::kMatchTypeWireless &&
       type != flimflam::kTypeEthernet && type != flimflam::kTypeVPN) {
     return true;
   }
   if (match_type == chromeos::NetworkStateHandler::kMatchTypeMobile &&
       (type == flimflam::kTypeCellular || type == flimflam::kTypeWimax)) {
     return true;
   }
   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) &&
          (network->connection_state() != flimflam::kStateIdle ||
           !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 {
 
 const char NetworkStateHandler::kMatchTypeDefault[] = "default";
 const char NetworkStateHandler::kMatchTypeWireless[] = "wireless";
 const char NetworkStateHandler::kMatchTypeMobile[] = "mobile";
 const char NetworkStateHandler::kMatchTypeNonVirtual[] = "non-virtual";
 
 static NetworkStateHandler* g_network_state_handler = NULL;
@@ skipping 62 matching lines @@
     state = TECHNOLOGY_UNAVAILABLE;
   VLOG(2) << "GetTechnologyState: " << type << " = " << state;
   return state;
 }
 
 void NetworkStateHandler::SetTechnologyEnabled(
     const std::string& type,
     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(
     const std::string& device_path) const {
   return GetModifiableDeviceState(device_path);
 }
 
 const DeviceState* NetworkStateHandler::GetDeviceStateByType(
     const std::string& type) const {
   for (ManagedStateList::const_iterator iter = device_list_.begin();
@@ skipping 106 matching lines @@
   list->clear();
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     list->push_back(network);
   }
 }
 
 void NetworkStateHandler::RequestScan() const {
-  network_event_log::AddEntry(kLogModule, "RequestScan", "");
+  NET_LOG_EVENT("RequestScan", "");
   shill_property_handler_->RequestScan();
 }
 
 void NetworkStateHandler::WaitForScan(const std::string& type,
                                       const base::Closure& callback) {
   scan_complete_callbacks_[type].push_back(callback);
   if (!GetScanningByType(type))
     RequestScan();
 }
 
 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()));
 }
 
 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(
     base::DictionaryValue* dictionary) const {
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     base::DictionaryValue* network_dict = new base::DictionaryValue;
     (*iter)->AsNetworkState()->GetProperties(network_dict);
     dictionary->SetWithoutPathExpansion((*iter)->path(), network_dict);
   }
@@ skipping 40 matching lines @@
     if (is_observing)
       managed->set_is_observed(true);
     if (request_properties)
       shill_property_handler_->RequestProperties(type, path);
   }
   // Delete any remaning entries in managed_map.
   STLDeleteContainerPairSecondPointers(managed_map.begin(), managed_map.end());
 }
 
 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(
         ManagedState::MANAGED_TYPE_NETWORK, (*iter)->path());
   }
 }
 
 void NetworkStateHandler::UpdateManagedStateProperties(
     ManagedState::ManagedType type,
     const std::string& path,
@@ skipping 10 matching lines @@
   for (base::DictionaryValue::Iterator iter(properties);
        !iter.IsAtEnd(); iter.Advance()) {
     if (type == ManagedState::MANAGED_TYPE_NETWORK) {
       if (managed->PropertyChanged(iter.key(), iter.value()))
         network_property_updated = true;
     } else {
       managed->PropertyChanged(iter.key(), iter.value());
     }
   }
   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();
     DCHECK(network);
     // Signal connection state changed after all properties have been updated.
     if (ConnectionStateChanged(network, prev_connection_state))
       OnNetworkConnectionStateChanged(network);
     NetworkPropertiesUpdated(network);
   }
 }
@@ skipping 13 matching lines @@
     if (ConnectionStateChanged(network, prev_connection_state))
       OnNetworkConnectionStateChanged(network);
   } else {
     if (network->path() == default_network_path_ &&
         key != flimflam::kSignalStrengthProperty) {
       // WiFi signal strength updates are too noisy, so don't
       // trigger default network updates for those changes.
       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);
 }
 
 void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
                                                const std::string& key,
                                                const base::Value& value) {
   DeviceState* device = GetModifiableDeviceState(device_path);
   if (!device)
     return;
   if (!device->PropertyChanged(key, value))
     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());
 
   if (key == flimflam::kScanningProperty && device->scanning() == false)
     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());
 }
 
 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 {
     NOTREACHED();
   }
 }
 
 //------------------------------------------------------------------------------
 // Private methods
 
@@ skipping 34 matching lines @@
     case ManagedState::MANAGED_TYPE_DEVICE:
       return &device_list_;
   }
   NOTREACHED();
   return NULL;
 }
 
 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_)
     OnDefaultNetworkChanged();
 }
 
 bool NetworkStateHandler::CheckDefaultNetworkChanged() {
   std::string new_default_network_path;
   const NetworkState* new_default_network = DefaultNetwork();
   if (new_default_network)
     new_default_network_path = new_default_network->path();
   if (new_default_network_path == default_network_path_)
     return false;
   default_network_path_ = new_default_network_path;
   return true;
 }
 
 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));
 }
 
 void NetworkStateHandler::NetworkPropertiesUpdated(
     const NetworkState* network) {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkPropertiesUpdated(network));
   // If |connecting_network_| transitions to a non-idle, non-connecting state,
   // clear it *after* signalling observers.
   if (network->path() == connecting_network_ &&
       !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];
   for (ScanCallbackList::iterator iter = callback_list.begin();
        iter != callback_list.end(); ++iter) {
     (*iter).Run();
   }
   scan_complete_callbacks_.erase(type);
 }
 
 std::string NetworkStateHandler::GetTechnologyForType(
     const std::string& type) const {
   if (type == kMatchTypeMobile) {
     if (shill_property_handler_->IsTechnologyAvailable(flimflam::kTypeWimax))
       return flimflam::kTypeWimax;
     else
       return flimflam::kTypeCellular;
   }
   if (type == kMatchTypeDefault || type == kMatchTypeNonVirtual ||
       type == kMatchTypeWireless) {
     NOTREACHED();
     return flimflam::kTypeWifi;
   }
   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