[Media Router] Clean up issues related code.

chrome/browser/media/router/mojo/media_router_struct_traits.h

Index: chrome/browser/media/router/mojo/media_router_struct_traits.h
diff --git a/chrome/browser/media/router/mojo/media_router_struct_traits.h b/chrome/browser/media/router/mojo/media_router_struct_traits.h
index 4fe10239cad005abec59e312780d0f09f10ba55d..bd564503423f48ee9febe65bb50c51d5f3d370b7 100644
--- a/chrome/browser/media/router/mojo/media_router_struct_traits.h
+++ b/chrome/browser/media/router/mojo/media_router_struct_traits.h
@@ -7,11 +7,57 @@
 
 #include <string>
 
+#include "chrome/browser/media/router/issue.h"
 #include "chrome/browser/media/router/mojo/media_router.mojom.h"
 #include "mojo/common/common_custom_types_struct_traits.h"
 
 namespace mojo {
 
+namespace {
+
+media_router::IssueInfo::Action IssueActionFromMojo(
+    media_router::mojom::Issue::ActionType action_type) {
+  switch (action_type) {
+    case media_router::mojom::Issue::ActionType::DISMISS:
+      return media_router::IssueInfo::Action::DISMISS;
+    case media_router::mojom::Issue::ActionType::LEARN_MORE:
+      return media_router::IssueInfo::Action::LEARN_MORE;
+    default:
+      NOTREACHED() << "Unknown issue action type " << action_type;
+      return media_router::IssueInfo::Action::DISMISS;
+  }
+}
+
+media_router::mojom::Issue::ActionType IssueActionToMojo(
+    media_router::IssueInfo::Action action) {
+  switch (action) {
+    case media_router::IssueInfo::Action::DISMISS:
+      return media_router::mojom::Issue::ActionType::DISMISS;
+    case media_router::IssueInfo::Action::LEARN_MORE:
+      return media_router::mojom::Issue::ActionType::LEARN_MORE;
+    default:
+      NOTREACHED() << "Unknown issue action type " << static_cast<int>(action);
+      return media_router::mojom::Issue::ActionType::DISMISS;
+  }
+}
+
+media_router::IssueInfo::Severity IssueSeverityFromMojo(
+    media_router::mojom::Issue::Severity severity) {
+  switch (severity) {
+    case media_router::mojom::Issue::Severity::FATAL:
+      return media_router::IssueInfo::Severity::FATAL;
+    case media_router::mojom::Issue::Severity::WARNING:
+      return media_router::IssueInfo::Severity::WARNING;
+    case media_router::mojom::Issue::Severity::NOTIFICATION:
+      return media_router::IssueInfo::Severity::NOTIFICATION;
+    default:
+      NOTREACHED() << "Unknown issue severity " << severity;
+      return media_router::IssueInfo::Severity::WARNING;
+  }
+}

[dcheng, 2016/12/07 18:58:02]

Please typemap the enums as well: then we can just write EnumTraits for this.

[imcheng, 2016/12/09 23:19:53]

Done.

+
+}  // namespace
+
 template <>
 struct StructTraits<media_router::mojom::RouteMessageDataView,
                     media_router::RouteMessage> {
@@ -66,6 +112,113 @@ struct StructTraits<media_router::mojom::RouteMessageDataView,
   }
 };
 
