Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14591005:
C++ readability review from original change https://chromiumcodereview.appspot.com/14263024/ (Closed)
|
Created:
7 years, 7 months ago by klm Modified:
7 years, 6 months ago CC:
chromium-reviews, kkania Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionC++ readability review from original change https://chromiumcodereview.appspot.com/14263024/
Review bug: http://b/8772053
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204053
Patch Set 1 #Patch Set 2 : Update to head. #
Total comments: 45
Patch Set 3 : Review changes. #Patch Set 4 : STREQ->EQ for string equality checks in unit tests. #Patch Set 5 : Rewrite ConsoleLogger::OnEvent from stringstream to printf. #
Total comments: 36
Patch Set 6 : Review changes. #
Total comments: 2
Patch Set 7 : Merge to head. Mainly: OnEvent returns Status, Log::AddEntry takes explicit Time. #Patch Set 8 : Restored log.cc in chrome_tests.gypi. #
Total comments: 2
Patch Set 9 : Restored AddEntry with no timestamp; with timestamp is now AddEntryTimestamped. #Patch Set 10 : Remove unnecessary null timestamp handling. #Patch Set 11 : Fix types in CHECK comparisons. #Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:19: const char* kConsoleLevelNames[] = { This is named as a constant, but isn't const; it probably should be. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:25: CHECK(Log::kDebug + i <= Log::kError); CHECK_LE(Log::kDebug + i, Log::kError); https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:52: const base::DictionaryValue *message_dict = NULL; Our style guide prefers nullptr over NULL for new code; is Chromium different? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:54: std::ostringstream message; Google's style guide disallows stringstreams, preferring either stdio or our own StrCat/StrAppend etc. Does Chromium differ? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger.h (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.h:22: explicit ConsoleLogger(Log* log); Google's style guide requires comments on (essentially) all comments ("Every function declaration should have comments immediately preceding it that describe what the function does and how to use it."), and particularly requires documentation of ownership/lifetime for pointer arguments (and ideally also whether they're permitted to be null). https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.h:30: Log* log_; We also require documentation for member variables, see http://go/cstyle-variable-comments (under "Class Data Members"). https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:91: Log::Level expect_level, The name "expected_level" would be more idiomatic https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:92: const char* expect_message) { "expected_message". https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:95: EXPECT_STREQ(expect_message, entry->message.c_str()); Why not the simpler EXPECT_EQ(expect_message, entry->message); https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:105: if (NULL != source) google3 doesn't do "backwards" comparisons. Are these normal for Chromium? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:164: EXPECT_STREQ("", client.PopSentCommand().c_str()); // No other commands sent. Same question about EXPECT_EQ -- or even using EXPECT_TRUE(client.PopSentCommand().empty()); here. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/log.h (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/log.h:14: // Accepts log entries that have a level, timestamp, and a string message. I'd expect the docs to say what a "Log" _is_, not just something about what it accepts. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/log.h:26: // Log a message with an explicit timestamp. Our style guide calls for the declarative voice ("Logs a...") rather than the imperative ("Log a"). Is Chromium different? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/log.h:32: void AddEntry(Level level, const std::string& message); Mixing non-virtuals and virtuals in the same overload set is often questionable; clients using the derived interface (if there are any) will find the overloads hidden, unless the author of the derived class plays the "using Log::AddEntry;" game. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger.cc:21: }; Make these const. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger.cc:39: for (size_t i_cmd = 0; i_cmd < arraysize(kDomainEnableCommands); ++i_cmd) { For Google code we'd use range-based for here. I'm not familiar with Chrome's rules around C++11 features, but given that it builds on Windows I assume that it has some. Can you give me any pointers? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/performance_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:101: DictionaryValue* dict = 0; nullptr or NULL https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... File chrome/test/chromedriver/logging.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... chrome/test/chromedriver/logging.cc:36: CHECK(static_cast<size_t>(index) < arraysize(kLogLevelToWebDriverLevels)); Why not CHECK_GE and CHECK_LT? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... chrome/test/chromedriver/logging.cc:73: << entries_->GetSize() << " entries on destruction"; Align first << with the first one of the previous line. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... chrome/test/chromedriver/logging.cc:113: "logging level must be a string for log type: " + type); 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.)
Thanks for a quick turnaround! PTAL. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:19: const char* kConsoleLevelNames[] = { On 2013/05/13 17:56:49, jdennett wrote: > This is named as a constant, but isn't const; it probably should be. Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:25: CHECK(Log::kDebug + i <= Log::kError); On 2013/05/13 17:56:49, jdennett wrote: > CHECK_LE(Log::kDebug + i, Log::kError); Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:52: const base::DictionaryValue *message_dict = NULL; On 2013/05/13 17:56:49, jdennett wrote: > Our style guide prefers nullptr over NULL for new code; is Chromium different? I have codesearched for it specifically, and seems like Chromium uses NULL pretty consistently. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:54: std::ostringstream message; On 2013/05/13 17:56:49, jdennett wrote: > Google's style guide disallows stringstreams, preferring either stdio or our own > StrCat/StrAppend etc. Does Chromium differ? 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. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger.h (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.h:22: explicit ConsoleLogger(Log* log); On 2013/05/13 17:56:49, jdennett wrote: > Google's style guide requires comments on (essentially) all comments ("Every > function declaration should have comments immediately preceding it that describe > what the function does and how to use it."), and particularly requires > documentation of ownership/lifetime for pointer arguments (and ideally also > whether they're permitted to be null). Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.h:30: Log* log_; On 2013/05/13 17:56:49, jdennett wrote: > We also require documentation for member variables, see > http://go/cstyle-variable-comments (under "Class Data Members"). Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:91: Log::Level expect_level, On 2013/05/13 17:56:49, jdennett wrote: > The name "expected_level" would be more idiomatic Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:92: const char* expect_message) { On 2013/05/13 17:56:49, jdennett wrote: > "expected_message". Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:95: EXPECT_STREQ(expect_message, entry->message.c_str()); On 2013/05/13 17:56:49, jdennett wrote: > Why not the simpler > EXPECT_EQ(expect_message, entry->message); Wouldn't that compare pointers instead of strings? https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:105: if (NULL != source) On 2013/05/13 17:56:49, jdennett wrote: > google3 doesn't do "backwards" comparisons. Are these normal for Chromium? Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:164: EXPECT_STREQ("", client.PopSentCommand().c_str()); // No other commands sent. On 2013/05/13 17:56:49, jdennett wrote: > Same question about EXPECT_EQ -- or even using > EXPECT_TRUE(client.PopSentCommand().empty()); > here. Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/log.h (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/log.h:14: // Accepts log entries that have a level, timestamp, and a string message. On 2013/05/13 17:56:49, jdennett wrote: > I'd expect the docs to say what a "Log" _is_, not just something about what it > accepts. Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/log.h:26: // Log a message with an explicit timestamp. On 2013/05/13 17:56:49, jdennett wrote: > Our style guide calls for the declarative voice ("Logs a...") rather than the > imperative ("Log a"). Is Chromium different? Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/log.h:32: void AddEntry(Level level, const std::string& message); On 2013/05/13 17:56:49, jdennett wrote: > Mixing non-virtuals and virtuals in the same overload set is often questionable; > clients using the derived interface (if there are any) will find the overloads > hidden, unless the author of the derived class plays the "using Log::AddEntry;" > game. Which I did. But made it virtual anyway. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger.cc:21: }; On 2013/05/13 17:56:49, jdennett wrote: > Make these const. Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger.cc:39: for (size_t i_cmd = 0; i_cmd < arraysize(kDomainEnableCommands); ++i_cmd) { On 2013/05/13 17:56:49, jdennett wrote: > For Google code we'd use range-based for here. I'm not familiar with Chrome's > rules around C++11 features, but given that it builds on Windows I assume that > it has some. Can you give me any pointers? AFAIK Chromium code is not yet allowed to use C++11. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/performance_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:101: DictionaryValue* dict = 0; On 2013/05/13 17:56:49, jdennett wrote: > nullptr or NULL Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... File chrome/test/chromedriver/logging.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... chrome/test/chromedriver/logging.cc:36: CHECK(static_cast<size_t>(index) < arraysize(kLogLevelToWebDriverLevels)); On 2013/05/13 17:56:49, jdennett wrote: > Why not CHECK_GE and CHECK_LT? Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... chrome/test/chromedriver/logging.cc:73: << entries_->GetSize() << " entries on destruction"; On 2013/05/13 17:56:49, jdennett wrote: > Align first << with the first one of the previous line. Done. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/l... chrome/test/chromedriver/logging.cc:113: "logging level must be a string for log type: " + type); On 2013/05/13 17:56:49, jdennett wrote: > 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.) Offhand doesn't look like there is StrCat in Chromium.
Next step for me: remembering/working out how to see the current code in this code review tool... https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger_unittest.cc:95: EXPECT_STREQ(expect_message, entry->message.c_str()); On 2013/05/13 20:35:57, klm wrote: > On 2013/05/13 17:56:49, jdennett wrote: > > Why not the simpler > > EXPECT_EQ(expect_message, entry->message); > > Wouldn't that compare pointers instead of strings? No ;) Slightly more helpfully: entry->message is not a pointer.
On 2013/05/13 20:39:10, jdennett wrote: > Next step for me: remembering/working out how to see the current code in this > code review tool... > > https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... > File chrome/test/chromedriver/chrome/console_logger_unittest.cc (right): > > https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... > chrome/test/chromedriver/chrome/console_logger_unittest.cc:95: > EXPECT_STREQ(expect_message, entry->message.c_str()); > On 2013/05/13 20:35:57, klm wrote: > > On 2013/05/13 17:56:49, jdennett wrote: > > > Why not the simpler > > > EXPECT_EQ(expect_message, entry->message); > > > > Wouldn't that compare pointers instead of strings? > > No ;) > > Slightly more helpfully: entry->message is not a pointer. Hm interesting, you're right:) I do remember getting compile errors when I was trying to do EQ on strings, and it involved macros and templates, so I didn't track it down and just saw that the code around me uses STREQ and switched to that. But yeah it works, so I now switched to EQ in all the unittests in this CL.
https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:52: const base::DictionaryValue *message_dict = NULL; On 2013/05/13 20:35:57, klm wrote: > On 2013/05/13 17:56:49, jdennett wrote: > > Our style guide prefers nullptr over NULL for new code; is Chromium different? > > I have codesearched for it specifically, and seems like Chromium uses NULL > pretty consistently. Yep, use NULL https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:54: std::ostringstream message; On 2013/05/13 20:35:57, klm wrote: > On 2013/05/13 17:56:49, jdennett wrote: > > Google's style guide disallows stringstreams, preferring either stdio or our > own > > StrCat/StrAppend etc. Does Chromium differ? > > 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. 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. https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/performance_logger.cc:39: for (size_t i_cmd = 0; i_cmd < arraysize(kDomainEnableCommands); ++i_cmd) { On 2013/05/13 20:35:57, klm wrote: > On 2013/05/13 17:56:49, jdennett wrote: > > For Google code we'd use range-based for here. I'm not familiar with Chrome's > > rules around C++11 features, but given that it builds on Windows I assume that > > it has some. Can you give me any pointers? > > AFAIK Chromium code is not yet allowed to use C++11. Correct
https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... File chrome/test/chromedriver/chrome/console_logger.cc (right): https://codereview.chromium.org/14591005/diff/9001/chrome/test/chromedriver/c... chrome/test/chromedriver/chrome/console_logger.cc:54: std::ostringstream message; On 2013/05/13 21:30:22, kkania wrote: > On 2013/05/13 20:35:57, klm wrote: > > On 2013/05/13 17:56:49, jdennett wrote: > > > Google's style guide disallows stringstreams, preferring either stdio or our > > own > > > StrCat/StrAppend etc. Does Chromium differ? > > > > 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. > > 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. Done.
ping...
On 2013/05/20 18:36:31, klm wrote: > ping... Hi. Sorry, I've not forgotten you. It's just that Chromium style is now pretty far removed from Google style, and that's causing me to scratch my head a lot -- it's not specific to this code, it's just that the notion that we have one common style has died, and so the basis for relating Chromium code to google3-readability makes rather less sense now. I'll get back to this today/tomorrow and see if we can salvage it.
On 2013/05/20 18:39:08, jdennett wrote: > On 2013/05/20 18:36:31, klm wrote: > > ping... > > Hi. Sorry, I've not forgotten you. It's just that Chromium style is now pretty > far removed from Google style, and that's causing me to scratch my head a lot -- > it's not specific to this code, it's just that the notion that we have one > common style has died, and so the basis for relating Chromium code to > google3-readability makes rather less sense now. I'll get back to this > today/tomorrow and see if we can salvage it. As far as this particular bit of code goes, there were a couple of formal differences -- like strstreams, but nothing major that I can see. If this were a google3-proper review, I would have trivially switched to those other APIs. Thanks for doing the review!
I don't see any major problems here; some protected/public data should be less visible, which might cause some ripples, but everything else is minor/local. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/console_logger.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger.cc:25: CHECK(Log::kDebug + i <= Log::kError); CHECK_LE will show the problematic value on failure. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/console_logger.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger.h:23: // The log is owned elsewhere and must not be NULL. Optional: I try to reserve all-caps NULL for the name of the macro from C. The adjective in plain text is just "null". https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/console_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:22: virtual ~FakeDevToolsClient() {} I'd normally recommend deleting this as it does nothing. If Chromium convention is to be cluttered, go with the flow. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:52: listener_ = listener; I'd expect "AddListener" to add a listener, not to have just a single listener. Would it be too painful to throw in a std::set<DevToolsEventListener*> to avoid violating the (admittedly vague) interface? https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:62: DevToolsEventListener* listener_; Document data members. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:129: EXPECT_TRUE(client.PopSentCommand().empty()); 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. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:132: ConsoleLogParams(¶ms1, "source1", "url1", "debug", 10, 1, "text1"); 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. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/log.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/log.h:33: virtual void AddEntry(Level level, const std::string& message); It's also strange to have overload sets of virtual functions! Generally a virtual function should not be overloaded, and should be the single customization point. I can live with this as is, but it's a touch awkward. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.cc:26: if (StartsWithASCII(method, kDomains[i_domain], true)) StartsWithASCII is an awful name for that, but that's beyond the scope of this review! https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.cc:26: if (StartsWithASCII(method, kDomains[i_domain], true)) Comment what "true" means above. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.h:24: // The log is owned elsewhere and must not be NULL. s/NULL/null/ https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:83: ScopedVector<LogEntry> entries; We generally disallow public data except in dumb "struct" classes; classes with bases and virtual functions don't qualify. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:95: if (NULL == value) { value == NULL https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:105: return scoped_ptr<DictionaryValue>(NULL); FYI: When Chrome gets C++11, use unique_ptr<T> and just write "return nullptr;". The type system knows that nullptr is null, so it's safe to implicitly convert to most smart pointer types. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:112: const char* expected_method) { Optional: In general I'd recommend just passing std::string here, unless you have some particular reason for wanting to play with pointers. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/logging.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/logging.cc:56: CHECK(WebDriverLog::kWdAll + i <= WebDriverLog::kWdOff); CHECK_LE https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/logging.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/logging.h:42: // Returns this log's type. That much I could guess from the name ;-) An example would be good, or a reference to what "type" means here. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/logging_unittest.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/logging_unittest.cc:15: static const char* kAllWdLevels[] = { +const
https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/console_logger.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger.cc:25: CHECK(Log::kDebug + i <= Log::kError); On 2013/05/23 03:44:24, jdennett wrote: > CHECK_LE will show the problematic value on failure. Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/console_logger.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger.h:23: // The log is owned elsewhere and must not be NULL. On 2013/05/23 03:44:24, jdennett wrote: > Optional: I try to reserve all-caps NULL for the name of the macro from C. The > adjective in plain text is just "null". Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/console_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:22: virtual ~FakeDevToolsClient() {} On 2013/05/23 03:44:24, jdennett wrote: > I'd normally recommend deleting this as it does nothing. If Chromium convention > is to be cluttered, go with the flow. Not sure if there is an official Chromium-wide convention, but fakes in other unit tests around here do this, so I'd leave it, since looks like it's at least this team's convention. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:52: listener_ = listener; On 2013/05/23 03:44:24, jdennett wrote: > I'd expect "AddListener" to add a listener, not to have just a single listener. > > Would it be too painful to throw in a std::set<DevToolsEventListener*> to avoid > violating the (admittedly vague) interface? It's just a fake, with the use limited to this unit test only. Also fakes should be as simple/dumb as possible to serve their limited purpose, otherwise the unit tests gradually become complex enough to need their own tests. Added CHECK(!listener_) here and in performance_logger_unittest, also added missing NULL initialization. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:62: DevToolsEventListener* listener_; On 2013/05/23 03:44:24, jdennett wrote: > Document data members. Done here and in performance_logger_unittest. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:129: EXPECT_TRUE(client.PopSentCommand().empty()); On 2013/05/23 03:44:24, jdennett wrote: > 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. 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. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/console_logger_unittest.cc:132: ConsoleLogParams(¶ms1, "source1", "url1", "debug", 10, 1, "text1"); On 2013/05/23 03:44:24, jdennett wrote: > 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. 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. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/log.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/log.h:33: virtual void AddEntry(Level level, const std::string& message); On 2013/05/23 03:44:24, jdennett wrote: > It's also strange to have overload sets of virtual functions! > > Generally a virtual function should not be overloaded, and should be the single > customization point. > > I can live with this as is, but it's a touch awkward. The idea was that callers can add a log entry with an explicit timestamp, or implicitly stamped as "now". This has caused me more trouble than it's worth -- C++ is a lot less overload-friendly than Java, so I now removed the overload and everyone must pass a timestamp, although it's allowed to be null. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.cc:26: if (StartsWithASCII(method, kDomains[i_domain], true)) On 2013/05/23 03:44:24, jdennett wrote: > StartsWithASCII is an awful name for that, but that's beyond the scope of this > review! Indeed both:) https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.cc:26: if (StartsWithASCII(method, kDomains[i_domain], true)) On 2013/05/23 03:44:24, jdennett wrote: > Comment what "true" means above. Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.h:24: // The log is owned elsewhere and must not be NULL. On 2013/05/23 03:44:24, jdennett wrote: > s/NULL/null/ Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger_unittest.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:83: ScopedVector<LogEntry> entries; On 2013/05/23 03:44:24, jdennett wrote: > We generally disallow public data except in dumb "struct" classes; classes with > bases and virtual functions don't qualify. Even in a test-only fake? I really think the getter detracts more than it adds here, but I'm not going to argue. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:95: if (NULL == value) { On 2013/05/23 03:44:24, jdennett wrote: > value == NULL Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:105: return scoped_ptr<DictionaryValue>(NULL); On 2013/05/23 03:44:24, jdennett wrote: > FYI: When Chrome gets C++11, use unique_ptr<T> and just write "return nullptr;". > The type system knows that nullptr is null, so it's safe to implicitly convert > to most smart pointer types. Acknowledged. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger_unittest.cc:112: const char* expected_method) { On 2013/05/23 03:44:24, jdennett wrote: > Optional: In general I'd recommend just passing std::string here, unless you > have some particular reason for wanting to play with pointers. Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/logging.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/logging.cc:56: CHECK(WebDriverLog::kWdAll + i <= WebDriverLog::kWdOff); On 2013/05/23 03:44:24, jdennett wrote: > CHECK_LE Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/logging.h (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/logging.h:42: // Returns this log's type. On 2013/05/23 03:44:24, jdennett wrote: > That much I could guess from the name ;-) > > An example would be good, or a reference to what "type" means here. Done. https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... File chrome/test/chromedriver/logging_unittest.cc (right): https://codereview.chromium.org/14591005/diff/30001/chrome/test/chromedriver/... chrome/test/chromedriver/logging_unittest.cc:15: static const char* kAllWdLevels[] = { On 2013/05/23 03:44:24, jdennett wrote: > +const Done.
ping...
lgtm Re fakes being as simple as possible: I'd agree, but I'd also say that they should not be simpler -- and in particular they should generally not violate LSP, though I can accept the compromise you've used here where they fail loudly with an assertion violation if they are unable to implement the interface (e.g., if a test attempts to install multiple listeners on the same object simultaneously). This code looks good to me for readability purposes. https://codereview.chromium.org/14591005/diff/43001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/43001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.cc:28: true)) // case_sensitive. FYI: Sometimes we prefer true /* case_sensitive */)); to indicate that the comment binds directly to the argument.
Merge to head. Mainly: OnEvent returns Status, Log::AddEntry takes explicit Time. https://codereview.chromium.org/14591005/diff/43001/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/performance_logger.cc (right): https://codereview.chromium.org/14591005/diff/43001/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/performance_logger.cc:28: true)) // case_sensitive. On 2013/06/03 13:51:34, jdennett wrote: > FYI: Sometimes we prefer > true /* case_sensitive */)); > to indicate that the comment binds directly to the argument. Done.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/14591005/diff/48002/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/log.h (right): https://codereview.chromium.org/14591005/diff/48002/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/log.h:27: virtual void AddEntry(const base::Time& timestamp, Instead of this, I would prefer two functions: AddEntry(level, message) and AddEntryWithTime(time, level, message)
On Mon, Jun 3, 2013 at 10:44 AM, <kkania@chromium.org> wrote: > > https://codereview.chromium.org/14591005/diff/48002/chrome/test/chromedriver/... > File chrome/test/chromedriver/chrome/log.h (right): > > https://codereview.chromium.org/14591005/diff/48002/chrome/test/chromedriver/... > chrome/test/chromedriver/chrome/log.h:27: virtual void AddEntry(const > base::Time& timestamp, > Instead of this, I would prefer two functions: > > AddEntry(level, message) > and > AddEntryWithTime(time, level, message) That certainly works for me; AddEntryWithTime being the virtual one, and AddEntry a non-virtual that just forwards to it.
https://codereview.chromium.org/14591005/diff/48002/chrome/test/chromedriver/... File chrome/test/chromedriver/chrome/log.h (right): https://codereview.chromium.org/14591005/diff/48002/chrome/test/chromedriver/... chrome/test/chromedriver/chrome/log.h:27: virtual void AddEntry(const base::Time& timestamp, On 2013/06/03 17:44:11, kkania wrote: > Instead of this, I would prefer two functions: > > AddEntry(level, message) > and > AddEntryWithTime(time, level, message) Done. Per chat, AddEntryTimestamped.
lgtm after updating log.h comment (to remove is_null check)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klm@google.com/14591005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klm@google.com/14591005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klm@google.com/14591005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klm@google.com/14591005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klm@google.com/14591005/60001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/klm@google.com/14591005/83002
Message was sent while issue was closed.
Change committed as 204053 |
