[Media Router] Add MediaSink subtypes

chrome/browser/media/router/media_sink_internal.h

Index: chrome/browser/media/router/media_sink_internal.h
diff --git a/chrome/browser/media/router/media_sink_internal.h b/chrome/browser/media/router/media_sink_internal.h
new file mode 100644
index 0000000000000000000000000000000000000000..28fe07348fa3e73c6b56a104859f56ca1b911929
--- /dev/null
+++ b/chrome/browser/media/router/media_sink_internal.h
@@ -0,0 +1,91 @@
+// Copyright 2017 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_INTERNAL_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SINK_INTERNAL_H_
+
+#include "chrome/browser/media/router/media_sink.h"
+#include "url/gurl.h"
+
+namespace media_router {
+
+// Extra data for dial media sink.
+struct DialSinkExtraData {
+  std::string ip_address;
+
+  // Model name of the sink, if it represents a physical device.
+  base::Optional<std::string> model_name;
+
+  // The base URL used for DIAL operations.
+  GURL app_url;
+
+  DialSinkExtraData();
+  DialSinkExtraData(const DialSinkExtraData& other);
+  ~DialSinkExtraData();
+};
+
+// Extra data for cast media sink.
+struct CastSinkExtraData {
+  std::string ip_address;
+
+  // Model name of the sink, if it represents a physical device.
+  base::Optional<std::string> model_name;
+
+  // A bit vector representing the capabilities of the sink.

[mark a. foltz, 2017/02/25 01:10:24]

Can you mention that the values are defined in the mojom?

[zhaobin, 2017/02/27 21:45:14]

Done.

+  int capabilities = 0;

[mark a. foltz, 2017/02/25 01:10:24]

Is the default a valid set of capabilities, or must the user set it to some
value?

[zhaobin, 2017/02/27 21:45:15]

Done.

+
+  // ID of Cast channel opened for the sink.
+  int cast_channel_id = 0;

[mark a. foltz, 2017/02/25 01:10:23]

Similar question - document that the caller must set this value to a valid
cast_channel_id.

Also note that the cast_channel_id may change over time as the browser
reconnects to a device.

[zhaobin, 2017/02/27 21:45:14]

Done.

+
+  CastSinkExtraData();
+  CastSinkExtraData(const CastSinkExtraData& other);
+  ~CastSinkExtraData();
+};
+
+// Represents a media sink discovered by MediaSinkService.

[imcheng, 2017/02/24 23:54:20]

You may also mention that it is used by MediaSinkService to push MediaSinks with
extra data to the MediaRouteProvider, and it is not exposed to users of
MediaRouter.

[zhaobin, 2017/02/27 21:45:14]

Done.

+class MediaSinkInternal {

[mark a. foltz, 2017/02/25 01:10:23]

This is really an implementation detail of the media sink discovery service.

It might be helpful to have a folder specifically for discovery related code and
make this part of that; otherwise it's difficult for the casual reader to
understand why there is MediaSink and MediaSinkInternal.

[zhaobin, 2017/02/27 21:45:15]

Done.

+ public:
+  // SinkType indicates if sink is discovered by DIAL / CAST media sink
+  // services.
+  enum class SinkType {

[mark a. foltz, 2017/02/25 01:10:24]

This can be inferred from which of the optional fields have values - so don't
need to define an enum.

[zhaobin, 2017/02/27 21:45:14]

Done.

+    GENERIC = 0,
+    DIAL,
+    CAST,
+  };
+
+  explicit MediaSinkInternal(const MediaSink& sink);
+  MediaSinkInternal(const MediaSink& sink, const DialSinkExtraData& dial_data);
+  MediaSinkInternal(const MediaSink& sink, const CastSinkExtraData& cast_data);
+  MediaSinkInternal();
+  MediaSinkInternal(const MediaSinkInternal& other);

[mark a. foltz, 2017/02/25 01:10:24]

Define operator== as well?  Especially helpful for unit testing?

How is the MediaSinkService expected to handle updates to existing sinks?  Since
you're defining a copy constructor, presumably it's free to make local copies of
sink data.  Will it be responsible for updating all copies?

If so, it would be a good idea to define operator=.

These are small objects and discovery shouldn't generate a lot - I don't think
it's worth trying to make them zero-copy/move-only.

[zhaobin, 2017/02/27 21:45:14]

Done.

+  ~MediaSinkInternal();
+
+  // Set |sink_| and invalidate extra data.
+  void set_sink(const MediaSink& sink);
+  const MediaSink& sink() const { return sink_; }
+
+  void set_dial_data(const DialSinkExtraData& dial_data);
+  const DialSinkExtraData& dial_data() const;
+  void set_cast_data(const CastSinkExtraData& cast_data);
+  const CastSinkExtraData& cast_data() const;
+
+  bool is_dial_sink() const { return type_ == SinkType::DIAL; }
+  bool is_cast_sink() const { return type_ == SinkType::CAST; }
+
+ private:
+  MediaSink sink_;
+
+  // Type of the MediaSink.
+  SinkType type_;
+
+  // Set this field if |type_| == DIAL.

[zhaobin, 2017/02/24 02:19:15]

Tried to use union, but cannot store DialSinkExtraData struct inside union
because it has non trivial copy constructor (compile succeeded, but got weird
crashes in unit tests). Workaround would be using base::ManualConstructor<> and
init these fields manually, which seems not necessary.

[imcheng, 2017/02/24 23:54:20]

I think it is fine to not use union as our code is not performance critical. 

Another way would be to define a BaseExtraData and use a
std::unique_ptr<BaseExtraData> here. Copy construction and assignment might
become tricky though, so I'd rather take the hit of having an extra field here.

[imcheng, 2017/02/24 23:54:20]

s/this field// here and below.

[mark a. foltz, 2017/02/25 01:10:23]

base::Optional<> is a good solution here.  It's what I used for
PresentationConnectionMessage.

[zhaobin, 2017/02/27 21:45:15]

Done.

+  base::Optional<DialSinkExtraData> dial_data_;
+
+  // Set this field if |type_| == CAST.
+  base::Optional<CastSinkExtraData> cast_data_;
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SINK_INTERNAL_H_