Add media log messages to the main log

media/base/media_log.cc

Index: media/base/media_log.cc
diff --git a/media/base/media_log.cc b/media/base/media_log.cc
index a5188f4c38c60e9b4e9a7ddd2cd1526a77a90aae..3897f094a0c3f996c0d86b4ed445eb5deb94548a 100644
--- a/media/base/media_log.cc
+++ b/media/base/media_log.cc
@@ -8,6 +8,7 @@
 
 #include "base/atomic_sequence_num.h"
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 
 namespace media {
@@ -98,6 +99,74 @@ std::string MediaLog::PipelineStatusToString(PipelineStatus status) {
   return NULL;
 }
 
+std::string MediaLog::MediaEventToLogString(const MediaLogEvent& event) {
+  std::string param;
+  switch (event.type) {
+    case MediaLogEvent::WEBMEDIAPLAYER_CREATED:
+    case MediaLogEvent::WEBMEDIAPLAYER_DESTROYED:
+    case MediaLogEvent::PIPELINE_CREATED:
+    case MediaLogEvent::PIPELINE_DESTROYED:
+    case MediaLogEvent::PLAY:
+    case MediaLogEvent::PAUSE:
+    case MediaLogEvent::ENDED:
+    case MediaLogEvent::TEXT_ENDED:
+    case MediaLogEvent::BUFFERED_EXTENTS_CHANGED:
+    case MediaLogEvent::TOTAL_BYTES_SET:
+    case MediaLogEvent::NETWORK_ACTIVITY_SET:
+    case MediaLogEvent::PROPERTY_CHANGE:
+      return EventTypeToString(event.type);
+
+    case MediaLogEvent::LOAD:

[wolenetz, 2015/02/13 20:36:36]

I still think this method is too fragile to changes elsewhere in which params
are populated differently for each event type. Mitigations might include:
1) Tests
2) Implementing a custom ValueSerializer and iterate across all event params (if
the existing JSON impl is too wordy for logs).
3) Refactoring MediaLogEvent creation to also setup all the requisite parameters
per each event type *here*, not in the scattered callers of
MediaLog::CreateEvent. A further refactoring (IMO not required, but could be
nice) might make MediaLogEvent abstract with concrete subtypes for each specific
log event type.

At a bare minimum, DCHECK the retval of the various event.params.Get*() used
here.

I do not want this CL to introduce technical debt that will bite us later when
the log event params might diverge from current impl.

ISTM like a combination of (3) and the DCHECK might be the best way forward.

Alternatively, a more scoped-to-the-current-problem CL might not add this
method, and instead use the JSON serializer directly for the non-PIPELINE_ERROR
DVLOGs (to show full detail), and a very specific construction of pipeline error
media log message for PIPELINE_ERROR LOG(INFO)s.

[servolk, 2015/03/26 00:04:09]

Ok, I understand your concert. I've looked a bit more into this, and found a
more maintainable way to do this. We could use base::JSONWriter for most event
types, and special case just PIPELINE_ERROR events (since for those we can get a
pretty,human-readable message via media::MediaLog::PipelineStatusToString,
instead of cryptic numeric error code written by JSONWriter by default)

+      event.params.GetString("url", &param);
+      return EventTypeToString(event.type) + " url=" + param;
+
+    case MediaLogEvent::SEEK:
+      {
+        double seek_target = -1.0;
+        event.params.GetDouble("seek_target", &seek_target);
+        return EventTypeToString(event.type) + " seek_target=" +
+            base::DoubleToString(seek_target);
+      }
+
+    case MediaLogEvent::PIPELINE_STATE_CHANGED:
+      event.params.GetString("pipeline_state", &param);
+      return EventTypeToString(event.type) + " pipeline_state=" + param;
+
+    case MediaLogEvent::PIPELINE_ERROR:
+      {
+        int err;
+        event.params.GetInteger("pipeline_error", &err);
+        media::PipelineStatus status = static_cast<media::PipelineStatus>(err);
+        return EventTypeToString(event.type) + " " +
+            media::MediaLog::PipelineStatusToString(status);
+      }
+
+    case MediaLogEvent::VIDEO_SIZE_SET:
+      {
+        int width = 0;
+        int height = 0;
+        event.params.GetInteger("width", &width);
+        event.params.GetInteger("height", &height);
+        return EventTypeToString(event.type) + " " + base::IntToString(width) +
+            "x" + base::IntToString(height);
+      }
+
+    case MediaLogEvent::DURATION_SET:
+      {
+        double duration = -1.0;
+        event.params.GetDouble("duration", &duration);
+        return EventTypeToString(event.type) + " duration=" +
+            base::DoubleToString(duration);
+      }
+
+    case MediaLogEvent::MEDIA_SOURCE_ERROR:
+      event.params.GetString("error", &param);
+      return EventTypeToString(event.type) + " error=" + param;
+  }
+  NOTREACHED();
+  return NULL;
+}
+
 LogHelper::LogHelper(const LogCB& log_cb) : log_cb_(log_cb) {}
 
 LogHelper::~LogHelper() {