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

base/logging.h

Index: base/logging.h
diff --git a/base/logging.h b/base/logging.h
index 3dde7027a61a332814ec808788670e3965851387..19d2cda4f65336ab4db7fb3e0eea4a86c6ed57ea 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -279,14 +279,44 @@ BASE_EXPORT void SetShowErrorDialogs(bool enable_dialogs);
 typedef void (*LogAssertHandlerFunction)(const std::string& str);
 BASE_EXPORT void SetLogAssertHandler(LogAssertHandlerFunction handler);
 
-// Sets the Log Message Handler that gets passed every log message before
-// it's sent to other log destinations (if any).
-// Returns true to signal that it handled the message and the message
-// should not be sent to other log destinations.
-typedef bool (*LogMessageHandlerFunction)(int severity,
-    const char* file, int line, size_t message_start, const std::string& str);
-BASE_EXPORT void SetLogMessageHandler(LogMessageHandlerFunction handler);
-BASE_EXPORT LogMessageHandlerFunction GetLogMessageHandler();
+// LogMessageHandler is the callback interface for log message handling.
+// OnMessage() is called for every log message before it's sent to other log
+// destinations (if any), but after LogMessageListener.
+// Return true in OnMessage() to signal that it handled the message and the
+// message should not be sent to other log destinations, including other
+// handlers added before the current one.
+class BASE_EXPORT LogMessageHandler {
+ public:
+  virtual ~LogMessageHandler();
+  virtual bool OnMessage(int severity,
+                         const char* file,
+                         int line,
+                         size_t message_start,
+                         const std::string& str) = 0;
+
+ protected:
+  LogMessageHandler();
+};
+
+BASE_EXPORT size_t LogMessageHandlerCountForTesting();
+
+// LogMessageListener is similar to LogMessageHandler except that this
+// interface doesn't support message hijacking, and is preferred over
+// LogMessageHandler if hijacking is not needed.
+class BASE_EXPORT LogMessageListener {

[Mark Mentovai, 2016/11/14 14:40:43]

The difference between Handler and Listener will be confusing in practice. I
don’t believe that we need to support both of them, although I haven’t examined
this entire change to weigh in on each use site.

[brettw, 2016/11/14 22:01:31]

I'm inclined to agree. What do you see as the cons of having one one type of
thing?

[wychen, 2016/11/15 23:52:27]

The problem we want to solve in this CL is that many agents are trying to
receive and hijack the log messages. Not overriding one another is the very
first step, but it might not be enough.

The model I had in mind is that the handlers are pushed into a stack, so that if
a message is hijacked by a handler, handlers added before it don't receive the
message, but handlers added after it do. In short, recently added handlers have
higher priority. In this case, two-tier system is needed since listeners are
guaranteed to receive the message, even if one of the handlers hijacks it.

Now looking at this CL with fresh eyes, I think we probably don't want handlers
to be able to hijack messages from one another. We can simplify the model by
making all handlers have the same priority, call all of them, and hijack if any
handlers says so. In this case, we don't need listeners.

On the other hand, message handlers with no side effect (listeners, essentially)
is cleaner, but alas there are a few hijacking handlers. I suspect most of them
can be (relatively) safely converted to listeners, but would like to play safe
and do it in stages. This CL preserves the original behavior as much as
possible.

The hijacking handlers are:
- safe_browsing_database_unittest.cc: http://crbug.com/56448
- chrome/test/chromedriver/logging.cc: no apparant reason
- components/proximity_auth/logging/logging_unittest.cc: avoid flooding stdout

These two are gone:
- installation_validation_helper.cc
- installation_validator_unittest.cc

This CL having two types is the intermediate state, and if this CL sticks, we
can gradually deprecate the hijacking one. What do you think about this
approach?

[wychen, 2016/11/18 21:14:00]

Only keep listeners now.

+ public:
+  virtual ~LogMessageListener();
+  virtual void OnMessage(int severity,
+                         const char* file,
+                         int line,
+                         size_t message_start,
+                         const std::string& str) = 0;
+
+ protected:
+  LogMessageListener();
+};
+
+BASE_EXPORT size_t LogMessageListenerCountForTesting();
 
 typedef int LogSeverity;
 const LogSeverity LOG_VERBOSE = -1;  // This is level 1 verbosity