Allow multiple logging::LogMessage{Handler,Listener}s

base/test/mock_log.cc

Index: base/test/mock_log.cc
diff --git a/base/test/mock_log.cc b/base/test/mock_log.cc
index a09000d8ed7f9e3080ff06a64cb060c76fa591f6..68eb68b2c7582fa6ed6e8abcbe16c3f684e133d9 100644
--- a/base/test/mock_log.cc
+++ b/base/test/mock_log.cc
@@ -9,7 +9,6 @@ namespace test {
 
 // static
 MockLog* MockLog::g_instance_ = nullptr;
-Lock MockLog::g_lock;
 
 MockLog::MockLog() : is_capturing_logs_(false) {
 }

[Dan Beam, 2016/07/19 19:07:23]

why can't the entirety of this file basically be:

MockLog::MockLog() : is_capturing_logs_(false) {}

MockLog::~MockLog() {
  if (is_capturing_logs_)
    StopCapturingLogs();
}

MockLog::StartCapturingLogs() {
  RAW_CHECK(!is_capturing_logs_);
  logging::AddLogMessageListener(Log);
  is_capturing_logs_ = true;
}

MockLog::StopCapturingLogs() {
  RAW_CHECK(is_capturing_logs_);
  logging::AddLogMessageListener(Log);
  is_capturing_logs_ = false;
}

why do we need g_instance_ or g_lock at all?

[Dan Beam, 2016/07/19 19:07:49]

Remove*

[wychen, 2016/07/19 20:56:22]

I think the reason is that the {Add,Remove}LogMessageListener
can only take static methods, but MOCK_METHOD(Log) is not
static. Therefore, a static wrapper method is introduced to
bridge the signals.

It seems gmock doesn't support static mocked method, and
base::Bind() couldn't be used because LogMessageListener is
a raw function pointer instead of Callback class.

Getting rid of g_instance_ and g_lock would be much nicer,
and could support multiple MockLog instances. Not sure how
useful it is though.

@@ -21,8 +20,6 @@ MockLog::~MockLog() {
 }
 
 void MockLog::StartCapturingLogs() {
-  AutoLock scoped_lock(g_lock);
-
   // We don't use CHECK(), which can generate a new LOG message, and
   // thus can confuse MockLog objects or other registered
   // LogSinks.
@@ -31,13 +28,10 @@ void MockLog::StartCapturingLogs() {
 
   is_capturing_logs_ = true;
   g_instance_ = this;
-  previous_handler_ = logging::GetLogMessageHandler();
-  logging::SetLogMessageHandler(LogMessageHandler);
+  logging::AddLogMessageListener(LogMessageListener);
 }
 
 void MockLog::StopCapturingLogs() {
-  AutoLock scoped_lock(g_lock);
-
   // We don't use CHECK(), which can generate a new LOG message, and
   // thus can confuse MockLog objects or other registered
   // LogSinks.
@@ -45,22 +39,16 @@ void MockLog::StopCapturingLogs() {
   RAW_CHECK(g_instance_ == this);
 
   is_capturing_logs_ = false;
-  logging::SetLogMessageHandler(previous_handler_);
+  logging::RemoveLogMessageListener(LogMessageListener);
   g_instance_ = nullptr;
 }
 
 // static
-bool MockLog::LogMessageHandler(int severity,
-                                const char* file,
-                                int line,
-                                size_t message_start,
-                                const std::string& str) {
-  // gMock guarantees thread-safety for calling a mocked method
-  // (https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#using-google-mock-and-threads)
-  // but we also need to make sure that Start/StopCapturingLogs are synchronized
-  // with LogMessageHandler.
-  AutoLock scoped_lock(g_lock);
-
+void MockLog::LogMessageListener(int severity,
+                                 const char* file,
+                                 int line,
+                                 size_t message_start,
+                                 const std::string& str) {
   return g_instance_->Log(severity, file, line, message_start, str);
 }