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

chrome/test/chromedriver/chrome/console_logger_unittest.cc

Index: chrome/test/chromedriver/chrome/console_logger_unittest.cc
diff --git a/chrome/test/chromedriver/chrome/console_logger_unittest.cc b/chrome/test/chromedriver/chrome/console_logger_unittest.cc
index deae61b0538db0e9cb486668d15183f4958e25b8..bde0c03a1e78ff3b6ced8e4b3d110396ba9df323 100644
--- a/chrome/test/chromedriver/chrome/console_logger_unittest.cc
+++ b/chrome/test/chromedriver/chrome/console_logger_unittest.cc
@@ -88,11 +88,11 @@ void FakeLog::AddEntry(
 }
 
 void ValidateLogEntry(LogEntry *entry,
-                      Log::Level expect_level,
-                      const char* expect_message) {
-  EXPECT_EQ(expect_level, entry->level);
+                      Log::Level expected_level,
+                      const char* 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 +102,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 +125,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());

[jdennett, 2013/05/23 03:44:24]

Optional: if you can use GMock, you can also write this as
  EXPECT_THAT(client.PopSentCommand(), IsEmpty());

I don't recommend using GMock to write mocks, but its matchers actually allow
for pretty clear assertions.

[klm, 2013/05/23 15:21:39]

Looks like gmock matchers are generally allowed, but no one is using IsEmpty
specifically, and doesn't look like there is IsEmpty defined in public headers.
Some of gmock's own tests define e.g. IsEmptyStringByRef, but I'd have to 1) do
this myself anywhere I use it, 2) the checks themselves would become longer than
just empty(), 3) it's an extra #include and an extra build dependency (which I
added and then reverted after finding out there is no public IsEmpty). Just not
worth it.

 
   base::DictionaryValue params1;  // All fields are set.
   ConsoleLogParams(&params1, "source1", "url1", "debug", 10, 1, "text1");

[jdennett, 2013/05/23 03:44:24]

Comment what 10 and 1 mean here.  See
https://www.corp.google.com/eng/doc/cppguide.xml#Implementation_Comments and in
particular the section under "nullptr/NULL, true/false, 1, 2, 3...".

Similarly for NULL etc. below.

[klm, 2013/05/23 15:21:39]

The style guide says "consider", not "always do". In this particular case I
would argue against line comments per arg, because IMHO the net effect would be
negative:

1. The test code would become substantially longer if I add 3-4 lines to each of
these calls.
2. This test-only function is defined right here, no need to flip files to look
it up.
3. If the signature changes, the effect is guaranteed limited to this file only.
4. If the signature changes, it's easier to edit the calls as they are now, than
if they were split.

BTW for shorter calls where the arg meaning is not obvious, in Java I sometimes
write inline e.g. generateOutput(/*indent=*/2). Should I interpret the style
guide as prohibiting this form in favor of split lines? E.g. in
performance_logger.cc:

    if (StartsWithASCII(method, kDomains[i_domain], /*case_sensitive=*/true)) {

vs.

    if (StartsWithASCII(method, kDomains[i_domain],
                        true))  // case_sensitive.

@@ -161,7 +161,7 @@ TEST(ConsoleLogger, ConsoleMessages) {
   params8.SetInteger("gaga", 8);
   client.TriggerEvent("Console.messageAdded", params8);
 
-  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");