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

base/logging.h

 // Copyright (c) 2012 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.
 
 #ifndef BASE_LOGGING_H_
 #define BASE_LOGGING_H_
 
 #include <stddef.h>
 
 #include <cassert>
@@ skipping 261 matching lines @@
 // Dialogs are not shown by default.
 BASE_EXPORT void SetShowErrorDialogs(bool enable_dialogs);
 
 // Sets the Log Assert Handler that will be used to notify of check failures.
 // The default handler shows a dialog box and then terminate the process,
 // however clients can use this function to override with their own handling
 // (e.g. a silent one for Unit Tests)
 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
 // Note: the log severities are used to index into the array of names,
 // see log_severity_names.
 const LogSeverity LOG_INFO = 0;
 const LogSeverity LOG_WARNING = 1;
 const LogSeverity LOG_ERROR = 2;
 const LogSeverity LOG_FATAL = 3;
 const LogSeverity LOG_NUM_SEVERITIES = 4;
@@ skipping 685 matching lines @@
 #elif NOTIMPLEMENTED_POLICY == 5
 #define NOTIMPLEMENTED() do {\
   static bool logged_once = false;\
   LOG_IF(ERROR, !logged_once) << NOTIMPLEMENTED_MSG;\
   logged_once = true;\
 } while(0);\
 EAT_STREAM_PARAMETERS
 #endif
 
 #endif  // BASE_LOGGING_H_