Re-factor network_event_log

chromeos/network/network_event_log.cc

Index: chromeos/network/network_event_log.cc
diff --git a/chromeos/network/network_event_log.cc b/chromeos/network/network_event_log.cc
index 7cb9e84a94d637747bf6c6ae4a0e647cb86e43cc..5d9050c9bd412c91b51b8471ae14504d0b6217e6 100644
--- a/chromeos/network/network_event_log.cc
+++ b/chromeos/network/network_event_log.cc
@@ -9,6 +9,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
+#include "base/values.h"
 
 namespace chromeos {
 
@@ -17,37 +18,61 @@ namespace network_event_log {
 namespace {
 
 struct LogEntry {
-  LogEntry(const std::string& module,
+  LogEntry(const std::string& file,
+           int file_line,
+           int log_level,

[gauravsh, 2013/05/13 00:34:51]

Why is this an int instead of a log-level? std::min and comparisons should still
work if use the enum type here.

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

Done.

            const std::string& event,
            const std::string& description);
 
-  std::string ToString() const;
+  std::string ToString(const std::string& format) const;

[gauravsh, 2013/05/13 00:34:51]

Comment for the format of |format|?

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

Changed to use bools

 
   bool ContentEquals(const LogEntry& other) const;
 
-  std::string module;
+  std::string file;

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

base::Location

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

Again, more complexity than I think we want, see ShillErrorCallbackFunction
comment.

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

With base::Location, the filename would not have to be copied for each entry.

+  int file_line;
+  int log_level;
   std::string event;
   std::string description;
   base::Time time;
   int count;
 };
 
-LogEntry::LogEntry(const std::string& module,
+LogEntry::LogEntry(const std::string& file,
+                   int file_line,
+                   int log_level,
                    const std::string& event,
                    const std::string& description)
-    : module(module),
+    : file(file),
+      file_line(file_line),
+      log_level(log_level),
       event(event),
       description(description),
       time(base::Time::Now()),
       count(1) {
 }
 
-std::string LogEntry::ToString() const {
+std::string LogEntry::ToString(const std::string& format) const {
   std::string line;
-  line += "[" + UTF16ToUTF8(base::TimeFormatShortDateAndTime(time)) + "]";
-  line += " " + module + ":" + event;
-  if (!description.empty())
+  if (format.find("time") != std::string::npos)
+    line += "[" + UTF16ToUTF8(base::TimeFormatShortDateAndTime(time)) + "] ";
+  if (format.find("file") != std::string::npos)
+    line += base::StringPrintf("%s:%d ", file.c_str(), file_line);
+  bool italic = (log_level == LOG_LEVEL_DEBUG &&
+                 format.find("html") != std::string::npos);
+  bool bold = (log_level == LOG_LEVEL_ERROR &&
+               format.find("html") != std::string::npos);
+  if (italic)
+    line += "<i>";
+  if (bold)
+    line += "<b>";
+  line += event;
+  if ((format.find("desc") != std::string::npos) && !description.empty())
     line += ": " + description;
+  if (bold)
+    line += "</b>";
+  if (italic)
+    line += "</i>";
+
   if (count > 1)
     line += base::StringPrintf(" (%d)", count);
   line += "\n";
@@ -55,7 +80,8 @@ std::string LogEntry::ToString() const {
 }
 
 bool LogEntry::ContentEquals(const LogEntry& other) const {
-  return module == other.module &&
+  return file == other.file &&
+      file_line == other.file_line &&
       event == other.event &&
       description == other.description;
 }
@@ -67,9 +93,11 @@ class NetworkEventLog {
   NetworkEventLog() {}
   ~NetworkEventLog() {}
 
-  void AddEntry(const LogEntry& entry);
+  void AddLogEntry(const LogEntry& entry);
 
   std::string GetAsString(StringOrder order,
+                          const std::string& format,
+                          int max_level,

[gauravsh, 2013/05/13 00:34:51]

Why is this not a LogLevel?

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

Done.

                           size_t max_events);
 
  private:
@@ -78,12 +106,13 @@ class NetworkEventLog {
   DISALLOW_COPY_AND_ASSIGN(NetworkEventLog);
 };
 
-void NetworkEventLog::AddEntry(const LogEntry& entry) {
+void NetworkEventLog::AddLogEntry(const LogEntry& entry) {
   if (!entries_.empty()) {
     LogEntry& last = entries_.back();
     if (last.ContentEquals(entry)) {
       // Update count and time for identical events to avoid log spam.
       ++last.count;
+      last.log_level = std::min(last.log_level, entry.log_level);
       last.time = base::Time::Now();
       return;
     }
@@ -91,10 +120,15 @@ void NetworkEventLog::AddEntry(const LogEntry& entry) {
   if (entries_.size() >= kMaxNetworkEventLogEntries)
     entries_.pop_front();
   entries_.push_back(entry);
-  VLOG(1) << entry.ToString();
+  if (entry.log_level == LOG_LEVEL_ERROR)
+    LOG(ERROR) << entry.ToString("time,file,desc");
+  else
+    VLOG(1) << entry.ToString("time,file,desc");
 }
 
 std::string NetworkEventLog::GetAsString(StringOrder order,
+                                         const std::string& format,
+                                         int max_level,
                                          size_t max_events) {
   if (entries_.empty())
     return "No Log Entries.";
@@ -102,18 +136,32 @@ std::string NetworkEventLog::GetAsString(StringOrder order,
   std::string result;
   if (order == OLDEST_FIRST) {
     size_t offset = 0;
-    if (max_events > 0 && max_events < entries_.size())
+    if (max_events > 0 && max_events < entries_.size()) {
       offset = entries_.size() - max_events;
+      // Iterate backwards through the list, decramenting the offset for entries

[gauravsh, 2013/05/13 00:34:51]

decrementing

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

Done.

+      // that will be skipped.
+      for (LogEntryList::const_reverse_iterator riter = entries_.rbegin();
+         riter != entries_.rend(); ++riter) {
+        if (riter->log_level > max_level) {
+          if (--offset == 0)

[gauravsh, 2013/05/13 00:34:51]

BUG? Wouldn't this end up decrementing the offset more than necessary? If I have
5 log entries such as:
1,INFO
2,ERROR
3,INFO
4,ERROR
5,INFO

GetAsString(OLDEST_FIRST, .., ERROR, 1) should return "4,ERROR"

However, according to this code:
line 138:  sets |offset| = 5 - 1 = 4
line 146:  --offset executed 3 times (# of log entries at the INFO level)
line 151: Final |offset| = 4 - 3 = 1
lines 152-155 would then return: 
  2,ERROR
  4,ERROR
...which is incorrect.

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

You're right, I need to check the shown events against max_events. Will add a
test to cover this.
Done.

+            break;
+        }
+      }
+    }
     for (LogEntryList::const_iterator iter = entries_.begin() + offset;
          iter != entries_.end(); ++iter) {
-      result += (*iter).ToString();
+      if (iter->log_level > max_level)
+        continue;
+      result += (*iter).ToString(format);
     }
   } else {
     size_t nlines = 0;
     // Iterate backwards through the list to show the most recent entries first.
     for (LogEntryList::const_reverse_iterator riter = entries_.rbegin();
          riter != entries_.rend(); ++riter) {
-      result += (*riter).ToString();
+      if (riter->log_level > max_level)
+        continue;
+      result += (*riter).ToString(format);
       if (max_events > 0 && ++nlines >= max_events)
         break;
     }
@@ -124,6 +172,7 @@ std::string NetworkEventLog::GetAsString(StringOrder order,
 }  // namespace
 
 NetworkEventLog* g_network_event_log = NULL;
+const int kDefaultLogLevel = 1;
 const size_t kMaxNetworkEventLogEntries = 1000;
 
 void Initialize() {
@@ -141,21 +190,54 @@ bool IsInitialized() {
   return g_network_event_log != NULL;
 }
 
-void AddEntry(const std::string& module,
+void AddEntry(const char* file,
+              int file_line,
+              LogLevel log_level,
               const std::string& event,
               const std::string& description) {
-  LogEntry entry(module, event, description);
+  std::string filestr;

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

FilePath::BaseName()

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

Done.

+  if (file) {
+    filestr = std::string(file);
+    size_t n = filestr.find_last_of('/');
+    if (n)
+      filestr = filestr.substr(n+1);
+  }
+  LogEntry entry(filestr, file_line, log_level, event, description);
   if (!g_network_event_log) {
-    VLOG(1) << entry.ToString();
+    VLOG(1) << entry.ToString("time,file,desc");

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

this should respect the log level.

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

Done.

     return;
   }
-  g_network_event_log->AddEntry(entry);
+  g_network_event_log->AddLogEntry(entry);
 }
 
-std::string GetAsString(StringOrder order, size_t max_events) {
+std::string GetAsString(StringOrder order,
+                        const std::string& format,
+                        int max_level,
+                        size_t max_events) {
   if (!g_network_event_log)
     return "NetworkEventLog not intitialized.";
-  return g_network_event_log->GetAsString(order, max_events);
+  return g_network_event_log->GetAsString(order, format, max_level, max_events);
+}
+
+std::string DBusValueAsString(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 "";
 }
 
 }  // namespace network_event_log