Blimp: change logger to use switch/case for types.

blimp/common/logging.cc

Index: blimp/common/logging.cc
diff --git a/blimp/common/logging.cc b/blimp/common/logging.cc
index 174c2b4554a1111de459d0fd429f0a662d1d9f7d..eb2277995eb5275363cc97aa9fed4d26672af0e6 100644
--- a/blimp/common/logging.cc
+++ b/blimp/common/logging.cc
@@ -6,6 +6,7 @@
 
 #include <iostream>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/format_macros.h"
@@ -21,8 +22,7 @@
 namespace blimp {
 namespace {
 
-static base::LazyInstance<BlimpMessageLogger> g_logger =
-    LAZY_INSTANCE_INITIALIZER;
+typedef std::vector<std::pair<std::string, std::string>> LogFields;

[Wez, 2016/06/03 00:07:54]

nit: Style-guide says using using instead of typedef now.

[Kevin M, 2016/06/07 00:05:50]

Done.

 
 // The AddField() suite of functions are used to convert KV pairs with
 // arbitrarily typed values into string/string KV pairs for logging.
@@ -69,260 +69,233 @@ void AddField(const std::string& key, const T& value, LogFields* output) {
 // fields from BlimpMessages.
 
 // Logs fields from COMPOSITOR messages.
-class CompositorLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    AddField("render_widget_id", message.compositor().render_widget_id(),
-             output);
-  }
-};
+void ExtractCompositorMessageFields(const BlimpMessage& message,
+                                    LogFields* output) {
+  AddField("render_widget_id", message.compositor().render_widget_id(), output);
+}
 
 // Logs fields from INPUT messages.
-class InputLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    AddField("render_widget_id", message.input().render_widget_id(), output);
-    AddField("timestamp_seconds", message.input().timestamp_seconds(), output);
-    switch (message.input().type()) {
-      case InputMessage::Type_GestureScrollBegin:
-        AddField("subtype", "GestureScrollBegin", output);
-        break;
-      case InputMessage::Type_GestureScrollEnd:
-        AddField("subtype", "GestureScrollEnd", output);
-        break;
-      case InputMessage::Type_GestureScrollUpdate:
-        AddField("subtype", "GestureScrollUpdate", output);
-        break;
-      case InputMessage::Type_GestureFlingStart:
-        AddField("subtype", "GestureFlingStart", output);
-        break;
-      case InputMessage::Type_GestureFlingCancel:
-        AddField("subtype", "GestureFlingCancel", output);
-        AddField("prevent_boosting",
-                 message.input().gesture_fling_cancel().prevent_boosting(),
-                 output);
-        break;
-      case InputMessage::Type_GestureTap:
-        AddField("subtype", "GestureTap", output);
-        break;
-      case InputMessage::Type_GesturePinchBegin:
-        AddField("subtype", "GesturePinchBegin", output);
-        break;
-      case InputMessage::Type_GesturePinchEnd:
-        AddField("subtype", "GesturePinchEnd", output);
-        break;
-      case InputMessage::Type_GesturePinchUpdate:
-        AddField("subtype", "GesturePinchUpdate", output);
-        break;
-      default:  // unknown
-        break;
-    }
+void ExtractInputMessageFields(const BlimpMessage& message, LogFields* output) {
+  AddField("render_widget_id", message.input().render_widget_id(), output);
+  AddField("timestamp_seconds", message.input().timestamp_seconds(), output);
+  switch (message.input().type()) {
+    case InputMessage::Type_GestureScrollBegin:
+      AddField("subtype", "GestureScrollBegin", output);
+      break;
+    case InputMessage::Type_GestureScrollEnd:
+      AddField("subtype", "GestureScrollEnd", output);
+      break;
+    case InputMessage::Type_GestureScrollUpdate:
+      AddField("subtype", "GestureScrollUpdate", output);
+      break;
+    case InputMessage::Type_GestureFlingStart:
+      AddField("subtype", "GestureFlingStart", output);
+      break;
+    case InputMessage::Type_GestureFlingCancel:
+      AddField("subtype", "GestureFlingCancel", output);
+      AddField("prevent_boosting",
+               message.input().gesture_fling_cancel().prevent_boosting(),
+               output);
+      break;
+    case InputMessage::Type_GestureTap:
+      AddField("subtype", "GestureTap", output);
+      break;
+    case InputMessage::Type_GesturePinchBegin:
+      AddField("subtype", "GesturePinchBegin", output);
+      break;
+    case InputMessage::Type_GesturePinchEnd:
+      AddField("subtype", "GesturePinchEnd", output);
+      break;
+    case InputMessage::Type_GesturePinchUpdate:
+      AddField("subtype", "GesturePinchUpdate", output);
+      break;
+    default:  // unknown

[Wez, 2016/06/03 00:07:54]

Replace the default's, here and below, with explicit UNKNOWN case, like we have
at the top-level, so we can trap missing handlers for new message types too.

[Kevin M, 2016/06/07 00:05:50]

Done.

+      break;
   }
-};
+}
 
 // Logs fields from NAVIGATION messages.
