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

chrome/test/chromedriver/logging.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 "chrome/test/chromedriver/logging.h"
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/time.h"
 #include "chrome/test/chromedriver/capabilities.h"
@@ skipping 15 matching lines @@
 WebDriverLog::WebDriverLevel kLogLevelToWebDriverLevels[] = {
   WebDriverLog::kWdDebug,  // kDebug
   WebDriverLog::kWdInfo,  // kLog
   WebDriverLog::kWdWarning,  // kWarning
   WebDriverLog::kWdSevere  // kError
 };
 
 WebDriverLog::WebDriverLevel LogLevelToWebDriverLevel(Log::Level level) {
   const int index = level - Log::kDebug;
   CHECK(index >= 0);
   CHECK(static_cast<size_t>(index) < arraysize(kLogLevelToWebDriverLevels));

[jdennett, 2013/05/13 17:56:49]

Why not CHECK_GE and CHECK_LT?

[klm, 2013/05/13 20:35:57]

Done.

   return kLogLevelToWebDriverLevels[index];
 }
 
 std::string GetWebDriverLevelName(
     const WebDriverLog::WebDriverLevel level) {
   const int index = level - WebDriverLog::kWdAll;
   CHECK(index >= 0);
   CHECK(static_cast<size_t>(index) < arraysize(kWebDriverLevelNames));
   return kWebDriverLevelNames[index];
 }
@@ skipping 16 matching lines @@
 WebDriverLog::WebDriverLog(
     const std::string& type, WebDriverLog::WebDriverLevel min_wd_level)
     : type_(type),
       min_wd_level_(min_wd_level),
       entries_(new base::ListValue()) {
   VLOG(1) << "Log(" << type_ << ", " << min_wd_level_ << ")";
 }
 
 WebDriverLog::~WebDriverLog() {
   VLOG(1) << "Log type '" << type_ << "' lost "
       << entries_->GetSize() << " entries on destruction";

[jdennett, 2013/05/13 17:56:49]

Align first << with the first one of the previous line.

[klm, 2013/05/13 20:35:57]

Done.

 }
 
 const std::string& WebDriverLog::GetType() {
   return type_;
 }
 
 void WebDriverLog::AddEntry(const base::Time& time,
                             Log::Level level,
                             const std::string& message) {
   const WebDriverLog::WebDriverLevel wd_level = LogLevelToWebDriverLevel(level);
@@ skipping 19 matching lines @@
   ScopedVector<DevToolsEventListener> listeners;
   WebDriverLog::WebDriverLevel browser_log_level = WebDriverLog::kWdInfo;
 
   if (capabilities.logging_prefs) {
     for (DictionaryValue::Iterator pref(*capabilities.logging_prefs);
          !pref.IsAtEnd(); pref.Advance()) {
       const std::string type = pref.key();
       std::string level_name;
       if (!pref.value().GetAsString(&level_name)) {
         return Status(kUnknownError,
                       "logging level must be a string for log type: " + type);

[jdennett, 2013/05/13 17:56:49]

StrCat would have one small readability advantage here: I wouldn't have needed
to double-check that "type" wasn't integral.  (If type is an int, the code will
likely still compile, but with "+" will do the wrong thing.)

[klm, 2013/05/13 20:35:57]

Offhand doesn't look like there is StrCat in Chromium.

       }
       WebDriverLog::WebDriverLevel level = WebDriverLog::kWdOff;
       if (!WebDriverLog::NameToLevel(level_name, &level)) {
         return Status(kUnknownError,
                       "invalid log level \"" + level_name +
                       "\" for type: " + type);
       }
       if ("performance" == type) {
         if (WebDriverLog::kWdOff != level) {
           WebDriverLog* log = new WebDriverLog(type, WebDriverLog::kWdAll);
@@ skipping 14 matching lines @@
   WebDriverLog* browser_log = new WebDriverLog("browser", browser_log_level);
   devtools_logs.push_back(browser_log);
   // If the level is OFF, don't even bother listening for DevTools events.
   if (browser_log_level != WebDriverLog::kWdOff)
     listeners.push_back(new ConsoleLogger(browser_log));
 
   out_devtools_logs->swap(devtools_logs);
   out_listeners->swap(listeners);
   return Status(kOk);
 }
+