[Media Router] Add MediaSink subtypes

chrome/browser/media/router/media_sink.h

Index: chrome/browser/media/router/media_sink.h
diff --git a/chrome/browser/media/router/media_sink.h b/chrome/browser/media/router/media_sink.h
index e12b6b16f55de968635b3955b6f2484c8dfe3d5e..c896e0ccf9443016dec5cdb6dfa253e443579395 100644
--- a/chrome/browser/media/router/media_sink.h
+++ b/chrome/browser/media/router/media_sink.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/optional.h"
 #include "third_party/icu/source/common/unicode/uversion.h"
 
 namespace U_ICU_NAMESPACE {
@@ -31,6 +32,10 @@ class MediaSink {
     GENERIC
   };
 
+  // SinkType indicates if sink is discovered by DIAL / CAST / other (e.g.
+  // cloud) media sink services. Used by mojo.

[imcheng, 2017/02/08 01:28:09]

1) Remove "Used by mojo."
2) I would point out that each SinkType value, with the exception of GENERIC,
corresponds to a subtype of MediaSink, and then do something like:

enum class SinkType {
  DIAL,  // DialMediaSink
  CAST,  // CastMediaSink
  GENERIC
};

[zhaobin, 2017/02/09 00:13:52]

Done.

+  enum class SinkType { DIAL, CAST, GENERAL };

[imcheng, 2017/02/08 01:28:09]

s/GENERAL/GENERIC

[zhaobin, 2017/02/09 00:13:52]

Done.

+
   MediaSink(const MediaSink::Id& sink_id,
             const std::string& name,
             const IconType icon_type);
@@ -39,15 +44,22 @@ class MediaSink {
 
   ~MediaSink();

[imcheng, 2017/02/08 01:28:09]

This needs to be virtual now.

[dcheng, 2017/02/08 03:01:05]

Hmm. Our compiler is supposed to warn on this. zhaobin, do you mind sharing your
build config so I can see if we have a bug in this detection?

[zhaobin, 2017/02/09 00:13:52]

# Build arguments go here. Examples:
#   is_component_build = true
#   is_debug = false
   use_goma = true
# See "gn args <out_dir> --list" for available build arguments.

[zhaobin, 2017/02/09 00:13:52]

Done.

[dcheng, 2017/02/09 00:33:23]

Ah doh.... this is a known bug =(
https://llvm.org/bugs/show_bug.cgi?id=28460

[imcheng, 2017/02/09 02:19:57]

Wouldn't stack allocated objects (which I am pretty sure we do in at least
somewhere in unit tests) also trigger the warning? At any rate, thanks for
tracking this down.

 
+  SinkType type() const { return type_; }
   const MediaSink::Id& id() const { return sink_id_; }
   const std::string& name() const { return name_; }
   void set_description(const std::string& description) {
     description_ = description;
   }
-  const std::string& description() const { return description_; }
+  const base::Optional<std::string>& description() const {
+    return description_;
+  }
   void set_domain(const std::string& domain) { domain_ = domain; }
-  const std::string& domain() const { return domain_; }
+  const base::Optional<std::string>& domain() const { return domain_; }
   IconType icon_type() const { return icon_type_; }
+  void set_model_name(const std::string& model_name) {
+    model_name_ = model_name;
+  }
+  const base::Optional<std::string>& model_name() const { return model_name_; }
 
   bool Equals(const MediaSink& other) const;

[imcheng, 2017/02/08 01:28:09]

I would add a note here that this method only compares IDs.

[zhaobin, 2017/02/09 00:13:52]

Done.

 
@@ -63,6 +75,10 @@ class MediaSink {
     }
   };
 
+ protected:
+  // Type of the MediaSink.
+  SinkType type_;
+
  private:
   // Unique identifier for the MediaSink.
   MediaSink::Id sink_id_;
@@ -71,13 +87,16 @@ class MediaSink {
   std::string name_;
 
   // Optional description of the MediaSink.

[imcheng, 2017/02/08 01:28:09]

This comment doesn't add much - remove?

[zhaobin, 2017/02/09 00:13:52]

Done.

-  std::string description_;
+  base::Optional<std::string> description_;
 
   // Optional domain of the MediaSink.

[imcheng, 2017/02/08 01:28:09]

ditto

[zhaobin, 2017/02/09 00:13:52]

Done.

-  std::string domain_;
+  base::Optional<std::string> domain_;
 
   // The type of icon that corresponds with the MediaSink.
   IconType icon_type_;
+
+  // Optional model name of the MediaSink.

[imcheng, 2017/02/08 01:28:09]

ditto, or "Model name of the sink, if it represents a physical device."

[zhaobin, 2017/02/09 00:13:52]

Done.

+  base::Optional<std::string> model_name_;
 };
 
 }  // namespace media_router