-class NavigationLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    switch (message.navigation().type()) {
-      case NavigationMessage::NAVIGATION_STATE_CHANGED:
-        AddField("subtype", "NAVIGATION_STATE_CHANGED", output);
-        if (message.navigation().navigation_state_changed().has_url()) {
-          AddField("url", message.navigation().navigation_state_changed().url(),
-                   output);
-        }
-        if (message.navigation().navigation_state_changed().has_favicon()) {
-          AddField(
-              "favicon_size",
-              message.navigation().navigation_state_changed().favicon().size(),
-              output);
-        }
-        if (message.navigation().navigation_state_changed().has_title()) {
-          AddField("title",
-                   message.navigation().navigation_state_changed().title(),
-                   output);
-        }
-        if (message.navigation().navigation_state_changed().has_loading()) {
-          AddField("loading",
-                   message.navigation().navigation_state_changed().loading(),
-                   output);
-        }
-        break;
-      case NavigationMessage::LOAD_URL:
-        AddField("subtype", "LOAD_URL", output);
-        AddField("url", message.navigation().load_url().url(), output);
-        break;
-      case NavigationMessage::GO_BACK:
-        AddField("subtype", "GO_BACK", output);
-        break;
-      case NavigationMessage::GO_FORWARD:
-        AddField("subtype", "GO_FORWARD", output);
-        break;
-      case NavigationMessage::RELOAD:
-        AddField("subtype", "RELOAD", output);
-        break;
-      default:
-        break;
-    }
-  }
-};
-
-// Logs fields from PROTOCOL_CONTROL messages.
-class ProtocolControlLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    switch (message.protocol_control().connection_message_case()) {
-      case ProtocolControlMessage::kStartConnection:
-        AddField("subtype", "START_CONNECTION", output);
-        AddField("client_token",
-                 message.protocol_control().start_connection().client_token(),
+void ExtractNavigationMessageFields(const BlimpMessage& message,
+                                    LogFields* output) {
+  switch (message.navigation().type()) {
+    case NavigationMessage::NAVIGATION_STATE_CHANGED:
+      AddField("subtype", "NAVIGATION_STATE_CHANGED", output);
+      if (message.navigation().navigation_state_changed().has_url()) {
+        AddField("url", message.navigation().navigation_state_changed().url(),
                  output);
+      }
+      if (message.navigation().navigation_state_changed().has_favicon()) {
         AddField(
-            "protocol_version",
-            message.protocol_control().start_connection().protocol_version(),
+            "favicon_size",
+            message.navigation().navigation_state_changed().favicon().size(),
             output);
-        break;
-      case ProtocolControlMessage::kCheckpointAck:
-        AddField("subtype", "CHECKPOINT_ACK", output);
-        AddField("checkpoint_id",
-                 message.protocol_control().checkpoint_ack().checkpoint_id(),
+      }
+      if (message.navigation().navigation_state_changed().has_title()) {
+        AddField("title",
+                 message.navigation().navigation_state_changed().title(),
+                 output);
+      }
+      if (message.navigation().navigation_state_changed().has_loading()) {
+        AddField("loading",
+                 message.navigation().navigation_state_changed().loading(),
                  output);
-        break;
-      default:
-        break;
-    }
+      }
+      break;
+    case NavigationMessage::LOAD_URL:
+      AddField("subtype", "LOAD_URL", output);
+      AddField("url", message.navigation().load_url().url(), output);
+      break;
+    case NavigationMessage::GO_BACK:
+      AddField("subtype", "GO_BACK", output);
+      break;
+    case NavigationMessage::GO_FORWARD:
+      AddField("subtype", "GO_FORWARD", output);
+      break;
+    case NavigationMessage::RELOAD:
+      AddField("subtype", "RELOAD", output);
+      break;
+    default:
+      break;
   }
-};
+}
+
+// Logs fields from PROTOCOL_CONTROL messages.
+void ExtractProtocolControlMessageFields(const BlimpMessage& message,
+                                         LogFields* output) {
+  switch (message.protocol_control().connection_message_case()) {
+    case ProtocolControlMessage::kStartConnection:
+      AddField("subtype", "START_CONNECTION", output);
+      AddField("client_token",
+               message.protocol_control().start_connection().client_token(),
+               output);
+      AddField("protocol_version",
+               message.protocol_control().start_connection().protocol_version(),
+               output);
+      break;
+    case ProtocolControlMessage::kCheckpointAck:
+      AddField("subtype", "CHECKPOINT_ACK", output);
+      AddField("checkpoint_id",
+               message.protocol_control().checkpoint_ack().checkpoint_id(),
+               output);
+      break;
+    default:
+      break;
+  }
+}
 
 // Logs fields from RENDER_WIDGET messages.
-class RenderWidgetLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    switch (message.render_widget().type()) {
-      case RenderWidgetMessage::INITIALIZE:
-        AddField("subtype", "INITIALIZE", output);
-        break;
-      case RenderWidgetMessage::CREATED:
-        AddField("subtype", "CREATED", output);
-        break;
-      case RenderWidgetMessage::DELETED:
-        AddField("subtype", "DELETED", output);
-        break;
-    }
-    AddField("render_widget_id", message.render_widget().render_widget_id(),
-             output);
+void ExtractRenderWidgetMessageFields(const BlimpMessage& message,
+                                      LogFields* output) {
+  switch (message.render_widget().type()) {
+    case RenderWidgetMessage::INITIALIZE:
+      AddField("subtype", "INITIALIZE", output);
+      break;
+    case RenderWidgetMessage::CREATED:
+      AddField("subtype", "CREATED", output);
+      break;
+    case RenderWidgetMessage::DELETED:
+      AddField("subtype", "DELETED", output);
+      break;
   }
-};
+  AddField("render_widget_id", message.render_widget().render_widget_id(),
+           output);
+}
 
 // Logs fields from SETTINGS messages.
