C++ readability review from original change https://chromiumcodereview.appspot.com/14263024/

Review bug: http://b/8772053

Review URL: https://chromiumcodereview.appspot.com/14591005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204053 0039d316-1c4b-4281-b951-d872f2087c98

chrome/test/chromedriver/chrome/console_logger.cc

@@ -4,10 +4,9 @@
 
 #include "chrome/test/chromedriver/chrome/console_logger.h"
 
-#include <sstream>
-
 #include "base/json/json_writer.h"
-#include "base/string_util.h"
+#include "base/logging.h"
+#include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/test/chromedriver/chrome/devtools_client.h"
 #include "chrome/test/chromedriver/chrome/log.h"
@@ -15,14 +14,15 @@
 
 namespace {
 
+// Translates Console.messageAdded.message.level into Log::Level.
 bool ConsoleLevelToLogLevel(const std::string& name, Log::Level *out_level) {
-  const char* kConsoleLevelNames[] = {
+  const char* const kConsoleLevelNames[] = {
     "debug", "log", "warning", "error"
   };
 
   for (size_t i = 0; i < arraysize(kConsoleLevelNames); ++i) {
     if (name == kConsoleLevelNames[i]) {
-      CHECK(Log::kDebug + i <= Log::kError);
+      CHECK_LE(Log::kDebug + i, static_cast<size_t>(Log::kError));
       *out_level = static_cast<Log::Level>(Log::kDebug + i);
       return true;
     }
@@ -40,51 +40,52 @@ Status ConsoleLogger::OnConnected(DevToolsClient* client) {
   return client->SendCommand("Console.enable", params);
 }
 
-Status ConsoleLogger::OnEvent(DevToolsClient* client,
-                              const std::string& method,
-                              const base::DictionaryValue& params) {
-  if (!StartsWithASCII(method, "Console.messageAdded", true))
+Status ConsoleLogger::OnEvent(
+    DevToolsClient* client,
+    const std::string& method,
+    const base::DictionaryValue& params) {
+  if (method != "Console.messageAdded")
     return Status(kOk);
 
   // If the event has proper structure and fields, log formatted.
   // Else it's a weird message that we don't know how to format, log full JSON.
   const base::DictionaryValue *message_dict = NULL;
   if (params.GetDictionary("message", &message_dict)) {
-    std::ostringstream message;
-    std::string origin;
-    if (message_dict->GetString("url", &origin) && !origin.empty()) {
-      message << origin;
-    } else if (message_dict->GetString("source", &origin) && !origin.empty()) {
-      message << origin;
-    } else {
-      message << "unknown";
-    }
+    std::string text;
+    std::string level_name;
+    Log::Level level = Log::kLog;
+    if (message_dict->GetString("text", &text) && !text.empty() &&
+        message_dict->GetString("level", &level_name) &&
+        ConsoleLevelToLogLevel(level_name, &level)) {
 
-    int line = -1;
-    if (message_dict->GetInteger("line", &line)) {
-      message << " " << line;
-      int column = -1;
-      if (message_dict->GetInteger("column", &column)) {
-        message << ":" << column;
+      const char* origin_cstr = "unknown";
+      std::string origin;
+      if ((message_dict->GetString("url", &origin) && !origin.empty()) ||
+          (message_dict->GetString("source", &origin) && !origin.empty())) {
+        origin_cstr = origin.c_str();
       }
-    } else {
-      // No line number, but print anyway, just to maintain the number of
-      // fields in the formatted message in case someone wants to parse it.
-      message << " -";
-    }
 
-    std::string text;
-    if (message_dict->GetString("text", &text)) {
-      message << " " << text;
-
-      std::string level_name;
-      Log::Level level = Log::kLog;
-      if (message_dict->GetString("level", &level_name)) {
-        if (ConsoleLevelToLogLevel(level_name, &level)) {
-          log_->AddEntry(level, message.str());  // Found all expected fields.
-          return Status(kOk);
+      std::string line_column;
+      int line = -1;
+      if (message_dict->GetInteger("line", &line)) {
+        int column = -1;
+        if (message_dict->GetInteger("column", &column)) {
+          base::SStringPrintf(&line_column, "%d:%d", line, column);
+        } else {
+          base::SStringPrintf(&line_column, "%d", line);
         }
+      } else {
+        // No line number, but print anyway, just to maintain the number of
+        // fields in the formatted message in case someone wants to parse it.
+        line_column = "-";
       }
+
+      log_->AddEntry(level,
+                     base::StringPrintf("%s %s %s",
+                                        origin_cstr,
+                                        line_column.c_str(),
+                                        text.c_str()));
+      return Status(kOk);
     }
   }
 

chrome/test/chromedriver/chrome/console_logger.h

@@ -19,15 +19,19 @@ class Log;
 // Translates the level into Log::Level, drops all other fields.
 class ConsoleLogger : public DevToolsEventListener {
  public:
+  // Creates a ConsoleLogger that creates entries in the given Log object.
+  // The log is owned elsewhere and must not be null.
   explicit ConsoleLogger(Log* log);
 
+  // Enables Console events for the client, which must not be null.
   virtual Status OnConnected(DevToolsClient* client) OVERRIDE;
+  // Translates an event into a log entry.
   virtual Status OnEvent(DevToolsClient* client,
                          const std::string& method,
                          const base::DictionaryValue& params) OVERRIDE;
 
  private:
-  Log* log_;
+  Log* log_;  // The log where to create entries.
 
   DISALLOW_COPY_AND_ASSIGN(ConsoleLogger);
 };

chrome/test/chromedriver/chrome/console_logger_unittest.cc

@@ -18,7 +18,8 @@ namespace {
 
 class FakeDevToolsClient : public StubDevToolsClient {
  public:
-  explicit FakeDevToolsClient(const std::string& id) : id_(id) {}
+  explicit FakeDevToolsClient(const std::string& id)
+      : id_(id), listener_(NULL) {}
   virtual ~FakeDevToolsClient() {}
 
   std::string PopSentCommand() {
@@ -49,6 +50,7 @@ class FakeDevToolsClient : public StubDevToolsClient {
   }
 
   virtual void AddListener(DevToolsEventListener* listener) OVERRIDE {
+    CHECK(!listener_);
     listener_ = listener;
   }
 
@@ -57,9 +59,9 @@ class FakeDevToolsClient : public StubDevToolsClient {
   }
 
  private:
-  const std::string id_;
-  std::list<std::string> sent_command_queue_;
-  DevToolsEventListener* listener_;
+  const std::string id_;  // WebView id.
+  std::list<std::string> sent_command_queue_;  // Commands that were sent.
+  DevToolsEventListener* listener_;  // The fake allows only one event listener.
 };
 
 struct LogEntry {
@@ -75,24 +77,29 @@ struct LogEntry {
 
 class FakeLog : public Log {
  public:
-  virtual void AddEntry(const base::Time& time,
-                        Level level,
-                        const std::string& message) OVERRIDE;
+  virtual void AddEntryTimestamped(const base::Time& timestamp,
+                                   Level level,
+                                   const std::string& message) OVERRIDE;
 
-  ScopedVector<LogEntry> entries;
+  const ScopedVector<LogEntry>& GetEntries() {
+    return entries_;
+  }
+
+private:
+  ScopedVector<LogEntry> entries_;
 };
 
-void FakeLog::AddEntry(
-    const base::Time& time, Level level, const std::string& message) {
-  entries.push_back(new LogEntry(time, level, message));
+void FakeLog::AddEntryTimestamped(
+    const base::Time& timestamp, Level level, const std::string& message) {
+  entries_.push_back(new LogEntry(timestamp, level, message));
 }
 
-void ValidateLogEntry(LogEntry *entry,
-                      Log::Level expect_level,
-                      const char* expect_message) {
-  EXPECT_EQ(expect_level, entry->level);
+void ValidateLogEntry(const LogEntry *entry,
+                      Log::Level expected_level,
+                      const std::string& expected_message) {
+  EXPECT_EQ(expected_level, entry->level);
   EXPECT_LT(0, entry->timestamp.ToTimeT());
-  EXPECT_STREQ(expect_message, entry->message.c_str());
+  EXPECT_EQ(expected_message, entry->message);
 }
 
 void ConsoleLogParams(base::DictionaryValue* out_params,
@@ -102,17 +109,17 @@ void ConsoleLogParams(base::DictionaryValue* out_params,
                       int line,
                       int column,
                       const char* text) {
-  if (NULL != source)
+  if (source != NULL)
     out_params->SetString("message.source", source);
-  if (NULL != url)
+  if (url != NULL)
     out_params->SetString("message.url", url);
-  if (NULL != level)
+  if (level != NULL)
     out_params->SetString("message.level", level);
-  if (-1 != line)
+  if (line != -1)
     out_params->SetInteger("message.line", line);
-  if (-1 != column)
+  if (column != -1)
     out_params->SetInteger("message.column", column);
-  if (NULL != text)
+  if (text != NULL)
     out_params->SetString("message.text", text);
 }
 
@@ -125,8 +132,8 @@ TEST(ConsoleLogger, ConsoleMessages) {
 
   client.AddListener(&logger);
   logger.OnConnected(&client);
-  EXPECT_STREQ("Console.enable", client.PopSentCommand().c_str());
-  EXPECT_STREQ("", client.PopSentCommand().c_str());
+  EXPECT_EQ("Console.enable", client.PopSentCommand());
+  EXPECT_TRUE(client.PopSentCommand().empty());
 
   base::DictionaryValue params1;  // All fields are set.
   ConsoleLogParams(&params1, "source1", "url1", "debug", 10, 1, "text1");
@@ -162,24 +169,24 @@ TEST(ConsoleLogger, ConsoleMessages) {
   params8.SetInteger("gaga", 8);
   ASSERT_EQ(kOk, client.TriggerEvent("Console.messageAdded", params8).code());
 
-  EXPECT_STREQ("", client.PopSentCommand().c_str());  // No other commands sent.
+  EXPECT_TRUE(client.PopSentCommand().empty());  // No other commands sent.
 
-  ASSERT_EQ(8u, log.entries.size());
-  ValidateLogEntry(log.entries[0], Log::kDebug, "url1 10:1 text1");
-  ValidateLogEntry(log.entries[1], Log::kLog, "source2 - text2");
-  ValidateLogEntry(log.entries[2], Log::kWarning, "url3 30 text3");
-  ValidateLogEntry(log.entries[3], Log::kError, "url4 - text4");
+  ASSERT_EQ(8u, log.GetEntries().size());
+  ValidateLogEntry(log.GetEntries()[0], Log::kDebug, "url1 10:1 text1");
+  ValidateLogEntry(log.GetEntries()[1], Log::kLog, "source2 - text2");
+  ValidateLogEntry(log.GetEntries()[2], Log::kWarning, "url3 30 text3");
+  ValidateLogEntry(log.GetEntries()[3], Log::kError, "url4 - text4");
   ValidateLogEntry(
-      log.entries[4], Log::kWarning,
+      log.GetEntries()[4], Log::kWarning,
       "{\"message\":{\"column\":5,\"level\":\"gaga\",\"line\":50,"
       "\"source\":\"source5\",\"text\":\"ulala\",\"url\":\"url5\"}}");
   ValidateLogEntry(
-      log.entries[5], Log::kWarning,
+      log.GetEntries()[5], Log::kWarning,
       "{\"message\":{\"column\":6,\"line\":60,"
       "\"source\":\"source6\",\"url\":\"url6\"}}");
   ValidateLogEntry(
-      log.entries[6], Log::kWarning,
+      log.GetEntries()[6], Log::kWarning,
       "{\"message\":{\"level\":\"log\","
       "\"source\":\"source7\",\"url\":\"url7\"}}");
-  ValidateLogEntry(log.entries[7], Log::kWarning, "{\"gaga\":8}");
+  ValidateLogEntry(log.GetEntries()[7], Log::kWarning, "{\"gaga\":8}");
 }

chrome/test/chromedriver/chrome/log.cc

@@ -78,8 +78,8 @@ std::string ConvertForDisplay(const std::string& input) {
 
 }  // namespace
 
-void Log::AddEntry(Log::Level level, const std::string& message) {
-  return AddEntry(base::Time::Now(), level, message);
+void Log::AddEntry(Level level, const std::string& message) {
+ AddEntryTimestamped(base::Time::Now(), level, message);
 }
 
 Logger::Logger() : min_log_level_(kLog), start_(base::Time::Now()) {}
@@ -89,9 +89,9 @@ Logger::Logger(Level min_log_level)
 
 Logger::~Logger() {}
 
-void Logger::AddEntry(const base::Time& time,
-                      Level level,
-                      const std::string& message) {
+void Logger::AddEntryTimestamped(const base::Time& timestamp,
+                                 Level level,
+                                 const std::string& message) {
   if (level < min_log_level_)
     return;
 
@@ -114,7 +114,7 @@ void Logger::AddEntry(const base::Time& time,
   }
   std::string entry =
       base::StringPrintf("[%.3lf][%s]: %s",
-                         base::TimeDelta(time - start_).InSecondsF(),
+                         base::TimeDelta(timestamp - start_).InSecondsF(),
                          level_name,
                          ConvertForDisplay(message).c_str());
   const char* format = "%s\n";

chrome/test/chromedriver/chrome/log.h

@@ -10,9 +10,10 @@
 #include "base/compiler_specific.h"
 #include "base/time.h"
 
-// Accepts log entries that have a level, timestamp, and a string message.
+// Abstract class for logging entries with a level, timestamp, string message.
 class Log {
  public:
+  // Log entry severity level.
   enum Level {
     kDebug,
     kLog,
@@ -22,12 +23,12 @@ class Log {
 
   virtual ~Log() {}
 
-  // Log a message with an explicit timestamp.
-  virtual void AddEntry(const base::Time& time,
-                        Level level,
-                        const std::string& message) = 0;
+  // Adds an entry to the log.
+  virtual void AddEntryTimestamped(const base::Time& timestamp,
+                                   Level level,
+                                   const std::string& message) = 0;
 
-  // Implicit timestamp, default to current time.
+  // Adds an entry to the log, timestamped with the current time.
   void AddEntry(Level level, const std::string& message);
 };
 
@@ -39,9 +40,9 @@ class Logger : public Log {
   explicit Logger(Level min_log_level);
   virtual ~Logger();
 
-  virtual void AddEntry(const base::Time& time,
-                        Level level,
-                        const std::string& message) OVERRIDE;
+  virtual void AddEntryTimestamped(const base::Time& timestamp,
+                                   Level level,
+                                   const std::string& message) OVERRIDE;
 
  private:
   Level min_log_level_;

chrome/test/chromedriver/chrome/performance_logger.cc

@@ -14,16 +14,16 @@
 namespace {
 
 // DevTools event domain prefixes to intercept.
-const char* kDomains[] = {"Network.", "Page.", "Timeline."};
+const char* const kDomains[] = {"Network.", "Page.", "Timeline."};
 
-const char* kDomainEnableCommands[] = {
+const char* const kDomainEnableCommands[] = {
     "Network.enable", "Page.enable", "Timeline.start"
 };
 
 // Returns whether the event belongs to one of kDomains.
 bool ShouldLogEvent(const std::string& method) {
   for (size_t i_domain = 0; i_domain < arraysize(kDomains); ++i_domain) {
-    if (StartsWithASCII(method, kDomains[i_domain], true))
+    if (StartsWithASCII(method, kDomains[i_domain], true /* case_sensitive */))
       return true;
   }
   return false;

chrome/test/chromedriver/chrome/performance_logger.h

@@ -20,15 +20,19 @@ class Log;
 // }
 class PerformanceLogger : public DevToolsEventListener {
  public:
+  // Creates a PerformanceLogger that creates entries in the given Log object.
+  // The log is owned elsewhere and must not be null.
   explicit PerformanceLogger(Log* log);
 
+  // Enables Page,Network,Timeline events for client, which must not be null.
   virtual Status OnConnected(DevToolsClient* client) OVERRIDE;
+  // Translates an event into a log entry.
   virtual Status OnEvent(DevToolsClient* client,
                          const std::string& method,
                          const base::DictionaryValue& params) OVERRIDE;
 
  private:
-  Log* log_;
+  Log* log_;  // The log where to create entries.
 
   DISALLOW_COPY_AND_ASSIGN(PerformanceLogger);
 };

chrome/test/chromedriver/chrome/performance_logger_unittest.cc

@@ -19,7 +19,8 @@ namespace {
 
 class FakeDevToolsClient : public StubDevToolsClient {
  public:
-  explicit FakeDevToolsClient(const std::string& id) : id_(id) {}
+  explicit FakeDevToolsClient(const std::string& id)
+      : id_(id), listener_(NULL) {}
   virtual ~FakeDevToolsClient() {}
 
   std::string PopSentCommand() {
@@ -50,6 +51,7 @@ class FakeDevToolsClient : public StubDevToolsClient {
   }
 
   virtual void AddListener(DevToolsEventListener* listener) OVERRIDE {
+    CHECK(!listener_);
     listener_ = listener;
   }
 
@@ -58,9 +60,9 @@ class FakeDevToolsClient : public StubDevToolsClient {
   }
 
  private:
-  const std::string id_;
-  std::list<std::string> sent_command_queue_;
-  DevToolsEventListener* listener_;
+  const std::string id_;  // WebView id.
+  std::list<std::string> sent_command_queue_;  // Commands that were sent.
+  DevToolsEventListener* listener_;  // The fake allows only one event listener.
 };
 
 struct LogEntry {
@@ -76,29 +78,34 @@ struct LogEntry {
 
 class FakeLog : public Log {
  public:
-  virtual void AddEntry(const base::Time& time,
+  virtual void AddEntryTimestamped(const base::Time& timestamp,
                         Level level,
                         const std::string& message) OVERRIDE;
 
-  ScopedVector<LogEntry> entries;
+  const ScopedVector<LogEntry>& GetEntries() {
+    return entries_;
+  }
+
+ private:
+  ScopedVector<LogEntry> entries_;
 };
 
-void FakeLog::AddEntry(
-    const base::Time& time, Level level, const std::string& message) {
-  entries.push_back(new LogEntry(time, level, message));
+void FakeLog::AddEntryTimestamped(
+    const base::Time& timestamp, Level level, const std::string& message) {
+  entries_.push_back(new LogEntry(timestamp, level, message));
 }
 
 scoped_ptr<DictionaryValue> ParseDictionary(const std::string& json) {
   std::string error;
   scoped_ptr<Value> value(base::JSONReader::ReadAndReturnError(
       json, base::JSON_PARSE_RFC, NULL, &error));
-  if (NULL == value) {
+  if (value == NULL) {
     SCOPED_TRACE(json.c_str());
     SCOPED_TRACE(error.c_str());
     ADD_FAILURE();
     return scoped_ptr<DictionaryValue>(NULL);
   }
-  DictionaryValue* dict = 0;
+  DictionaryValue* dict = NULL;
   if (!value->GetAsDictionary(&dict)) {
     SCOPED_TRACE("JSON object is not a dictionary");
     ADD_FAILURE();
@@ -107,29 +114,29 @@ scoped_ptr<DictionaryValue> ParseDictionary(const std::string& json) {
   return scoped_ptr<DictionaryValue>(dict->DeepCopy());
 }
 
-void ValidateLogEntry(LogEntry *entry,
-                      const char* expect_webview,
-                      const char* expect_method) {
+void ValidateLogEntry(const LogEntry *entry,
+                      const std::string& expected_webview,
+                      const std::string& expected_method) {
   EXPECT_EQ(Log::kLog, entry->level);
   EXPECT_LT(0, entry->timestamp.ToTimeT());
 
   scoped_ptr<base::DictionaryValue> message(ParseDictionary(entry->message));
   std::string webview;
   EXPECT_TRUE(message->GetString("webview", &webview));
-  EXPECT_STREQ(expect_webview, webview.c_str());
+  EXPECT_EQ(expected_webview, webview);
   std::string method;
   EXPECT_TRUE(message->GetString("message.method", &method));
-  EXPECT_STREQ(expect_method, method.c_str());
+  EXPECT_EQ(expected_method, method);
   DictionaryValue* params;
   EXPECT_TRUE(message->GetDictionary("message.params", &params));
   EXPECT_EQ(0u, params->size());
 }
 
 void ExpectEnableDomains(FakeDevToolsClient& client) {
-  EXPECT_STREQ("Network.enable", client.PopSentCommand().c_str());
-  EXPECT_STREQ("Page.enable", client.PopSentCommand().c_str());
-  EXPECT_STREQ("Timeline.start", client.PopSentCommand().c_str());
-  EXPECT_STREQ("", client.PopSentCommand().c_str());
+  EXPECT_EQ("Network.enable", client.PopSentCommand());
+  EXPECT_EQ("Page.enable", client.PopSentCommand());
+  EXPECT_EQ("Timeline.start", client.PopSentCommand());
+  EXPECT_TRUE(client.PopSentCommand().empty());
 }
 
 }  // namespace
@@ -147,9 +154,9 @@ TEST(PerformanceLogger, OneWebView) {
   // Ignore -- different domain.
   ASSERT_EQ(kOk, client.TriggerEvent("Console.bad").code());
 
-  ASSERT_EQ(2u, log.entries.size());
-  ValidateLogEntry(log.entries[0], "webview-1", "Network.gaga");
-  ValidateLogEntry(log.entries[1], "webview-1", "Page.ulala");
+  ASSERT_EQ(2u, log.GetEntries().size());
+  ValidateLogEntry(log.GetEntries()[0], "webview-1", "Network.gaga");
+  ValidateLogEntry(log.GetEntries()[1], "webview-1", "Page.ulala");
 }
 
 TEST(PerformanceLogger, TwoWebViews) {
@@ -167,12 +174,12 @@ TEST(PerformanceLogger, TwoWebViews) {
   // OnConnected sends the enable command only to that client, not others.
   client1.ConnectIfNecessary();
   ExpectEnableDomains(client1);
-  EXPECT_STREQ("", client2.PopSentCommand().c_str());
+  EXPECT_TRUE(client2.PopSentCommand().empty());;
 
   ASSERT_EQ(kOk, client1.TriggerEvent("Page.gaga1").code());
   ASSERT_EQ(kOk, client2.TriggerEvent("Timeline.gaga2").code());
 
-  ASSERT_EQ(2u, log.entries.size());
-  ValidateLogEntry(log.entries[0], "webview-1", "Page.gaga1");
-  ValidateLogEntry(log.entries[1], "webview-2", "Timeline.gaga2");
+  ASSERT_EQ(2u, log.GetEntries().size());
+  ValidateLogEntry(log.GetEntries()[0], "webview-1", "Page.gaga1");
+  ValidateLogEntry(log.GetEntries()[1], "webview-2", "Timeline.gaga2");
 }

chrome/test/chromedriver/logging.cc

@@ -17,7 +17,7 @@ namespace {
 
 // Map between WebDriverLog::WebDriverLevel and its name in WD wire protocol.
 // Array indices are the WebDriverLog::WebDriverLevel enum values.
-const char* kWebDriverLevelNames[] = {
+const char* const kWebDriverLevelNames[] = {
   "ALL", "DEBUG", "INFO", "WARNING", "SEVERE", "OFF"
 };
 
@@ -30,18 +30,20 @@ WebDriverLog::WebDriverLevel kLogLevelToWebDriverLevels[] = {
   WebDriverLog::kWdSevere  // kError
 };
 
+// Translates Log::Level to WebDriverLog::WebDriverLevel.
 WebDriverLog::WebDriverLevel LogLevelToWebDriverLevel(Log::Level level) {
   const int index = level - Log::kDebug;
-  CHECK(index >= 0);
-  CHECK(static_cast<size_t>(index) < arraysize(kLogLevelToWebDriverLevels));
+  CHECK_GE(index, 0);
+  CHECK_LT(static_cast<size_t>(index), arraysize(kLogLevelToWebDriverLevels));
   return kLogLevelToWebDriverLevels[index];
 }
 
+// Returns WD wire protocol level name for a WebDriverLog::WebDriverLevel.
 std::string GetWebDriverLevelName(
     const WebDriverLog::WebDriverLevel level) {
   const int index = level - WebDriverLog::kWdAll;
-  CHECK(index >= 0);
-  CHECK(static_cast<size_t>(index) < arraysize(kWebDriverLevelNames));
+  CHECK_GE(index, 0);
+  CHECK_LT(static_cast<size_t>(index), arraysize(kWebDriverLevelNames));
   return kWebDriverLevelNames[index];
 }
 
@@ -51,7 +53,8 @@ bool WebDriverLog::NameToLevel(
     const std::string& name, WebDriverLog::WebDriverLevel* out_level) {
   for (size_t i = 0; i < arraysize(kWebDriverLevelNames); ++i) {
     if (name == kWebDriverLevelNames[i]) {
-      CHECK(WebDriverLog::kWdAll + i <= WebDriverLog::kWdOff);
+      CHECK_LE(WebDriverLog::kWdAll + i,
+               static_cast<size_t>(WebDriverLog::kWdOff));
       *out_level =
           static_cast<WebDriverLog::WebDriverLevel>(WebDriverLog::kWdAll + i);
       return true;
@@ -70,21 +73,22 @@ WebDriverLog::WebDriverLog(
 
 WebDriverLog::~WebDriverLog() {
   VLOG(1) << "Log type '" << type_ << "' lost "
-      << entries_->GetSize() << " entries on destruction";
+          << entries_->GetSize() << " entries on destruction";
 }
 
 const std::string& WebDriverLog::GetType() {
   return type_;
 }
 
-void WebDriverLog::AddEntry(const base::Time& time,
-                            Log::Level level,
-                            const std::string& message) {
+void WebDriverLog::AddEntryTimestamped(const base::Time& timestamp,
+                                       Log::Level level,
+                                       const std::string& message) {
   const WebDriverLog::WebDriverLevel wd_level = LogLevelToWebDriverLevel(level);
   if (wd_level < min_wd_level_)
     return;
   scoped_ptr<base::DictionaryValue> log_entry_dict(new base::DictionaryValue());
-  log_entry_dict->SetDouble("timestamp", static_cast<int64>(time.ToJsTime()));
+  log_entry_dict->SetDouble("timestamp",
+                            static_cast<int64>(timestamp.ToJsTime()));
   log_entry_dict->SetString("level", GetWebDriverLevelName(wd_level));
   log_entry_dict->SetString("message", message);
   entries_->Append(log_entry_dict.release());

chrome/test/chromedriver/logging.h

@@ -18,10 +18,11 @@ class DevToolsEventListener;
 class ListValue;
 class Status;
 
-// Accumulates WebDriver Logging API events of a given type and minimum level.
+// Accumulates WebDriver Logging API entries of a given type and minimum level.
 // See https://code.google.com/p/selenium/wiki/Logging.
 class WebDriverLog : public Log {
  public:
+  // Constants corresponding to log entry severity levels in the wire protocol.
   enum WebDriverLevel {
     kWdAll,
     kWdDebug,
@@ -34,22 +35,28 @@ class WebDriverLog : public Log {
   // Converts WD wire protocol level name -> WebDriverLevel, false on bad name.
   static bool NameToLevel(const std::string& name, WebDriverLevel* out_level);
 
+  // Creates a WebDriverLog with the given type and minimum level.
   WebDriverLog(const std::string& type, WebDriverLevel min_wd_level);
   virtual ~WebDriverLog();
 
+  // Returns this log's type, for the WD wire protocol "/log" and "/log/types".
   const std::string& GetType();
 
+  // Returns entries accumulated so far, as a ListValue ready for serialization
+  // into the wire protocol response to the "/log" command.
+  // The caller assumes ownership of the ListValue, and the WebDriverLog
+  // creates and owns a new empty ListValue for further accumulation.
   scoped_ptr<base::ListValue> GetAndClearEntries();
 
-  virtual void AddEntry(const base::Time& time,
-                        Level level,
-                        const std::string& message) OVERRIDE;
-  using Log::AddEntry;  // Inherited overload that takes level and message.
+  // Translates a Log entry level into a WebDriver level and stores the entry.
+  virtual void AddEntryTimestamped(const base::Time& timestamp,
+                                   Level level,
+                                   const std::string& message) OVERRIDE;
 
  private:
-  const std::string type_;
-  const WebDriverLevel min_wd_level_;
-  scoped_ptr<base::ListValue> entries_;
+  const std::string type_;  // WebDriver log type.
+  const WebDriverLevel min_wd_level_;  // Minimum level of entries to store.
+  scoped_ptr<base::ListValue> entries_;  // Accumulated entries.
 
   DISALLOW_COPY_AND_ASSIGN(WebDriverLog);
 };

chrome/test/chromedriver/logging_unittest.cc

@@ -12,7 +12,7 @@
 
 namespace {
 
-static const char* kAllWdLevels[] = {
+const char* const kAllWdLevels[] = {
   "ALL", "DEBUG", "INFO", "WARNING", "SEVERE", "OFF"
 };
 
@@ -46,16 +46,16 @@ namespace {
 
 void ValidateLogEntry(base::ListValue *entries,
                       int index,
-                      const char* expect_level,
-                      const char* expect_message) {
+                      const std::string& expected_level,
+                      const std::string& expected_message) {
   const base::DictionaryValue *entry;
   ASSERT_TRUE(entries->GetDictionary(index, &entry));
   std::string level;
   EXPECT_TRUE(entry->GetString("level", &level));
-  EXPECT_STREQ(expect_level, level.c_str());
+  EXPECT_EQ(expected_level, level);
   std::string message;
   ASSERT_TRUE(entry->GetString("message", &message));
-  EXPECT_STREQ(expect_message, message.c_str());
+  EXPECT_EQ(expected_message, message);
   double timestamp = 0;
   EXPECT_TRUE(entry->GetDouble("timestamp", &timestamp));
   EXPECT_LT(0, timestamp);
@@ -109,8 +109,8 @@ TEST(Logging, CreatePerformanceLog) {
   ASSERT_TRUE(status.IsOk());
   ASSERT_EQ(2u, logs.size());
   ASSERT_EQ(2u, listeners.size());
-  ASSERT_STREQ("performance", logs[0]->GetType().c_str());
-  ASSERT_STREQ("browser", logs[1]->GetType().c_str());  // Always created.
+  ASSERT_EQ("performance", logs[0]->GetType());
+  ASSERT_EQ("browser", logs[1]->GetType());  // Always created.
 }
 
 TEST(Logging, CreateBrowserLogOff) {
@@ -124,7 +124,7 @@ TEST(Logging, CreateBrowserLogOff) {
   ASSERT_TRUE(status.IsOk());
   ASSERT_EQ(1u, logs.size());
   ASSERT_EQ(0u, listeners.size());
-  ASSERT_STREQ("browser", logs[0]->GetType().c_str());
+  ASSERT_EQ("browser", logs[0]->GetType());
 
   // Verify the created log is "OFF" -- drops all messages.
   logs[0]->AddEntry(Log::kError, "drop even errors");
@@ -143,7 +143,7 @@ TEST(Logging, IgnoreUnknownLogType) {
   EXPECT_TRUE(status.IsOk());
   ASSERT_EQ(1u, logs.size());
   ASSERT_EQ(1u, listeners.size());
-  ASSERT_STREQ("browser", logs[0]->GetType().c_str());
+  ASSERT_EQ("browser", logs[0]->GetType());
 }
 
 TEST(Logging, BrowserLogCreatedWithoutLoggingPrefs) {
@@ -155,7 +155,7 @@ TEST(Logging, BrowserLogCreatedWithoutLoggingPrefs) {
   EXPECT_TRUE(status.IsOk());
   ASSERT_EQ(1u, logs.size());
   ASSERT_EQ(1u, listeners.size());
-  ASSERT_STREQ("browser", logs[0]->GetType().c_str());
+  ASSERT_EQ("browser", logs[0]->GetType());
 
   // Verify the created "browser" log is "INFO" level.
   logs[0]->AddEntry(Log::kLog, "info message");