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..aed2465a87e58da6b112dabde350d69278b2bafe 100644
--- a/chromeos/network/network_event_log.cc
+++ b/chromeos/network/network_event_log.cc
@@ -4,11 +4,15 @@
 
 #include "chromeos/network/network_event_log.h"
 
+#include "base/files/file_path.h"
 #include "base/i18n/time_formatting.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stringprintf.h"
+#include "base/strings/string_tokenizer.h"
 #include "base/utf_string_conversions.h"
+#include "base/values.h"
+#include "net/base/escape.h"
 
 namespace chromeos {
 
@@ -17,49 +21,116 @@ namespace network_event_log {
 namespace {
 
 struct LogEntry {
-  LogEntry(const std::string& module,
+  LogEntry(const std::string& file,
+           int file_line,
+           LogLevel log_level,
            const std::string& event,
            const std::string& description);
 
-  std::string ToString() const;
+  std::string ToString(bool show_time,
+                       bool show_file,
+                       bool show_desc,
+                       bool format_html) const;
+  void SendToVLogOrErrorLog() const;
 
   bool ContentEquals(const LogEntry& other) const;
 
-  std::string module;
+  std::string file;
+  int file_line;
+  LogLevel 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,
+                   LogLevel 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(bool show_time,
+                               bool show_file,
+                               bool show_desc,
+                               bool format_html) const {
   std::string line;
-  line += "[" + UTF16ToUTF8(base::TimeFormatShortDateAndTime(time)) + "]";
-  line += " " + module + ":" + event;
-  if (!description.empty())
-    line += ": " + description;
+  if (show_time)
+    line += "[" + UTF16ToUTF8(base::TimeFormatShortDateAndTime(time)) + "] ";
+  if (show_file) {
+    line += net::EscapeForHTML(
+        base::StringPrintf("%s:%d ", file.c_str(), file_line));
+  }
+  bool italic = (log_level == LOG_LEVEL_DEBUG && format_html);
+  bool bold = (log_level == LOG_LEVEL_ERROR && format_html);
+  if (italic)
+    line += "<i>";
+  if (bold)
+    line += "<b>";
+  line += event;
+  if (show_desc && !description.empty())
+    line += ": " + net::EscapeForHTML(description);
+  if (bold)
+    line += "</b>";
+  if (italic)
+    line += "</i>";
+
   if (count > 1)
     line += base::StringPrintf(" (%d)", count);
   line += "\n";
   return line;
 }
 
+void LogEntry::SendToVLogOrErrorLog() const {
+  const bool show_time = true;
+  const bool show_file = true;
+  const bool show_desc = true;
+  const bool format_html = false;
+  std::string output = ToString(show_time, show_file, show_desc, format_html);
+  if (log_level == LOG_LEVEL_ERROR)
+    LOG(ERROR) << output;
+  else
+    VLOG(1) << output;
+}
+
 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;
 }
 
+void GetFormat(const std::string& format_string,
+               bool* show_time,
+               bool* show_file,
+               bool* show_desc,
+               bool* format_html) {
+  base::StringTokenizer tokens(format_string, ",");
+  *show_time = false;
+  *show_file = false;
+  *show_desc = false;
+  *format_html = false;
+  while (tokens.GetNext()) {
+    std::string tok(tokens.token());
+    if (tok == "time")
+      *show_time = true;
+    if (tok == "file")
+      *show_file = true;
+    if (tok == "desc")
+      *show_desc = true;
+    if (tok == "html")
+      *format_html = true;
+  }
+}
+
 typedef std::deque<LogEntry> LogEntryList;
 
 class NetworkEventLog {
@@ -67,9 +138,11 @@ class NetworkEventLog {
   NetworkEventLog() {}
   ~NetworkEventLog() {}
 
-  void AddEntry(const LogEntry& entry);
+  void AddLogEntry(const LogEntry& entry);
 
   std::string GetAsString(StringOrder order,
+                          const std::string& format,
+                          LogLevel max_level,
                           size_t max_events);
 
  private:
@@ -78,12 +151,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,29 +165,51 @@ void NetworkEventLog::AddEntry(const LogEntry& entry) {
   if (entries_.size() >= kMaxNetworkEventLogEntries)
     entries_.pop_front();
   entries_.push_back(entry);
-  VLOG(1) << entry.ToString();
+  entry.SendToVLogOrErrorLog();
 }
 
 std::string NetworkEventLog::GetAsString(StringOrder order,
+                                         const std::string& format,
+                                         LogLevel max_level,
                                          size_t max_events) {
   if (entries_.empty())
     return "No Log Entries.";
 
+  bool show_time, show_file, show_desc, format_html;
+  GetFormat(format, &show_time, &show_file, &show_desc, &format_html);
+
   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, decrementing the offset for entries
+      // that will be skipped.
+      size_t shown_events = 0;
+      for (LogEntryList::const_reverse_iterator riter = entries_.rbegin();
+         riter != entries_.rend(); ++riter) {
+        if (riter->log_level > max_level) {
+          if (--offset == 0)
+            break;
+        }
+        if (++shown_events >= max_events)

[gauravsh, 2013/05/14 00:01:39]

You want an else here, or this will bail too early*. shown_events should only be
incremented if the entry wouldn't be skipped.

You could consider rewriting the code by just keeping track of |shown_events|
and then before breaking out of the loop, computing the offset based on the
current position of the iterator. That's probably more readable and less error
prone.

[*Can be verified by the following addition to
NetworkEventLogTest.TestMaxEvents:

output = network_event_log::GetAsString(
      network_event_log::OLDEST_FIRST, "",
      network_event_log::LOG_LEVEL_ERROR, 2);
EXPECT_EQ("error2\nerror4\n", output);
]

[stevenjb, 2013/05/14 00:27:42]

Bah, where's my brain, must be Monday. I was thinking else when I added it...

Yeah, not a huge fan of iterator subtraction, but might be more clear... Added a
couple more tests, but please triple check my logic...

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(show_time, show_file, show_desc, format_html);
     }
   } 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(show_time, show_file, show_desc, format_html);
       if (max_events > 0 && ++nlines >= max_events)
         break;
     }
@@ -124,6 +220,7 @@ std::string NetworkEventLog::GetAsString(StringOrder order,
 }  // namespace
 
 NetworkEventLog* g_network_event_log = NULL;
+const LogLevel kDefaultLogLevel = LOG_LEVEL_EVENT;
 const size_t kMaxNetworkEventLogEntries = 1000;
 
 void Initialize() {
@@ -141,21 +238,33 @@ bool IsInitialized() {
   return g_network_event_log != NULL;
 }
 
-void AddEntry(const std::string& module,
+namespace internal {
+
+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;
+  if (file)
+    filestr = base::FilePath(std::string(file)).BaseName().value();
+  LogEntry entry(filestr, file_line, log_level, event, description);
   if (!g_network_event_log) {
-    VLOG(1) << entry.ToString();
+    entry.SendToVLogOrErrorLog();
     return;
   }
-  g_network_event_log->AddEntry(entry);
+  g_network_event_log->AddLogEntry(entry);
 }
 
-std::string GetAsString(StringOrder order, size_t max_events) {
+}  // namespace internal
+
+std::string GetAsString(StringOrder order,
+                        const std::string& format,
+                        LogLevel 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 "NetworkEventLog not initialized.";
+  return g_network_event_log->GetAsString(order, format, max_level, max_events);
 }
 
 }  // namespace network_event_log