-class SettingsLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    if (message.settings().has_engine_settings()) {
-      const EngineSettingsMessage& engine_settings =
-          message.settings().engine_settings();
-      AddField("subtype", "ENGINE_SETTINGS", output);
-      AddField("record_whole_document", engine_settings.record_whole_document(),
-               output);
-      AddField("client_os_info", engine_settings.client_os_info(), output);
-    }
+void ExtractSettingsMessageFields(const BlimpMessage& message,
+                                  LogFields* output) {
+  if (message.settings().has_engine_settings()) {
+    const EngineSettingsMessage& engine_settings =
+        message.settings().engine_settings();
+    AddField("subtype", "ENGINE_SETTINGS", output);
+    AddField("record_whole_document", engine_settings.record_whole_document(),
+             output);
+    AddField("client_os_info", engine_settings.client_os_info(), output);
   }
-};
+}
 
 // Logs fields from TAB_CONTROL messages.
-class TabControlLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    switch (message.tab_control().tab_control_case()) {
-      case TabControlMessage::kCreateTab:
-        AddField("subtype", "CREATE_TAB", output);
-        break;
-      case TabControlMessage::kCloseTab:
-        AddField("subtype", "CLOSE_TAB", output);
-        break;
-      case TabControlMessage::kSize:
-        AddField("subtype", "SIZE", output);
-        AddField("size", message.tab_control().size(), output);
-        break;
-      default:  // unknown
-        break;
-    }
+void ExtractTabControlMessageFields(const BlimpMessage& message,
+                                    LogFields* output) {
+  switch (message.tab_control().tab_control_case()) {
+    case TabControlMessage::kCreateTab:
+      AddField("subtype", "CREATE_TAB", output);
+      break;
+    case TabControlMessage::kCloseTab:
+      AddField("subtype", "CLOSE_TAB", output);
+      break;
+    case TabControlMessage::kSize:
+      AddField("subtype", "SIZE", output);
+      AddField("size", message.tab_control().size(), output);
+      break;
+    default:  // unknown
+      break;
   }
-};
+}
 
 // Logs fields from BLOB_CHANNEL messages.
