[Media Router] Add MediaSink subtypes

chrome/browser/media/router/media_sink.h

 // Copyright 2015 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 CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SINK_H_
 #define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SINK_H_
 
 #include <string>
 
+#include "base/optional.h"
 #include "third_party/icu/source/common/unicode/uversion.h"
 
 namespace U_ICU_NAMESPACE {
 class Collator;
 }  // namespace U_ICU_NAMESPACE
 
 namespace media_router {
 
 // Represents a sink to which media can be routed.
 class MediaSink {

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

Could you add a TODO to convert this into a struct?

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

Done.

  public:
   using Id = std::string;
 
   // IconTypes are listed in the order in which sinks should be sorted.
   // The order must stay in sync with
   // chrome/browser/resources/media_router/media_router_data.js.
   enum IconType {
     CAST,
     CAST_AUDIO_GROUP,
     CAST_AUDIO,
     HANGOUT,
     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);
 
   MediaSink(const MediaSink& other);
 
   ~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.

 
   // Compares |this| to |other| first by their icon types, then their names
   // using |collator|, and finally their IDs.
   bool CompareUsingCollator(const MediaSink& other,
                             const icu::Collator* collator) const;
 
   // For storing in sets and in maps as keys.
   struct Compare {
     bool operator()(const MediaSink& sink1, const MediaSink& sink2) const {
       return sink1.id() < sink2.id();
     }
   };
 
+ protected:
+  // Type of the MediaSink.
+  SinkType type_;
+
  private:
   // Unique identifier for the MediaSink.
   MediaSink::Id sink_id_;
 
   // Descriptive name of the 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
 
 #endif  // CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SINK_H_