+template <>
+struct StructTraits<media_router::mojom::IssueDataView,
+                    media_router::IssueInfo> {
+  static bool Read(media_router::mojom::IssueDataView data,
+                   media_router::IssueInfo* out) {

[dcheng, 2016/12/07 18:58:02]

Please out-of-line this method (since it's pretty long) into a .cc file.

[imcheng, 2016/12/09 23:19:53]

Done.

[dcheng, 2016/12/12 01:45:36]

It looks like the .cc file didn't get uploaded.

[imcheng, 2016/12/12 19:09:12]

My bad. Forgot to git add it.

+    if (!data.ReadTitle(&out->title))
+      return false;
+
+    media_router::mojom::Issue::ActionType default_action;
+    if (!data.ReadDefaultAction(&default_action))
+      return false;
+
+    out->default_action = IssueActionFromMojo(default_action);
+
+    media_router::mojom::Issue::Severity severity;
+    if (!data.ReadSeverity(&severity))
+      return false;
+
+    out->severity = IssueSeverityFromMojo(severity);
+
+    base::Optional<std::string> message;
+    if (!data.ReadMessage(&message))
+      return false;
+
+    out->message = message.value_or(std::string());
+
+    base::Optional<std::vector<media_router::mojom::Issue::ActionType>>
+        secondary_actions;
+    if (!data.ReadSecondaryActions(&secondary_actions))
+      return false;
+
+    if (secondary_actions.has_value()) {
+      for (const auto action : secondary_actions.value())
+        out->secondary_actions.push_back(IssueActionFromMojo(action));
+    }
+
+    base::Optional<std::string> route_id;
+    if (!data.ReadRouteId(&route_id))
+      return false;
+
+    out->route_id = route_id.value_or(std::string());
+
+    out->is_blocking = data.is_blocking();

[dcheng, 2016/12/07 18:58:02]

There's nothing that requires is_blocking() to agree with severity. As mentioned
earlier, I think we can get around this by not having this as an explicit field
and just deriving the value as a getter.

[imcheng, 2016/12/09 23:19:53]

See other comment.

+    out->help_page_id = data.help_page_id();
+
+    return true;
+  }
+
+  static base::Optional<std::string> route_id(
+      const media_router::IssueInfo& issue) {
+    return issue.route_id.empty() ? base::Optional<std::string>()
+                                  : base::make_optional(issue.route_id);
+  }
+
+  static media_router::mojom::Issue::Severity severity(
+      const media_router::IssueInfo& issue) {
+    switch (issue.severity) {
+      case media_router::IssueInfo::Severity::FATAL:
+        return media_router::mojom::Issue::Severity::FATAL;
+      case media_router::IssueInfo::Severity::WARNING:
+        return media_router::mojom::Issue::Severity::WARNING;
+      case media_router::IssueInfo::Severity::NOTIFICATION:
+        return media_router::mojom::Issue::Severity::NOTIFICATION;
+      default:
+        NOTREACHED() << "Unknown issue severity "
+                     << static_cast<int>(issue.severity);
+        return media_router::mojom::Issue::Severity::WARNING;
+    }
+  }
+
+  static bool is_blocking(const media_router::IssueInfo& issue) {
+    return issue.is_blocking;
+  }
+
+  static std::string title(const media_router::IssueInfo& issue) {
+    return issue.title;
+  }
+
+  static base::Optional<std::string> message(
+      const media_router::IssueInfo& issue) {
+    return issue.message.empty() ? base::Optional<std::string>()

[dcheng, 2016/12/07 18:58:02]

Maybe the C++ struct should just use base::Optional<> directly?

Getters are actually called multiple times during serialization, so it's best to
make them very cheap getters.

[imcheng, 2016/12/09 23:19:53]

Ok. OTOH, we are not converting Issues from C++ to mojo -- is there a way to
avoid writing these code which seem unnecessary to me?

[dcheng, 2016/12/12 01:45:36]

I suppose this doesn't compile if we omit these: I guess we can just annotate
them all with NOTREACHED().

(I would personally also just return bogus values, but I don't feel strongly)

[imcheng, 2016/12/12 19:09:12]

Ok. I will just keep them as is.

+                                 : base::make_optional(issue.message);
+  }
+
+  static media_router::mojom::Issue::ActionType default_action(
+      const media_router::IssueInfo& issue) {
+    return IssueActionToMojo(issue.default_action);
+  }
+
+  static base::Optional<std::vector<media_router::mojom::Issue::ActionType>>
+  secondary_actions(const media_router::IssueInfo& issue) {
+    if (issue.secondary_actions.empty())
+      return base::Optional<
+          std::vector<media_router::mojom::Issue::ActionType>>();
+
+    std::vector<media_router::mojom::Issue::ActionType> mojo_action_types;
+    for (const auto& action : issue.secondary_actions)
+      mojo_action_types.push_back(IssueActionToMojo(action));
+
+    return base::make_optional(std::move(mojo_action_types));
+  }
+
+  static int32_t help_page_id(const media_router::IssueInfo& issue) {
+    return issue.help_page_id;
+  }
+};
+
 }  // namespace mojo
 
 #endif  // CHROME_BROWSER_MEDIA_ROUTER_MOJO_MEDIA_ROUTER_STRUCT_TRAITS_H_