-class BlobChannelLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {
-    switch (message.blob_channel().type_case()) {
-      case BlobChannelMessage::TypeCase::kTransferBlob:
-        AddField("subtype", "TRANSFER_BLOB", output);
-        AddField("id",
-                 base::HexEncode(
-                     message.blob_channel().transfer_blob().blob_id().data(),
-                     message.blob_channel().transfer_blob().blob_id().size()),
-                 output);
-        AddField("payload_size",
-                 message.blob_channel().transfer_blob().payload().size(),
-                 output);
-        break;
-      case BlobChannelMessage::TypeCase::TYPE_NOT_SET:  // unknown
-        break;
-    }
+void ExtractBlobChannelMessageFields(const BlimpMessage& message,
+                                     LogFields* output) {
+  switch (message.blob_channel().type_case()) {
+    case BlobChannelMessage::TypeCase::kTransferBlob:
+      AddField("subtype", "TRANSFER_BLOB", output);
+      AddField("id",
+               base::HexEncode(
+                   message.blob_channel().transfer_blob().blob_id().data(),
+                   message.blob_channel().transfer_blob().blob_id().size()),
+               output);
+      AddField("payload_size",
+               message.blob_channel().transfer_blob().payload().size(), output);
+      break;
+    case BlobChannelMessage::TypeCase::TYPE_NOT_SET:  // unknown
+      break;
   }
-};
-
-// No fields are extracted from |message|.
-class NullLogExtractor : public LogExtractor {
-  void ExtractFields(const BlimpMessage& message,
-                     LogFields* output) const override {}
-};
-
-}  // namespace
-
-BlimpMessageLogger::BlimpMessageLogger() {
-  AddHandler("COMPOSITOR", BlimpMessage::kCompositor,
-             base::WrapUnique(new CompositorLogExtractor));
-  AddHandler("INPUT", BlimpMessage::kInput,
-             base::WrapUnique(new InputLogExtractor));
-  AddHandler("NAVIGATION", BlimpMessage::kNavigation,
-             base::WrapUnique(new NavigationLogExtractor));
-  AddHandler("PROTOCOL_CONTROL", BlimpMessage::kProtocolControl,
-             base::WrapUnique(new ProtocolControlLogExtractor));
-  AddHandler("RENDER_WIDGET", BlimpMessage::kRenderWidget,
-             base::WrapUnique(new RenderWidgetLogExtractor));
-  AddHandler("SETTINGS", BlimpMessage::kSettings,
-             base::WrapUnique(new SettingsLogExtractor));
-  AddHandler("TAB_CONTROL", BlimpMessage::kTabControl,
-             base::WrapUnique(new TabControlLogExtractor));
-  AddHandler("BLOB_CHANNEL", BlimpMessage::kBlobChannel,
-             base::WrapUnique(new BlobChannelLogExtractor));
 }
 
