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

chrome/test/chromedriver/chrome/console_logger.cc

 // Copyright (c) 2013 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/chrome/console_logger.h"
 
 #include <sstream>
 
 #include "base/json/json_writer.h"
 #include "base/string_util.h"
 #include "base/values.h"
 #include "chrome/test/chromedriver/chrome/devtools_client.h"
 #include "chrome/test/chromedriver/chrome/log.h"
 #include "chrome/test/chromedriver/chrome/status.h"
 
 namespace {
 
 bool ConsoleLevelToLogLevel(const std::string& name, Log::Level *out_level) {
   const char* kConsoleLevelNames[] = {

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

This is named as a constant, but isn't const; it probably should be.

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

Done.

     "debug", "log", "warning", "error"
   };
 
   for (size_t i = 0; i < arraysize(kConsoleLevelNames); ++i) {
     if (name == kConsoleLevelNames[i]) {
       CHECK(Log::kDebug + i <= Log::kError);

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

CHECK_LE(Log::kDebug + i, Log::kError);

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

Done.

       *out_level = static_cast<Log::Level>(Log::kDebug + i);
       return true;
     }
   }
   return false;
 }
 
 }  // namespace
 
 ConsoleLogger::ConsoleLogger(Log* log)
     : log_(log) {}
 
 Status ConsoleLogger::OnConnected(DevToolsClient* client) {
   base::DictionaryValue params;
   return client->SendCommand("Console.enable", params);
 }
 
 void ConsoleLogger::OnEvent(
     DevToolsClient* client,
     const std::string& method,
     const base::DictionaryValue& params) {
   if (!StartsWithASCII(method, "Console.messageAdded", true))
     return;
 
   // 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;

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

Our style guide prefers nullptr over NULL for new code; is Chromium different?

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

I have codesearched for it specifically, and seems like Chromium uses NULL
pretty consistently.

[kkania, 2013/05/13 21:30:22]

Yep, use NULL

   if (params.GetDictionary("message", &message_dict)) {
     std::ostringstream message;

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

Google's style guide disallows stringstreams, preferring either stdio or our own
StrCat/StrAppend etc.  Does Chromium differ?

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

When I codesearched for how Chromium constructs strings from pieces, I
definitely saw std::stringstream used in many places. I can't seem to find
StrCat/StrAppend specifically in base/ header files.

[kkania, 2013/05/13 21:30:22]

Yes, we don't have StrCat and I don't think there's a rule against string
streams. You could save the pieces and put them together with StringPrintf,
which might actually be a bit easier to read.

[klm, 2013/05/14 03:34:47]

Done.

     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";
     }
 
     int line = -1;
@@ skipping 22 matching lines @@
         }
       }
     }
   }
 
   // Don't know how to format, log full JSON.
   std::string message_json;
   base::JSONWriter::Write(&params, &message_json);
   log_->AddEntry(Log::kWarning, message_json);
 }
+