-BlimpMessageLogger::~BlimpMessageLogger() {}
-
-void BlimpMessageLogger::AddHandler(const std::string& feature_name,
-                                    BlimpMessage::FeatureCase feature_case,
-                                    std::unique_ptr<LogExtractor> extractor) {
-  DCHECK(extractors_.find(feature_case) == extractors_.end());
-  DCHECK(!feature_name.empty());
-  extractors_[feature_case] = make_pair(feature_name, std::move(extractor));
-}
+}  // namespace
 
-void BlimpMessageLogger::LogMessageToStream(const BlimpMessage& message,
-                                            std::ostream* out) const {
+std::ostream& operator<<(std::ostream& out, const BlimpMessage& message) {
   LogFields fields;
 
-  auto extractor = extractors_.find(message.feature_case());
-  if (extractor != extractors_.end()) {
-    // An extractor is registered for |message|.
-    // Add the human-readable name of |message.type|.
-    fields.push_back(make_pair("type", extractor->second.first));
-    extractor->second.second->ExtractFields(message, &fields);
-  } else {
-    // Don't know the human-readable name of |message.type|.
-    // Just represent it using its numeric form instead.
-    AddField("type", message.feature_case(), &fields);
+  switch (message.feature_case()) {
+    case BlimpMessage::kCompositor:
+      fields.push_back(std::make_pair("type", "COMPOSITOR"));

[Wez, 2016/06/03 00:07:54]

What was wrong with AddField() here? Seems shorter and easier to grok,
especially since it's used in the logger functions.

[Kevin M, 2016/06/07 00:05:50]

Done.

+      ExtractCompositorMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kInput:
+      fields.push_back(std::make_pair("type", "INPUT"));
+      ExtractInputMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kNavigation:
+      fields.push_back(std::make_pair("type", "NAVIGATION"));
+      ExtractNavigationMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kProtocolControl:
+      fields.push_back(std::make_pair("type", "PROTOCOL_CONTROL"));
+      ExtractProtocolControlMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kRenderWidget:
+      fields.push_back(std::make_pair("type", "RENDER_WIDGET"));
+      ExtractRenderWidgetMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kSettings:
+      fields.push_back(std::make_pair("type", "SETTINGS"));
+      ExtractSettingsMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kTabControl:
+      fields.push_back(std::make_pair("type", "TAB_CONTROL"));
+      ExtractTabControlMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kBlobChannel:
+      fields.push_back(std::make_pair("type", "BLOB_CHANNEL"));
+      ExtractBlobChannelMessageFields(message, &fields);
+      break;
+    case BlimpMessage::kIme:
+      fields.push_back(std::make_pair("type", "IME"));
+      break;
+    case BlimpMessage::FEATURE_NOT_SET:
+      fields.push_back(std::make_pair("type", "<UNKNOWN>"));

[Wez, 2016/06/03 00:07:54]

Off-topic: This is an interesting limitation of using oneof - no ability to
print out what type a message from a newer sender was.

[Kevin M, 2016/06/07 00:05:50]

Yeah, there's no analogue to unknown_fields :(

+      break;
   }
 
   // Append "target_tab_id" (if present) and "byte_size" to the field set.
@@ -333,16 +306,13 @@ void BlimpMessageLogger::LogMessageToStream(const BlimpMessage& message,
 
   // Format message using the syntax:
   // <BlimpMessage field1=value1 field2="value 2">
-  *out << "<BlimpMessage ";
+  out << "<BlimpMessage ";
   for (size_t i = 0; i < fields.size(); ++i) {
-    *out << fields[i].first << "=" << fields[i].second
-         << (i != fields.size() - 1 ? " " : "");
+    out << fields[i].first << "=" << fields[i].second
+        << (i != fields.size() - 1 ? " " : "");
   }
-  *out << ">";
-}
+  out << ">";
 
-std::ostream& operator<<(std::ostream& out, const BlimpMessage& message) {
-  g_logger.Get().LogMessageToStream(message, &out);
   return out;
 }