Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675033002:
[Media Router] Add MediaSink subtypes (Closed)
|
Created:
3 years, 10 months ago by zhaobin Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Add MediaSink subtypes
- Introduce subclasses of MediaSink (DialMediaSink and CastMediaSink) in C++
- Introduce Mojo struct definitions
- Add typemap for MediaSink subclasses
- Add unit tests for typemap conversion
MediaSink typemap is based on https://codereview.chromium.org/2666873006/. Will merge when that lands.
BUG=687365
Review-Url: https://codereview.chromium.org/2675033002
Cr-Commit-Position: refs/heads/master@{#457123}
Committed: https://chromium.googlesource.com/chromium/src/+/88c63227018cd614c0eab270dbc2c6aea43a79ec
Patch Set 1 #
Total comments: 12
Patch Set 2 : resolve code review comments from dcheng #Patch Set 3 : add url.mojom to media_router.mojoms JsResources #
Total comments: 46
Patch Set 4 : resolve code review comments from Derek #
Total comments: 1
Patch Set 5 : add TypedMediaSink class #
Total comments: 30
Patch Set 6 : resolve code review comments from Derek #Patch Set 7 : merge with master #
Total comments: 12
Patch Set 8 : resolve code review comments from Mark #Patch Set 9 : use base::Optional instead of union to store extra data #
Total comments: 52
Patch Set 10 : resolve code review comments from Derek and Mark #
Total comments: 25
Patch Set 11 : resolve code review comments from Mark #
Total comments: 8
Patch Set 12 : resolve code review comments from Mark #
Total comments: 10
Patch Set 13 : resolve code review comments from Derek #
Total comments: 17
Patch Set 14 : rebase #Patch Set 15 : resolve code review comments from dcheng #Patch Set 16 : fix android test failures #Patch Set 17 : resolve code review comments from dcheng #
Total comments: 4
Patch Set 18 : resolve code review comments from dcheng #
Total comments: 6
Patch Set 19 : depend on net::IPAddress typemap patch #
Total comments: 24
Patch Set 20 : resolve code review comments from Derek and Mark #Messages
Total messages: 88 (38 generated)
Patchset #1 (id:1) has been deleted
zhaobin@chromium.org changed reviewers: + dcheng@chromium.org, imcheng@chromium.org, mfoltz@chromium.org
https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:39: CastMediaSink? cast_media_sink; ? shouldn't be required on lines 38 and 39 (it's implicitly implied in a union that only one will be set) https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.typemap (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.typemap:24: "media_router.mojom.MediaSink=std::unique_ptr<media_router::MediaSink>[move_only,nullable_is_same_type]", Is nullable_is_same_type required here? Is there an instance where we want to send nullable MediaSinks? https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.cc:61: media_router::MediaSink* sink = nullptr; Make this a std::unique_ptr (the early returns will leak otherwise) https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:98: default: Nit: remove default (that way the compiler will warn if new enum values are added) https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:122: default: Nit: remove default (that way the compiler will warn if new enum values are added) https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:153: return sink->description().empty() Make the fields on the C++ class optional; doing this conversion in the getter has non-zero overhead (getters are invoked twice on serialization). It's possible to use something called a context object to stash temporary results... but it's kind of overkill.
https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:39: CastMediaSink? cast_media_sink; On 2017/02/05 09:23:00, dcheng wrote: > ? shouldn't be required on lines 38 and 39 (it's implicitly implied in a union > that only one will be set) Done. https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.typemap (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.typemap:24: "media_router.mojom.MediaSink=std::unique_ptr<media_router::MediaSink>[move_only,nullable_is_same_type]", On 2017/02/05 09:23:00, dcheng wrote: > Is nullable_is_same_type required here? Is there an instance where we want to > send nullable MediaSinks? Done. https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.cc:61: media_router::MediaSink* sink = nullptr; On 2017/02/05 09:23:00, dcheng wrote: > Make this a std::unique_ptr (the early returns will leak otherwise) Done. https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:98: default: On 2017/02/05 09:23:00, dcheng wrote: > Nit: remove default (that way the compiler will warn if new enum values are > added) Done. https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:122: default: On 2017/02/05 09:23:00, dcheng wrote: > Nit: remove default (that way the compiler will warn if new enum values are > added) Done. https://codereview.chromium.org/2675033002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:153: return sink->description().empty() On 2017/02/05 09:23:00, dcheng wrote: > Make the fields on the C++ class optional; doing this conversion in the getter > has non-zero overhead (getters are invoked twice on serialization). It's > possible to use something called a context object to stash temporary results... > but it's kind of overkill. Done.
mojo lg to me; I will stamp once someone who actually knows media router code is happy as well.
I have a number of comments but before addressing, I wanted to get Mark's take on whether derived MediaSink classes are a good idea first (see comment in media_router_mojo_impl.cc) https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/cast_media_sink.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.cc:17: type_ = SinkType::CAST; Can the type be passed into the base constructor? https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/cast_media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.h:37: int capabilities_; Please add a comment: // A bit vector representing the capabilities of the sink. Note: we may have to define capability as an enum in the future. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.h:39: // Used for feedback We probably don't need this comment. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.h:42: // ID of cast channel opened by Media Router. The ID can be // ID of Cast channel opened for the sink. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/dial_media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/dial_media_sink.h:34: // Used for feedback Not needed. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/dial_media_sink.h:37: // Used for DIAL launch I would probably write this as: // The base URL used for DIAL operations. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.cc:15: : type_(SinkType::GENERAL), We can define a protected MediaSink constructor that takes a SinkType in addition to the existing 3 arguments. Then this constructor and the Dial / Cast MediaSink constructors can call it. WDYT? https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:20: class MediaSink { Could you add a TODO to convert this into a struct? https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:36: // cloud) media sink services. Used by mojo. 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 }; https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:37: enum class SinkType { DIAL, CAST, GENERAL }; s/GENERAL/GENERIC https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:45: ~MediaSink(); This needs to be virtual now. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:64: bool Equals(const MediaSink& other) const; I would add a note here that this method only compares IDs. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:89: // Optional description of the MediaSink. This comment doesn't add much - remove? https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:92: // Optional domain of the MediaSink. ditto https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:98: // Optional model name of the MediaSink. ditto, or "Model name of the sink, if it represents a physical device." https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:31: // Used for feedback Replace the "Used for XXX" with a description of what the field represents. The "Used for XXX" in the design doc are for my own / readers' information on why we are adding the field. :) https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.typemap (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.typemap:24: "media_router.mojom.MediaSink=std::unique_ptr<media_router::MediaSink>[move_only]", Seems like this should be ordered alphabetically. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:203: sink_list.push_back(*sinks[i]); In here we are performing object slicing on derived MediaSink instances. In practice this is fine, provided we don't downcast MediaSinks outside of media_router_struct_traits. But it does create an inconsistency with the API that pushes browser-discovered sinks, which we cannot slice, to MR extension. To resolve this we would have to convert the MediaSinksObserver API to use std::vector<std::unique_ptr<MediaSink>>. I wonder if derived classes are truly useful enough to worth the trouble. The alternative is to use a property map on MediaSink to store DIAL / Cast specific fields, at the cost of losing strong type safety. Mark, WDYT? https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:194: default: Omit default case to let compiler catch cases we forgot to include. https://codereview.chromium.org/2675033002/diff/60001/extensions/renderer/res... File extensions/renderer/resources/extensions_renderer_resources.grd (right): https://codereview.chromium.org/2675033002/diff/60001/extensions/renderer/res... extensions/renderer/resources/extensions_renderer_resources.grd:92: <include name="IDR_MOJO_URL_MOJOM_JS" file="${mojom_root}\url\mojo\url.mojom.js" use_base_dir="false" type="BINDATA" /> You will need to update the deps in https://cs.chromium.org/chromium/src/extensions/BUILD.gn?type=cs&q=extensions...
https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:45: ~MediaSink(); On 2017/02/08 01:28:09, imcheng wrote: > This needs to be virtual now. 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?
https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/cast_media_sink.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.cc:17: type_ = SinkType::CAST; On 2017/02/08 01:28:08, imcheng wrote: > Can the type be passed into the base constructor? Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/cast_media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.h:37: int capabilities_; On 2017/02/08 01:28:08, imcheng wrote: > Please add a comment: > > // A bit vector representing the capabilities of the sink. > > Note: we may have to define capability as an enum in the future. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.h:39: // Used for feedback On 2017/02/08 01:28:08, imcheng wrote: > We probably don't need this comment. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/cast_media_sink.h:42: // ID of cast channel opened by Media Router. The ID can be On 2017/02/08 01:28:08, imcheng wrote: > // ID of Cast channel opened for the sink. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/dial_media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/dial_media_sink.h:34: // Used for feedback On 2017/02/08 01:28:08, imcheng wrote: > Not needed. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/dial_media_sink.h:37: // Used for DIAL launch On 2017/02/08 01:28:08, imcheng wrote: > I would probably write this as: > > // The base URL used for DIAL operations. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.cc:15: : type_(SinkType::GENERAL), On 2017/02/08 01:28:08, imcheng wrote: > We can define a protected MediaSink constructor that takes a SinkType in > addition to the existing 3 arguments. Then this constructor and the Dial / Cast > MediaSink constructors can call it. WDYT? Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:20: class MediaSink { On 2017/02/08 01:28:09, imcheng wrote: > Could you add a TODO to convert this into a struct? Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:36: // cloud) media sink services. Used by mojo. On 2017/02/08 01:28:09, imcheng wrote: > 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 > }; Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:37: enum class SinkType { DIAL, CAST, GENERAL }; On 2017/02/08 01:28:09, imcheng wrote: > s/GENERAL/GENERIC Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:45: ~MediaSink(); On 2017/02/08 03:01:05, dcheng wrote: > On 2017/02/08 01:28:09, imcheng wrote: > > This needs to be virtual now. > > 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? # 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. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:45: ~MediaSink(); On 2017/02/08 01:28:09, imcheng wrote: > This needs to be virtual now. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:64: bool Equals(const MediaSink& other) const; On 2017/02/08 01:28:09, imcheng wrote: > I would add a note here that this method only compares IDs. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:89: // Optional description of the MediaSink. On 2017/02/08 01:28:09, imcheng wrote: > This comment doesn't add much - remove? Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:92: // Optional domain of the MediaSink. On 2017/02/08 01:28:09, imcheng wrote: > ditto Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:98: // Optional model name of the MediaSink. On 2017/02/08 01:28:09, imcheng wrote: > ditto, or "Model name of the sink, if it represents a physical device." Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:31: // Used for feedback On 2017/02/08 01:28:09, imcheng wrote: > Replace the "Used for XXX" with a description of what the field represents. The > "Used for XXX" in the design doc are for my own / readers' information on why we > are adding the field. :) Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.typemap (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.typemap:24: "media_router.mojom.MediaSink=std::unique_ptr<media_router::MediaSink>[move_only]", On 2017/02/08 01:28:09, imcheng wrote: > Seems like this should be ordered alphabetically. Done. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:203: sink_list.push_back(*sinks[i]); On 2017/02/08 01:28:09, imcheng wrote: > In here we are performing object slicing on derived MediaSink instances. In > practice this is fine, provided we don't downcast MediaSinks outside of > media_router_struct_traits. But it does create an inconsistency with the API > that pushes browser-discovered sinks, which we cannot slice, to MR extension. To > resolve this we would have to convert the MediaSinksObserver API to use > std::vector<std::unique_ptr<MediaSink>>. > > I wonder if derived classes are truly useful enough to worth the trouble. The > alternative is to use a property map on MediaSink to store DIAL / Cast specific > fields, at the cost of losing strong type safety. Mark, WDYT? Acknowledged. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.h:194: default: On 2017/02/08 01:28:09, imcheng wrote: > Omit default case to let compiler catch cases we forgot to include. Done. https://codereview.chromium.org/2675033002/diff/60001/extensions/renderer/res... File extensions/renderer/resources/extensions_renderer_resources.grd (right): https://codereview.chromium.org/2675033002/diff/60001/extensions/renderer/res... extensions/renderer/resources/extensions_renderer_resources.grd:92: <include name="IDR_MOJO_URL_MOJOM_JS" file="${mojom_root}\url\mojo\url.mojom.js" use_base_dir="false" type="BINDATA" /> On 2017/02/08 01:28:09, imcheng wrote: > You will need to update the deps in > https://cs.chromium.org/chromium/src/extensions/BUILD.gn?type=cs&q=extensions... Done.
https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:45: ~MediaSink(); On 2017/02/09 00:13:52, zhaobin wrote: > On 2017/02/08 01:28:09, imcheng wrote: > > This needs to be virtual now. > > Done. Ah doh.... this is a known bug =( https://llvm.org/bugs/show_bug.cgi?id=28460
Thanks. I have a couple of comments following our discussion today. Additional comments will follow after I do another pass of the code. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:45: ~MediaSink(); On 2017/02/09 00:33:23, dcheng wrote: > On 2017/02/09 00:13:52, zhaobin wrote: > > On 2017/02/08 01:28:09, imcheng wrote: > > > This needs to be virtual now. > > > > Done. > > Ah doh.... this is a known bug =( > https://llvm.org/bugs/show_bug.cgi?id=28460 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. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:203: sink_list.push_back(*sinks[i]); On 2017/02/08 01:28:09, imcheng wrote: > In here we are performing object slicing on derived MediaSink instances. In > practice this is fine, provided we don't downcast MediaSinks outside of > media_router_struct_traits. But it does create an inconsistency with the API > that pushes browser-discovered sinks, which we cannot slice, to MR extension. To > resolve this we would have to convert the MediaSinksObserver API to use > std::vector<std::unique_ptr<MediaSink>>. > > I wonder if derived classes are truly useful enough to worth the trouble. The > alternative is to use a property map on MediaSink to store DIAL / Cast specific > fields, at the cost of losing strong type safety. Mark, WDYT? So this is probably fine since we don't need to (or want to) expose any data beyond what's in the generic MediaSink to consumers. We would have to be clear in the documentation that the type field and the MediaSink subclasses are only used by MediaSinkService and in Mojo conversion, or make sure we don't create MediaSink instances that is not GENERIC - see comment in media_sink.h. https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_struct_traits.cc:65: if (extra_data_data_view.is_null()) { We shouldn't be returning non-generic MediaSinks from the extension. Maybe add a NOTREACHED() if extra data is not null? https://codereview.chromium.org/2675033002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:53: SinkType type() const { return type_; } We should make this method virtual, and return GENERIC here, and override it in the subclasses to return DIAL/CAST.
Per yesterday's discussion with Bin, we are going with a slightly different approach. For pushing sinks to the extension, we will be using a data structure that wraps a MediaSink with extra data. In effect we will leave the MediaSink class intact. This separation should make it easier to express DIAL/Cast specific sink data to the extension without affecting other consumers of MediaSinks.
Patchset #5 (id:100001) has been deleted
zhaobin@chromium.org changed reviewers: - dcheng@chromium.org
Added TypedMediaSink class to mojo. -dcheng@ for now till we finalize mojo API.
I think this is a good improvement over the original design. It isolates unrelated data from consumers of MediaSinks and now the mojo conversion is more straightforward. I have some preliminary comments which can be addressed after Mark signs off on the design. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:39: enum class SinkType { Can be removed. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:52: virtual ~MediaSink(); revert virtual https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:71: // void set_model_name(const std::string& model_name) { Remove https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:92: protected: Remove ctor and SinkType https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:116: base::Optional<std::string> model_name_; This can be removed as it is now in TypedMediaSink. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:32: // Represents a sink discovered by MediaSinkService. Represents a sink discovered by Media Router, which is pushed to the MediaRouteProvider. It wraps a MediaSink with additional data required by the MediaRouteProvider to perform additional operations, such as route creation. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:112: NOTREACHED(); NOTREACHED() not needed anymore? https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:196: NOTREACHED(); should this be a CHECK(false) since we shouldn't ever be passing in an unknown TypedMediaSink? Not sure if there's a meaningful way to recover from it, either. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/typed_media_sink.cc (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.cc:19: : type_(SinkType::UNKNOWN), dial_sink_extra_data_(DialSinkExtraData()) {} Did you need to initialize dial_sink_extra_data_ here to avoid compile error? https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.cc:39: DCHECK_EQ(type_, SinkType::UNKNOWN); Why do we need these DCHECKs? It seems perfectly legal to overwrite the extra data as long as we are doing so consistently. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.cc:44: const DialSinkExtraData* TypedMediaSink::dial_sink_extra_data() const { Suggest making these return const ref, and turn the if statement into a CHECK. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/typed_media_sink.h (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:22: int capabilities; = 0; https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:25: int cast_channel_id; ditto https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:28: // Represents a media sink discovered by MediaSinkService. Comment that this is only used by MediaSinkService to push MediaSinks to the Media Route Provider? https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:39: TypedMediaSink(const MediaSink::Id& sink_id, How about these two ctors in addition to the empty ctor required by Mojo: TypedMediaSink(const MediaSink& sink, const DialSinkExtraData& dial_data) TypedMediaSink(const MediaSink& sink, const CastSinkExtraData& cast_data)
https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:39: enum class SinkType { On 2017/02/11 01:00:21, imcheng wrote: > Can be removed. Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:52: virtual ~MediaSink(); On 2017/02/11 01:00:21, imcheng wrote: > revert virtual Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:71: // void set_model_name(const std::string& model_name) { On 2017/02/11 01:00:21, imcheng wrote: > Remove Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:92: protected: On 2017/02/11 01:00:21, imcheng wrote: > Remove ctor and SinkType Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_sink.h:116: base::Optional<std::string> model_name_; On 2017/02/11 01:00:21, imcheng wrote: > This can be removed as it is now in TypedMediaSink. Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:32: // Represents a sink discovered by MediaSinkService. On 2017/02/11 01:00:21, imcheng wrote: > Represents a sink discovered by Media Router, which is pushed to the > MediaRouteProvider. It wraps a MediaSink with additional data required by the > MediaRouteProvider to perform additional operations, such as route creation. Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:112: NOTREACHED(); On 2017/02/11 01:00:21, imcheng wrote: > NOTREACHED() not needed anymore? Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:196: NOTREACHED(); On 2017/02/11 01:00:22, imcheng wrote: > should this be a CHECK(false) since we shouldn't ever be passing in an unknown > TypedMediaSink? Not sure if there's a meaningful way to recover from it, either. Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/typed_media_sink.cc (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.cc:19: : type_(SinkType::UNKNOWN), dial_sink_extra_data_(DialSinkExtraData()) {} On 2017/02/11 01:00:22, imcheng wrote: > Did you need to initialize dial_sink_extra_data_ here to avoid compile error? Unit test crashes if we do not init DialSinkExtraData. GURL operator= uses a swap. dial_sink_extra_data_.app_url is invalid memory if we do not initialize it here... https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.cc:39: DCHECK_EQ(type_, SinkType::UNKNOWN); On 2017/02/11 01:00:22, imcheng wrote: > Why do we need these DCHECKs? It seems perfectly legal to overwrite the extra > data as long as we are doing so consistently. Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.cc:44: const DialSinkExtraData* TypedMediaSink::dial_sink_extra_data() const { On 2017/02/11 01:00:22, imcheng wrote: > Suggest making these return const ref, and turn the if statement into a CHECK. Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/typed_media_sink.h (right): https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:22: int capabilities; On 2017/02/11 01:00:22, imcheng wrote: > = 0; Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:25: int cast_channel_id; On 2017/02/11 01:00:22, imcheng wrote: > ditto Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:28: // Represents a media sink discovered by MediaSinkService. On 2017/02/11 01:00:22, imcheng wrote: > Comment that this is only used by MediaSinkService to push MediaSinks to the > Media Route Provider? Done. https://codereview.chromium.org/2675033002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/typed_media_sink.h:39: TypedMediaSink(const MediaSink::Id& sink_id, On 2017/02/11 01:00:22, imcheng wrote: > How about these two ctors in addition to the empty ctor required by Mojo: > > TypedMediaSink(const MediaSink& sink, const DialSinkExtraData& dial_data) > TypedMediaSink(const MediaSink& sink, const CastSinkExtraData& cast_data) Done.
https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:36: struct TypedMediaSink { Does this need to be distinct from MediaSink? If they were merged, we could typemap the combined MediaSink to a C++ object MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink struct for the public Media Router API. This would reduce the amount of typemapping code that needs to be maintained. The downside is that cloud media sinks passed from MRP to the MR would not be able to fill all the fields. But it seems like you will need to handle that case in the MR anyway. WDYT? https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:58: int32 capabilities; Can this be defined further? What do the bit positions mean? https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.typemap (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.typemap:34: "media_router.mojom.RouteRequestResultCode=media_router::RouteRequestResult::ResultCode", Can these be sorted? https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:123: DCHECK(false); NOTREACHED()
https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:36: struct TypedMediaSink { On 2017/02/17 02:22:31, mark a. foltz wrote: > Does this need to be distinct from MediaSink? > > If they were merged, we could typemap the combined MediaSink to a C++ object > MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink struct for > the public Media Router API. This would reduce the amount of typemapping code > that needs to be maintained. > > The downside is that cloud media sinks passed from MRP to the MR would not be > able to fill all the fields. But it seems like you will need to handle that > case in the MR anyway. > > WDYT? It seems doing this will weaken the validation provided by Mojo, since we would have to make all of these fields optional, in order to accommodate the OnSinksReceived use case. IMO, the extra type map for TypedMediaSink isn't too bad since it mostly reuses the MediaSink typemap.
https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:13: enum IconType { Does this generated a nested class enum in the mojom.h? It would save some typing to hoist this to the top level. (Not to fix in this patch.) https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:36: struct TypedMediaSink { On 2017/02/17 at 19:56:16, imcheng wrote: > On 2017/02/17 02:22:31, mark a. foltz wrote: > > Does this need to be distinct from MediaSink? > > > > If they were merged, we could typemap the combined MediaSink to a C++ object > > MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink struct for > > the public Media Router API. This would reduce the amount of typemapping code > > that needs to be maintained. > > > > The downside is that cloud media sinks passed from MRP to the MR would not be > > able to fill all the fields. But it seems like you will need to handle that > > case in the MR anyway. > > > > WDYT? > > It seems doing this will weaken the validation provided by Mojo, since we would have to make all of these fields optional, in order to accommodate the OnSinksReceived use case. > > IMO, the extra type map for TypedMediaSink isn't too bad since it mostly reuses the MediaSink typemap. I see. However we could move all the additional fields introduced in TypedMediaSink to the type-specific structs. CastMediaSink ip_address (is this needed since the MRP gets the cast channel id?) model_name (for feedback?) capabilities DialMediaSink ip_address (is this needed since the MRP gets the app_url for launch/stop?) model_name (for feedback?) CloudMediaSink domain This way the same container struct is passed to/from the MRP through both APIs, fewer typemaps are written, and each sink type defines exactly the fields it needs (albeit with a couple of duplicates). I feel this is conceptually cleaner and less complex in the long run.
On 2017/02/21 17:32:37, mark a. foltz wrote: > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > File chrome/browser/media/router/mojo/media_router.mojom (right): > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > chrome/browser/media/router/mojo/media_router.mojom:13: enum IconType { > Does this generated a nested class enum in the mojom.h? It would save some > typing to hoist this to the top level. (Not to fix in this patch.) > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > chrome/browser/media/router/mojo/media_router.mojom:36: struct TypedMediaSink { > On 2017/02/17 at 19:56:16, imcheng wrote: > > On 2017/02/17 02:22:31, mark a. foltz wrote: > > > Does this need to be distinct from MediaSink? > > > > > > If they were merged, we could typemap the combined MediaSink to a C++ object > > > MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink struct > for > > > the public Media Router API. This would reduce the amount of typemapping > code > > > that needs to be maintained. > > > > > > The downside is that cloud media sinks passed from MRP to the MR would not > be > > > able to fill all the fields. But it seems like you will need to handle that > > > case in the MR anyway. > > > > > > WDYT? > > > > It seems doing this will weaken the validation provided by Mojo, since we > would have to make all of these fields optional, in order to accommodate the > OnSinksReceived use case. > > > > IMO, the extra type map for TypedMediaSink isn't too bad since it mostly > reuses the MediaSink typemap. > > I see. However we could move all the additional fields introduced in > TypedMediaSink to the type-specific structs. > > CastMediaSink > ip_address (is this needed since the MRP gets the cast channel id?) > model_name (for feedback?) > capabilities > > DialMediaSink > ip_address (is this needed since the MRP gets the app_url for launch/stop?) > model_name (for feedback?) > > CloudMediaSink > domain > > This way the same container struct is passed to/from the MRP through both APIs, > fewer typemaps are written, and each sink type defines exactly the fields it > needs (albeit with a couple of duplicates). I feel this is conceptually cleaner > and less complex in the long run. Ok. I tend to agree that it will be cleaner if we push the duplicated fields down to the type-specific structs (resulting in fewer optional fields in the combined struct). The domain field is interesting because it is currently already defined in the MediaSink struct on both side -- maybe we can migrate that separately?
On 2017/02/21 at 18:53:09, imcheng wrote: > On 2017/02/21 17:32:37, mark a. foltz wrote: > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > > File chrome/browser/media/router/mojo/media_router.mojom (right): > > > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > > chrome/browser/media/router/mojo/media_router.mojom:13: enum IconType { > > Does this generated a nested class enum in the mojom.h? It would save some > > typing to hoist this to the top level. (Not to fix in this patch.) > > > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > > chrome/browser/media/router/mojo/media_router.mojom:36: struct TypedMediaSink { > > On 2017/02/17 at 19:56:16, imcheng wrote: > > > On 2017/02/17 02:22:31, mark a. foltz wrote: > > > > Does this need to be distinct from MediaSink? > > > > > > > > If they were merged, we could typemap the combined MediaSink to a C++ object > > > > MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink struct > > for > > > > the public Media Router API. This would reduce the amount of typemapping > > code > > > > that needs to be maintained. > > > > > > > > The downside is that cloud media sinks passed from MRP to the MR would not > > be > > > > able to fill all the fields. But it seems like you will need to handle that > > > > case in the MR anyway. > > > > > > > > WDYT? > > > > > > It seems doing this will weaken the validation provided by Mojo, since we > > would have to make all of these fields optional, in order to accommodate the > > OnSinksReceived use case. > > > > > > IMO, the extra type map for TypedMediaSink isn't too bad since it mostly > > reuses the MediaSink typemap. > > > > I see. However we could move all the additional fields introduced in > > TypedMediaSink to the type-specific structs. > > > > CastMediaSink > > ip_address (is this needed since the MRP gets the cast channel id?) > > model_name (for feedback?) > > capabilities > > > > DialMediaSink > > ip_address (is this needed since the MRP gets the app_url for launch/stop?) > > model_name (for feedback?) > > > > CloudMediaSink > > domain > > > > This way the same container struct is passed to/from the MRP through both APIs, > > fewer typemaps are written, and each sink type defines exactly the fields it > > needs (albeit with a couple of duplicates). I feel this is conceptually cleaner > > and less complex in the long run. > > Ok. I tend to agree that it will be cleaner if we push the duplicated fields down to the type-specific structs (resulting in fewer optional fields in the combined struct). The domain field is interesting because it is currently already defined in the MediaSink struct on both side -- maybe we can migrate that separately? Sure. Just so we're on the same page: - No more TypedMediaSink - Mojo MediaSink (with new union member) gets typemapped to MediaSinkInternal subclass (in C++) and to existing Javascript MediaSink in media_router_bindings.js - Code in MR converts MediaSinkInternal to C++ MediaSink for MediaRouter public API (so that UI and content don't have to know about type-specific details) - Later we can migrate domain to CloudMediaSink struct
On 2017/02/21 19:03:21, mark a. foltz wrote: > On 2017/02/21 at 18:53:09, imcheng wrote: > > On 2017/02/21 17:32:37, mark a. foltz wrote: > > > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > > > File chrome/browser/media/router/mojo/media_router.mojom (right): > > > > > > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > > > chrome/browser/media/router/mojo/media_router.mojom:13: enum IconType { > > > Does this generated a nested class enum in the mojom.h? It would save some > > > typing to hoist this to the top level. (Not to fix in this patch.) > > > > > > > https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... > > > chrome/browser/media/router/mojo/media_router.mojom:36: struct > TypedMediaSink { > > > On 2017/02/17 at 19:56:16, imcheng wrote: > > > > On 2017/02/17 02:22:31, mark a. foltz wrote: > > > > > Does this need to be distinct from MediaSink? > > > > > > > > > > If they were merged, we could typemap the combined MediaSink to a C++ > object > > > > > MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink > struct > > > for > > > > > the public Media Router API. This would reduce the amount of > typemapping > > > code > > > > > that needs to be maintained. > > > > > > > > > > The downside is that cloud media sinks passed from MRP to the MR would > not > > > be > > > > > able to fill all the fields. But it seems like you will need to handle > that > > > > > case in the MR anyway. > > > > > > > > > > WDYT? > > > > > > > > It seems doing this will weaken the validation provided by Mojo, since we > > > would have to make all of these fields optional, in order to accommodate the > > > OnSinksReceived use case. > > > > > > > > IMO, the extra type map for TypedMediaSink isn't too bad since it mostly > > > reuses the MediaSink typemap. > > > > > > I see. However we could move all the additional fields introduced in > > > TypedMediaSink to the type-specific structs. > > > > > > CastMediaSink > > > ip_address (is this needed since the MRP gets the cast channel id?) > > > model_name (for feedback?) > > > capabilities > > > > > > DialMediaSink > > > ip_address (is this needed since the MRP gets the app_url for > launch/stop?) > > > model_name (for feedback?) > > > > > > CloudMediaSink > > > domain > > > > > > This way the same container struct is passed to/from the MRP through both > APIs, > > > fewer typemaps are written, and each sink type defines exactly the fields it > > > needs (albeit with a couple of duplicates). I feel this is conceptually > cleaner > > > and less complex in the long run. > > > > Ok. I tend to agree that it will be cleaner if we push the duplicated fields > down to the type-specific structs (resulting in fewer optional fields in the > combined struct). The domain field is interesting because it is currently > already defined in the MediaSink struct on both side -- maybe we can migrate > that separately? > > Sure. Just so we're on the same page: > > - No more TypedMediaSink > - Mojo MediaSink (with new union member) gets typemapped to MediaSinkInternal > subclass (in C++) and to existing Javascript MediaSink in > media_router_bindings.js > - Code in MR converts MediaSinkInternal to C++ MediaSink for MediaRouter public > API (so that UI and content don't have to know about type-specific details) > - Later we can migrate domain to CloudMediaSink struct SG.
https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:13: enum IconType { On 2017/02/21 17:32:37, mark a. foltz wrote: > Does this generated a nested class enum in the mojom.h? It would save some > typing to hoist this to the top level. (Not to fix in this patch.) Created crbug.com/694841. https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:36: struct TypedMediaSink { On 2017/02/21 17:32:37, mark a. foltz wrote: > On 2017/02/17 at 19:56:16, imcheng wrote: > > On 2017/02/17 02:22:31, mark a. foltz wrote: > > > Does this need to be distinct from MediaSink? > > > > > > If they were merged, we could typemap the combined MediaSink to a C++ object > > > MediaSinkInternal (replacing TypedMediaSink) and retain the MediaSink struct > for > > > the public Media Router API. This would reduce the amount of typemapping > code > > > that needs to be maintained. > > > > > > The downside is that cloud media sinks passed from MRP to the MR would not > be > > > able to fill all the fields. But it seems like you will need to handle that > > > case in the MR anyway. > > > > > > WDYT? > > > > It seems doing this will weaken the validation provided by Mojo, since we > would have to make all of these fields optional, in order to accommodate the > OnSinksReceived use case. > > > > IMO, the extra type map for TypedMediaSink isn't too bad since it mostly > reuses the MediaSink typemap. > > I see. However we could move all the additional fields introduced in > TypedMediaSink to the type-specific structs. > > CastMediaSink > ip_address (is this needed since the MRP gets the cast channel id?) > model_name (for feedback?) > capabilities > > DialMediaSink > ip_address (is this needed since the MRP gets the app_url for launch/stop?) > model_name (for feedback?) > > CloudMediaSink > domain > > This way the same container struct is passed to/from the MRP through both APIs, > fewer typemaps are written, and each sink type defines exactly the fields it > needs (albeit with a couple of duplicates). I feel this is conceptually cleaner > and less complex in the long run. > > > > > Done. https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:58: int32 capabilities; On 2017/02/17 02:22:31, mark a. foltz wrote: > Can this be defined further? What do the bit positions mean? Done. https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.typemap (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.typemap:34: "media_router.mojom.RouteRequestResultCode=media_router::RouteRequestResult::ResultCode", On 2017/02/17 02:22:31, mark a. foltz wrote: > Can these be sorted? Done. https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:123: DCHECK(false); On 2017/02/17 02:22:31, mark a. foltz wrote: > NOTREACHED() Done.
PTAL
https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:82: // Set this field if |type_| == DIAL. 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.
https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.cc:29: type_ = SinkType::GENERIC; Hmm... so set_sink changes the type but not the extra data fields? I think it would also work if we made sure |type_| is currently GENERIC when calling set_{dial,cast}_data instead. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:46: // Represents a media sink discovered by MediaSinkService. 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. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:82: // Set this field if |type_| == DIAL. s/this field// here and below. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:82: // Set this field if |type_| == DIAL. On 2017/02/24 02:19:15, zhaobin wrote: > 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. 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. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:32: MediaSinkExtraData? extra_data; Mention that this is currently only set by MediaRouter in OnSinksDiscovered(). https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:44: string? model_name; I think this is no longer optional https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:54: string? model_name; Same as above; not optional https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:68: // used to send call chrome.cast.channel.send. nit: the second sentence is probably not necessary. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: media_router::MediaSink sink; Setting fields directly on out->sink would be more efficient. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits_unittest.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2675033002/diff/200001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2675033002/diff/200001/extensions/renderer/di... extensions/renderer/dispatcher.cc:750: {"url/mojo/url.mojom", IDR_MOJO_URL_MOJOM_JS}, nit: Could you please sort this? https://codereview.chromium.org/2675033002/diff/200001/extensions/renderer/di... extensions/renderer/dispatcher.cc:750: {"url/mojo/url.mojom", IDR_MOJO_URL_MOJOM_JS}, You might also need to update extensions/renderer/resources/extensions_renderer_resources.grd with the new resource when you update media_router_bindings.js.
Overall looks really good!!! Mostly minor comments - didn't review unittest closely this round. The component isn't trusted so it would be good to validate the data it is sending back. I am okay with stricter validations on the expectation that any component bugs that would trigger them would be caught quickly; killing the extension process should cause something user visible to break or the e2e/browser tests to break right away. But there might be some gray areas in terms of how strictly to verify ID syntax etc. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.cc:33: void MediaSinkInternal::set_dial_data(const DialSinkExtraData& dial_data) { I don't think you should be able to change the type of a sink from CAST to DIAL or vice versa. Can you DCHECK(dial_data_ || !cast_data_)? https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.cc:40: DCHECK(dial_data_.has_value()); This DCHECK is redundant with the one in Optional. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:35: // A bit vector representing the capabilities of the sink. Can you mention that the values are defined in the mojom? https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:36: int capabilities = 0; Is the default a valid set of capabilities, or must the user set it to some value? https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:39: int cast_channel_id = 0; 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. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:47: class MediaSinkInternal { 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. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:51: enum class SinkType { This can be inferred from which of the optional fields have values - so don't need to define an enum. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:61: MediaSinkInternal(const MediaSinkInternal& other); 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. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:82: // Set this field if |type_| == DIAL. On 2017/02/24 at 23:54:20, imcheng wrote: > s/this field// here and below. base::Optional<> is a good solution here. It's what I used for PresentationConnectionMessage. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:68: // used to send call chrome.cast.channel.send. On 2017/02/24 at 23:54:21, imcheng wrote: > nit: the second sentence is probably not necessary. nit: Cast I think it would be good to mention that the ID is defined by the chrome.cast.channel API. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:187: std::vector<MediaSink> sinks; nit: reserve length to accommodate internal_sinks.size() https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:54: if (!data.ReadSinkId(&id)) Enforce non-empty sink_id made up of valid characters? https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:114: if (!data.ReadIpAddress(&out->ip_address)) Enforce valid ip_address? https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:131: if (!data.ReadIpAddress(&out->ip_address)) Enforce valid ip_address? https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:100: static bool IsNull(const media_router::MediaSinkInternal& sink) { If we aren't passing any null sinks you don't need to define IsNull().
https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.cc:29: type_ = SinkType::GENERIC; On 2017/02/24 23:54:20, imcheng wrote: > Hmm... so set_sink changes the type but not the extra data fields? > I think it would also work if we made sure |type_| is currently GENERIC when > calling set_{dial,cast}_data instead. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.cc:33: void MediaSinkInternal::set_dial_data(const DialSinkExtraData& dial_data) { On 2017/02/25 01:10:23, mark a. foltz wrote: > I don't think you should be able to change the type of a sink from CAST to DIAL > or vice versa. > Can you DCHECK(dial_data_ || !cast_data_)? Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.cc:40: DCHECK(dial_data_.has_value()); On 2017/02/25 01:10:23, mark a. foltz wrote: > This DCHECK is redundant with the one in Optional. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:35: // A bit vector representing the capabilities of the sink. On 2017/02/25 01:10:24, mark a. foltz wrote: > Can you mention that the values are defined in the mojom? Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:36: int capabilities = 0; On 2017/02/25 01:10:24, mark a. foltz wrote: > Is the default a valid set of capabilities, or must the user set it to some > value? Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:39: int cast_channel_id = 0; On 2017/02/25 01:10:23, mark a. foltz wrote: > 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. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:46: // Represents a media sink discovered by MediaSinkService. On 2017/02/24 23:54:20, imcheng wrote: > 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. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:47: class MediaSinkInternal { On 2017/02/25 01:10:23, mark a. foltz wrote: > 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. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:51: enum class SinkType { On 2017/02/25 01:10:24, mark a. foltz wrote: > This can be inferred from which of the optional fields have values - so don't > need to define an enum. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:61: MediaSinkInternal(const MediaSinkInternal& other); On 2017/02/25 01:10:24, mark a. foltz wrote: > 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. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_internal.h:82: // Set this field if |type_| == DIAL. On 2017/02/24 23:54:20, imcheng wrote: > s/this field// here and below. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:32: MediaSinkExtraData? extra_data; On 2017/02/24 23:54:21, imcheng wrote: > Mention that this is currently only set by MediaRouter in OnSinksDiscovered(). Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:44: string? model_name; On 2017/02/24 23:54:21, imcheng wrote: > I think this is no longer optional Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:54: string? model_name; On 2017/02/24 23:54:21, imcheng wrote: > Same as above; not optional Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:68: // used to send call chrome.cast.channel.send. On 2017/02/24 23:54:21, imcheng wrote: > nit: the second sentence is probably not necessary. Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:187: std::vector<MediaSink> sinks; On 2017/02/25 01:10:24, mark a. foltz wrote: > nit: reserve length to accommodate internal_sinks.size() Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: media_router::MediaSink sink; On 2017/02/24 23:54:21, imcheng wrote: > Setting fields directly on out->sink would be more efficient. out->sink() returns const MediaSink&. It seems ok to return const reference for MediaSinkInternal::sink(). https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:54: if (!data.ReadSinkId(&id)) On 2017/02/25 01:10:24, mark a. foltz wrote: > Enforce non-empty sink_id made up of valid characters? Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:114: if (!data.ReadIpAddress(&out->ip_address)) On 2017/02/25 01:10:24, mark a. foltz wrote: > Enforce valid ip_address? Done. Note: MRP should not reach this code and send DialMediaSink back to MR. Put Read() here for completeness and unit test... https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:131: if (!data.ReadIpAddress(&out->ip_address)) On 2017/02/25 01:10:24, mark a. foltz wrote: > Enforce valid ip_address? Done. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:100: static bool IsNull(const media_router::MediaSinkInternal& sink) { On 2017/02/25 01:10:24, mark a. foltz wrote: > If we aren't passing any null sinks you don't need to define IsNull(). This is needed so Generic sink does not read extra_data field when serializing to mojo object. https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits_unittest.cc (right): https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/24 23:54:21, imcheng wrote: > 2017 Done. https://codereview.chromium.org/2675033002/diff/200001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2675033002/diff/200001/extensions/renderer/di... extensions/renderer/dispatcher.cc:750: {"url/mojo/url.mojom", IDR_MOJO_URL_MOJOM_JS}, On 2017/02/24 23:54:21, imcheng wrote: > nit: Could you please sort this? Done. https://codereview.chromium.org/2675033002/diff/200001/extensions/renderer/di... extensions/renderer/dispatcher.cc:750: {"url/mojo/url.mojom", IDR_MOJO_URL_MOJOM_JS}, On 2017/02/24 23:54:21, imcheng wrote: > You might also need to update > extensions/renderer/resources/extensions_renderer_resources.grd with the new > resource when you update media_router_bindings.js. Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:13: return (('A' <= ch) && (ch <= 'Z')) || (('a' <= ch) && (ch <= 'z')) || This check passes for our cloud view devices. Not sure if it is sufficient...
Getting close https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:12: bool IsValidSinkIdChar(char ch) { I would just simplify this to base::IsStringAscii for now https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:13: // Extra data for dial media sink. nit: DIAL https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:15: std::string ip_address; net::IPAddress would be more accurate, type-wise, and allow you to enforce validity of data passed from the MRP. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:30: std::string ip_address; net::IPAddress https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:37: // capacity set. Is 0 a valid set? https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:41: // valid cast_channel_id. The cast_channel_id may change over time as the Is 0 a valid id? IIRC the channel_ids start at 1, but I'm not sure. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:55: explicit MediaSinkInternal(const MediaSink& sink); Can you add a one-line comment explaining when you would use each of these ctors? https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:58: MediaSinkInternal(); nit: Can you put this ctor first in the list? https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:68: void set_dial_data(const DialSinkExtraData& dial_data); Add comments explaining that the getters must only be called if the sink is a DIAL/Cast sink. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:77: static bool IsValidIPAddress(const std::string& ip_address); See comment above about using net::IPAddress. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:82: // Set if sink is Dial sink. nit: DIAL https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:140: static base::Optional<std::string> model_name( I think model_name is now mandatory
https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:12: bool IsValidSinkIdChar(char ch) { On 2017/02/28 00:05:29, mark a. foltz wrote: > I would just simplify this to base::IsStringAscii for now Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:13: // Extra data for dial media sink. On 2017/02/28 00:05:29, mark a. foltz wrote: > nit: DIAL Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:15: std::string ip_address; On 2017/02/28 00:05:30, mark a. foltz wrote: > net::IPAddress would be more accurate, type-wise, and allow you to enforce > validity of data passed from the MRP. Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:30: std::string ip_address; On 2017/02/28 00:05:29, mark a. foltz wrote: > net::IPAddress Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:37: // capacity set. On 2017/02/28 00:05:29, mark a. foltz wrote: > Is 0 a valid set? Yes. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:41: // valid cast_channel_id. The cast_channel_id may change over time as the On 2017/02/28 00:05:29, mark a. foltz wrote: > Is 0 a valid id? IIRC the channel_ids start at 1, but I'm not sure. No. It will be populated when we successfully open a cast channel. ChannelInfo default ctor also init channel_id to 0. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:55: explicit MediaSinkInternal(const MediaSink& sink); On 2017/02/28 00:05:29, mark a. foltz wrote: > Can you add a one-line comment explaining when you would use each of these > ctors? Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:58: MediaSinkInternal(); On 2017/02/28 00:05:29, mark a. foltz wrote: > nit: Can you put this ctor first in the list? Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:68: void set_dial_data(const DialSinkExtraData& dial_data); On 2017/02/28 00:05:29, mark a. foltz wrote: > Add comments explaining that the getters must only be called if the sink is a > DIAL/Cast sink. Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:77: static bool IsValidIPAddress(const std::string& ip_address); On 2017/02/28 00:05:29, mark a. foltz wrote: > See comment above about using net::IPAddress. Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:82: // Set if sink is Dial sink. On 2017/02/28 00:05:29, mark a. foltz wrote: > nit: DIAL Done. https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:140: static base::Optional<std::string> model_name( On 2017/02/28 00:05:30, mark a. foltz wrote: > I think model_name is now mandatory Done.
LGTM with a few remaining comments addressed. Thanks for unit testing the struct traits. https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:36: DCHECK(dial_data_ || !cast_data_); Just DCHECK(!cast_data_)? https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:41: DCHECK(is_dial_sink()); This is already done by the * operator on base::Optional. https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:46: DCHECK(cast_data_ || !dial_data_); I think this should just be DCHECK(!dial_data_)? Is it okay to set cast_data regardless of the previous value? https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:51: DCHECK(is_cast_sink()); See above.
zhaobin@chromium.org changed reviewers: + dcheng@chromium.org, rdevlin.cronin@chromium.org
+dcheng@ back for mojo related changes. +rdevlin.cronin@ for extensions related changes. extensions/BUILD.gn extensions/renderer/dispatcher.cc extensions/renderer/resources/extensions_renderer_resources.grd https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:36: DCHECK(dial_data_ || !cast_data_); On 2017/03/01 04:53:19, mark a. foltz wrote: > Just DCHECK(!cast_data_)? Done. https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:41: DCHECK(is_dial_sink()); On 2017/03/01 04:53:19, mark a. foltz wrote: > This is already done by the * operator on base::Optional. Done. https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:46: DCHECK(cast_data_ || !dial_data_); On 2017/03/01 04:53:19, mark a. foltz wrote: > I think this should just be DCHECK(!dial_data_)? Is it okay to set cast_data > regardless of the previous value? Done. https://codereview.chromium.org/2675033002/diff/240001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:51: DCHECK(is_cast_sink()); On 2017/03/01 04:53:19, mark a. foltz wrote: > See above. Done.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:18: // Model name of the sink nit: add period at end of sentence. https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:34: std::string model_name; nit: add period at end of sentence. https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:37: // defined in media_router.mojom. The caller must set this value to a valid Since the default value of 0 is valid, do we still need the last sentence? https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:73: // Set |sink_| and invalidate extra data. Remove this comment since the second part no longer applies and first part is self explanatory. https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:187: std::vector<MediaSink> sinks(internal_sinks.size()); This actually creates a vector with N default-constructed MediaSinks. I think what you want is: std::vector<MediaSink> sinks; sinks.reserve(internal_sinks.size());
extensions lgtm
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:93: // Set if sink is DIAL sink. One option (that results in a smaller object) is to use a C++11 union + an enum tag, which will make the checks on 85-86 simpler (and make it more obvious that there can only be one type of extra data). https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; Can we add a typemap for net::IPAddress? We already have one for net::IPEndpoint: https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: media_router::MediaSink sink; Any particular reason for using a local? |out| is never touched if Read() returns false. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:87: const media_router::MediaSinkInternal& sink) { This is probably large enough that I would consider out-of-lining.
https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:18: // Model name of the sink On 2017/03/01 22:54:18, imcheng wrote: > nit: add period at end of sentence. Done. https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:34: std::string model_name; On 2017/03/01 22:54:18, imcheng wrote: > nit: add period at end of sentence. Done. https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:37: // defined in media_router.mojom. The caller must set this value to a valid On 2017/03/01 22:54:18, imcheng wrote: > Since the default value of 0 is valid, do we still need the last sentence? Done. https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:187: std::vector<MediaSink> sinks(internal_sinks.size()); On 2017/03/01 22:54:18, imcheng wrote: > This actually creates a vector with N default-constructed MediaSinks. I think > what you want is: > > std::vector<MediaSink> sinks; > sinks.reserve(internal_sinks.size()); Thanks a lot! https://codereview.chromium.org/2675033002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:187: std::vector<MediaSink> sinks(internal_sinks.size()); On 2017/03/01 22:54:18, imcheng wrote: > This actually creates a vector with N default-constructed MediaSinks. I think > what you want is: > > std::vector<MediaSink> sinks; > sinks.reserve(internal_sinks.size()); Done. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:93: // Set if sink is DIAL sink. On 2017/03/03 10:37:10, dcheng wrote: > One option (that results in a smaller object) is to use a C++11 union + an enum > tag, which will make the checks on 85-86 simpler (and make it more obvious that > there can only be one type of extra data). We tried union. It is a little complicated to construct & destruct objects with non-trivial constructors with union. base::Optional seems cleaner (https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r...) https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; On 2017/03/03 10:37:10, dcheng wrote: > Can we add a typemap for net::IPAddress? We already have one for > net::IPEndpoint: > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... Do it in a seperate patch? Created crbug.com/698423. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: media_router::MediaSink sink; On 2017/03/03 10:37:10, dcheng wrote: > Any particular reason for using a local? |out| is never touched if Read() > returns false. out->sink() returns const ref. Besides, it seems ok to use a local variable here. sink is a whole object. We either set it or not set it, instead of setting partial fields. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:87: const media_router::MediaSinkInternal& sink) { On 2017/03/03 10:37:10, dcheng wrote: > This is probably large enough that I would consider out-of-lining. Done.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:93: // Set if sink is DIAL sink. On 2017/03/03 23:56:42, zhaobin wrote: > On 2017/03/03 10:37:10, dcheng wrote: > > One option (that results in a smaller object) is to use a C++11 union + an > enum > > tag, which will make the checks on 85-86 simpler (and make it more obvious > that > > there can only be one type of extra data). > > We tried union. It is a little complicated to construct & destruct objects with > non-trivial constructors with union. base::Optional seems cleaner > (https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r...) Hmm... it's still preferable here though: with this, it's hard to tell that they're mutually exclusive. If you object to writing this out, you can union using ManualConstructor<T> to make it look nicer. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; On 2017/03/03 23:56:42, zhaobin wrote: > On 2017/03/03 10:37:10, dcheng wrote: > > Can we add a typemap for net::IPAddress? We already have one for > > net::IPEndpoint: > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > Do it in a seperate patch? Created crbug.com/698423. The reason I mention is because mojo struct trait getters are currently called twice on serialization. So IPAddress::ToString() gets called twice, + deserialization is more complicated. It's true that this code is probably not perf critical, but where possible, we should try to use best practices throughout the code. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: media_router::MediaSink sink; On 2017/03/03 23:56:42, zhaobin wrote: > On 2017/03/03 10:37:10, dcheng wrote: > > Any particular reason for using a local? |out| is never touched if Read() > > returns false. > > out->sink() returns const ref. Besides, it seems ok to use a local variable > here. sink is a whole object. We either set it or not set it, instead of setting > partial fields. It doesn't really matter though: on failure, |out| is just completely ignored. It's an extra copy that ideally shouldn't be needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/04 01:49:16, dcheng wrote: > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > File chrome/browser/media/router/discovery/media_sink_internal.h (right): > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > chrome/browser/media/router/discovery/media_sink_internal.h:93: // Set if sink > is DIAL sink. > On 2017/03/03 23:56:42, zhaobin wrote: > > On 2017/03/03 10:37:10, dcheng wrote: > > > One option (that results in a smaller object) is to use a C++11 union + an > > enum > > > tag, which will make the checks on 85-86 simpler (and make it more obvious > > that > > > there can only be one type of extra data). > > > > We tried union. It is a little complicated to construct & destruct objects > with > > non-trivial constructors with union. base::Optional seems cleaner > > > (https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r...) > > Hmm... it's still preferable here though: with this, it's hard to tell that > they're mutually exclusive. If you object to writing this out, you can union > using ManualConstructor<T> to make it look nicer. > I believe Bin also tried union with ManualConstructor and the resulting code is quite clunky. Since we aren't particular about performance in this code path, it seems using two base::Optional fields would be the simpler approach here. > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > File chrome/browser/media/router/mojo/media_router.mojom (right): > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; > On 2017/03/03 23:56:42, zhaobin wrote: > > On 2017/03/03 10:37:10, dcheng wrote: > > > Can we add a typemap for net::IPAddress? We already have one for > > > net::IPEndpoint: > > > > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > > > Do it in a seperate patch? Created crbug.com/698423. > > The reason I mention is because mojo struct trait getters are currently called > twice on serialization. So IPAddress::ToString() gets called twice, + > deserialization is more complicated. > > It's true that this code is probably not perf critical, but where possible, we > should try to use best practices throughout the code. > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: > media_router::MediaSink sink; > On 2017/03/03 23:56:42, zhaobin wrote: > > On 2017/03/03 10:37:10, dcheng wrote: > > > Any particular reason for using a local? |out| is never touched if Read() > > > returns false. > > > > out->sink() returns const ref. Besides, it seems ok to use a local variable > > here. sink is a whole object. We either set it or not set it, instead of > setting > > partial fields. > > It doesn't really matter though: on failure, |out| is just completely ignored. > It's an extra copy that ideally shouldn't be needed.
On 2017/03/04 03:07:14, imcheng wrote: > On 2017/03/04 01:49:16, dcheng wrote: > > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > > File chrome/browser/media/router/discovery/media_sink_internal.h (right): > > > > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > > chrome/browser/media/router/discovery/media_sink_internal.h:93: // Set if sink > > is DIAL sink. > > On 2017/03/03 23:56:42, zhaobin wrote: > > > On 2017/03/03 10:37:10, dcheng wrote: > > > > One option (that results in a smaller object) is to use a C++11 union + an > > > enum > > > > tag, which will make the checks on 85-86 simpler (and make it more obvious > > > that > > > > there can only be one type of extra data). > > > > > > We tried union. It is a little complicated to construct & destruct objects > > with > > > non-trivial constructors with union. base::Optional seems cleaner > > > > > > (https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r...) > > > > Hmm... it's still preferable here though: with this, it's hard to tell that > > they're mutually exclusive. If you object to writing this out, you can union > > using ManualConstructor<T> to make it look nicer. > > > > I believe Bin also tried union with ManualConstructor and the resulting code is > quite clunky. Since we aren't particular about performance in this code path, it > seems using two base::Optional fields would be the simpler approach here. It's not about performance though; it's about making it easy to understand the invariants. > > > > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > > File chrome/browser/media/router/mojo/media_router.mojom (right): > > > > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > > chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; > > On 2017/03/03 23:56:42, zhaobin wrote: > > > On 2017/03/03 10:37:10, dcheng wrote: > > > > Can we add a typemap for net::IPAddress? We already have one for > > > > net::IPEndpoint: > > > > > > > > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > > > > > Do it in a seperate patch? Created crbug.com/698423. > > > > The reason I mention is because mojo struct trait getters are currently called > > twice on serialization. So IPAddress::ToString() gets called twice, + > > deserialization is more complicated. > > > > It's true that this code is probably not perf critical, but where possible, we > > should try to use best practices throughout the code. > > > > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > > File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): > > > > > https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... > > chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: > > media_router::MediaSink sink; > > On 2017/03/03 23:56:42, zhaobin wrote: > > > On 2017/03/03 10:37:10, dcheng wrote: > > > > Any particular reason for using a local? |out| is never touched if Read() > > > > returns false. > > > > > > out->sink() returns const ref. Besides, it seems ok to use a local variable > > > here. sink is a whole object. We either set it or not set it, instead of > > setting > > > partial fields. > > > > It doesn't really matter though: on failure, |out| is just completely ignored. > > It's an extra copy that ideally shouldn't be needed.
https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:93: // Set if sink is DIAL sink. On 2017/03/04 01:49:15, dcheng wrote: > On 2017/03/03 23:56:42, zhaobin wrote: > > On 2017/03/03 10:37:10, dcheng wrote: > > > One option (that results in a smaller object) is to use a C++11 union + an > > enum > > > tag, which will make the checks on 85-86 simpler (and make it more obvious > > that > > > there can only be one type of extra data). > > > > We tried union. It is a little complicated to construct & destruct objects > with > > non-trivial constructors with union. base::Optional seems cleaner > > > (https://codereview.chromium.org/2675033002/diff/200001/chrome/browser/media/r...) > > Hmm... it's still preferable here though: with this, it's hard to tell that > they're mutually exclusive. If you object to writing this out, you can union > using ManualConstructor<T> to make it look nicer. Done. https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; On 2017/03/04 01:49:16, dcheng wrote: > On 2017/03/03 23:56:42, zhaobin wrote: > > On 2017/03/03 10:37:10, dcheng wrote: > > > Can we add a typemap for net::IPAddress? We already have one for > > > net::IPEndpoint: > > > > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > > > Do it in a seperate patch? Created crbug.com/698423. > > The reason I mention is because mojo struct trait getters are currently called > twice on serialization. So IPAddress::ToString() gets called twice, + > deserialization is more complicated. > > It's true that this code is probably not perf critical, but where possible, we > should try to use best practices throughout the code. Added IPAddress here. host_resolver_service.mojom has this warning: "// WARNING! Do NOT use this mojom". Adding IPAddress here might not work. Any suggestions? https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:51: media_router::MediaSink sink; On 2017/03/04 01:49:16, dcheng wrote: > On 2017/03/03 23:56:42, zhaobin wrote: > > On 2017/03/03 10:37:10, dcheng wrote: > > > Any particular reason for using a local? |out| is never touched if Read() > > > returns false. > > > > out->sink() returns const ref. Besides, it seems ok to use a local variable > > here. sink is a whole object. We either set it or not set it, instead of > setting > > partial fields. > > It doesn't really matter though: on failure, |out| is just completely ignored. > It's an extra copy that ideally shouldn't be needed. Done.
https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; On 2017/03/06 18:31:26, zhaobin wrote: > On 2017/03/04 01:49:16, dcheng wrote: > > On 2017/03/03 23:56:42, zhaobin wrote: > > > On 2017/03/03 10:37:10, dcheng wrote: > > > > Can we add a typemap for net::IPAddress? We already have one for > > > > net::IPEndpoint: > > > > > > > > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > > > > > Do it in a seperate patch? Created crbug.com/698423. > > > > The reason I mention is because mojo struct trait getters are currently called > > twice on serialization. So IPAddress::ToString() gets called twice, + > > deserialization is more complicated. > > > > It's true that this code is probably not perf critical, but where possible, we > > should try to use best practices throughout the code. > > Added IPAddress here. host_resolver_service.mojom has this warning: > "// WARNING! Do NOT use this mojom". Adding IPAddress here might not work. Any > suggestions? How about just creating a new mojom for the struct? https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:72: int32 capabilities; Nit: We usually prefer unsigned for bitmasks https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:116: if (extra_data_data_view.is_null()) { Is it possible to: - name the new union instead of making it anonymous - add a typemap entry for it then we can just use the struct traits automatic dispatching instead of having to manually dispatch here.
https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; On 2017/03/06 23:02:05, dcheng wrote: > On 2017/03/06 18:31:26, zhaobin wrote: > > On 2017/03/04 01:49:16, dcheng wrote: > > > On 2017/03/03 23:56:42, zhaobin wrote: > > > > On 2017/03/03 10:37:10, dcheng wrote: > > > > > Can we add a typemap for net::IPAddress? We already have one for > > > > > net::IPEndpoint: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > > > > > > > Do it in a seperate patch? Created crbug.com/698423. > > > > > > The reason I mention is because mojo struct trait getters are currently > called > > > twice on serialization. So IPAddress::ToString() gets called twice, + > > > deserialization is more complicated. > > > > > > It's true that this code is probably not perf critical, but where possible, > we > > > should try to use best practices throughout the code. > > > > Added IPAddress here. host_resolver_service.mojom has this warning: > > "// WARNING! Do NOT use this mojom". Adding IPAddress here might not work. Any > > suggestions? > > How about just creating a new mojom for the struct? Is it ok to leave struct IPAddress in media_router.mojom in this patch? Do not want to touch net/interface code in this patch. https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:72: int32 capabilities; On 2017/03/06 23:02:05, dcheng wrote: > Nit: We usually prefer unsigned for bitmasks Done. https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:116: if (extra_data_data_view.is_null()) { On 2017/03/06 23:02:05, dcheng wrote: > Is it possible to: > - name the new union instead of making it anonymous > - add a typemap entry for it > > then we can just use the struct traits automatic dispatching instead of having > to manually dispatch here. Done.
https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:41: string ip_address; On 2017/03/07 19:28:49, zhaobin wrote: > On 2017/03/06 23:02:05, dcheng wrote: > > On 2017/03/06 18:31:26, zhaobin wrote: > > > On 2017/03/04 01:49:16, dcheng wrote: > > > > On 2017/03/03 23:56:42, zhaobin wrote: > > > > > On 2017/03/03 10:37:10, dcheng wrote: > > > > > > Can we add a typemap for net::IPAddress? We already have one for > > > > > > net::IPEndpoint: > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/net/interfaces/host_resolver_service.moj... > > > > > > > > > > Do it in a seperate patch? Created crbug.com/698423. > > > > > > > > The reason I mention is because mojo struct trait getters are currently > > called > > > > twice on serialization. So IPAddress::ToString() gets called twice, + > > > > deserialization is more complicated. > > > > > > > > It's true that this code is probably not perf critical, but where > possible, > > we > > > > should try to use best practices throughout the code. > > > > > > Added IPAddress here. host_resolver_service.mojom has this warning: > > > "// WARNING! Do NOT use this mojom". Adding IPAddress here might not work. > Any > > > suggestions? > > > > How about just creating a new mojom for the struct? > > Is it ok to leave struct IPAddress in media_router.mojom in this patch? Do not > want to touch net/interface code in this patch. (Sorry just saw this comment) Hmm... it's not great to separate the mojom struct from where it's actually defined. I know it's nice not to have to touch //net, but it's better to just have it in the right place to begin with. https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:40: // Mirror of net::IPAddress. Sorry, I clarify, I meant that this struct should be defined in //net in a new mojom file: as long as it's outside the host resolver mojom, then I think that's fine. (I think I missed this in a previous review, so sorry for the trouble) https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:124: if (data.is_dial_media_sink()) { The body of this should look something like https://cs.chromium.org/chromium/src/cc/ipc/quads_struct_traits.h?rcl=f4ac7d7... https://codereview.chromium.org/2675033002/diff/380001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/380001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:52: struct StructTraits<net::interfaces::IPAddressDataView, net::IPAddress> { Basically, just take this and move it to a new mojom / struct traits, and that should be fine.
https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:40: // Mirror of net::IPAddress. On 2017/03/07 19:54:32, dcheng wrote: > Sorry, I clarify, I meant that this struct should be defined in //net in a new > mojom file: as long as it's outside the host resolver mojom, then I think that's > fine. > > (I think I missed this in a previous review, so sorry for the trouble) Done. https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.cc (right): https://codereview.chromium.org/2675033002/diff/380001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.cc:124: if (data.is_dial_media_sink()) { On 2017/03/07 19:54:32, dcheng wrote: > The body of this should look something like > https://cs.chromium.org/chromium/src/cc/ipc/quads_struct_traits.h?rcl=f4ac7d7... Done. https://codereview.chromium.org/2675033002/diff/380001/net/dns/mojo_host_stru... File net/dns/mojo_host_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/380001/net/dns/mojo_host_stru... net/dns/mojo_host_struct_traits.h:52: struct StructTraits<net::interfaces::IPAddressDataView, net::IPAddress> { On 2017/03/07 19:54:32, dcheng wrote: > Basically, just take this and move it to a new mojom / struct traits, and that > should be fine. Done.
Bin, could you please test with the MR extension to make sure the extension resources are loaded properly? Again I question the usefulness of introducing a union here. I understand it is more succinct from API's point of view, but I can't help but feel the amount of boilerplate code introduced is a bit of an overkill. IMO a good compromise would've been by providing documentation in the header file. Since the change has already been made, I am not going to argue the point further. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:55: if (!(sink_ == other.sink_)) Can this just use MediaSink::Equals? https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:66: NOTREACHED(); Hmm.. I thought you'd still have to have a return statement following a NOTREACHED()? https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:73: void MediaSinkInternal::set_dial_data(const DialSinkExtraData& dial_data) { Is there a case set_*_data would be called more than once? Maybe we can just DCHECK_EQ(SinkType::GENERIC, sink_type_) here? https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:115: enum class SinkType { GENERIC = 0, DIAL, CAST }; The = 0 is not necessary? https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal_unittest.cc (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal_unittest.cc:39: MediaSinkInternal copy_generic_sink(generic_sink); s/copy/copied https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal_unittest.cc:43: ASSERT_TRUE(generic_sink == copy_generic_sink); ASSERT_EQ(generic_sink, copied_generic_sink); similar below
dcheng@, it seems that imcheng@ and mfoltz@ both prefer base::Optional over union for extra data. I am fine to rever back. WDYT?
Bin: since you've already made the change to use union, I am fine with it staying that way.
lgtm I understand that union introduces some extra boilerplate, but for things that are serialized over IPC, it's ideal to have the underlying C++ types map as closely to the concepts as possible. Thanks for making the changes. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:93: static void SetToNull(media_router::MediaSinkInternal* out) {} Hmm... is it OK to leave this empty? I guess the default state is empty, so it's OK.
https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:39: if (this != &other) { The conversion to union seems to require all this extra code to do copy and assignment. What is the benefit? https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:55: if (!(sink_ == other.sink_)) On 2017/03/11 at 00:33:08, imcheng wrote: > Can this just use MediaSink::Equals? Equals() methods are discouraged vs. operator==. https://google.github.io/styleguide/cppguide.html#Operator_Overloading https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:34: // Extra data for cast media sink. nit: Cast https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:66: explicit MediaSinkInternal(const MediaSink& sink); If this is just for tests, it seems like this can be made a utility function in the test, since there are setters for individual sink fields. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:77: // Used by unit test. Not sure this comment is needed; also, these will be used outside of unit tests via STL.
Shall we change media_router.mojom to use optional field instead of union? So we can keep mojo consistent with c++ objects and remove extra code in MediaSinkInternal. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:39: if (this != &other) { On 2017/03/13 17:47:24, mark a. foltz wrote: > The conversion to union seems to require all this extra code to do copy and > assignment. What is the benefit? dcheng@ suggests that .mojom should be consistent with c++ object (if we use union in media_router.mojom, we need to use union in c++ object). Maybe we can change extra data to optional instead of union in mojom? https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:55: if (!(sink_ == other.sink_)) On 2017/03/11 00:33:08, imcheng wrote: > Can this just use MediaSink::Equals? MediaSink::Equals only compares IDs. Here we want to compare all fields, use != instead... https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:55: if (!(sink_ == other.sink_)) On 2017/03/13 17:47:24, mark a. foltz wrote: > On 2017/03/11 at 00:33:08, imcheng wrote: > > Can this just use MediaSink::Equals? > > Equals() methods are discouraged vs. operator==. > > https://google.github.io/styleguide/cppguide.html#Operator_Overloading Acknowledged. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:66: NOTREACHED(); On 2017/03/11 00:33:08, imcheng wrote: > Hmm.. I thought you'd still have to have a return statement following a > NOTREACHED()? Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.cc:73: void MediaSinkInternal::set_dial_data(const DialSinkExtraData& dial_data) { On 2017/03/11 00:33:08, imcheng wrote: > Is there a case set_*_data would be called more than once? Maybe we can just > DCHECK_EQ(SinkType::GENERIC, sink_type_) here? I think it is valid to call set_dial_data() multiple times: e.g. MediaSinkInternal media_sink(sink, dial_extra_data); media_sink.set_dial_data(dial_extra_data2); // DCHECK and crash this does not seem right. We may DCHECK sink_type_ is GENERIC or DIAL. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal.h (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:34: // Extra data for cast media sink. On 2017/03/13 17:47:24, mark a. foltz wrote: > nit: Cast Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:66: explicit MediaSinkInternal(const MediaSink& sink); On 2017/03/13 17:47:24, mark a. foltz wrote: > If this is just for tests, it seems like this can be made a utility function in > the test, since there are setters for individual sink fields. Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:77: // Used by unit test. On 2017/03/13 17:47:24, mark a. foltz wrote: > Not sure this comment is needed; also, these will be used outside of unit tests > via STL. Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal.h:115: enum class SinkType { GENERIC = 0, DIAL, CAST }; On 2017/03/11 00:33:08, imcheng wrote: > The = 0 is not necessary? Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/discovery/media_sink_internal_unittest.cc (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal_unittest.cc:39: MediaSinkInternal copy_generic_sink(generic_sink); On 2017/03/11 00:33:08, imcheng wrote: > s/copy/copied Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/discovery/media_sink_internal_unittest.cc:43: ASSERT_TRUE(generic_sink == copy_generic_sink); On 2017/03/11 00:33:08, imcheng wrote: > ASSERT_EQ(generic_sink, copied_generic_sink); > > similar below Done. https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_struct_traits.h (right): https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_struct_traits.h:93: static void SetToNull(media_router::MediaSinkInternal* out) {} On 2017/03/11 07:24:23, dcheng wrote: > Hmm... is it OK to leave this empty? I guess the default state is empty, so it's > OK. Acknowledged.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, rdevlin.cronin@chromium.org, imcheng@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2675033002/#ps420001 (title: "resolve code review comments from Derek and Mark")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1489598810849300,
"parent_rev": "b4f78964d73c1f967ce5d3e8ca914dcc41eec1e7", "commit_rev":
"bede2733a941e43cb8c7782ff74a2679a0ad86d3"}
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1489598810849300,
"parent_rev": "20f4feafa5d6c72cb25737bd87af559e5046c96d", "commit_rev":
"88c63227018cd614c0eab270dbc2c6aea43a79ec"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add MediaSink subtypes - Introduce subclasses of MediaSink (DialMediaSink and CastMediaSink) in C++ - Introduce Mojo struct definitions - Add typemap for MediaSink subclasses - Add unit tests for typemap conversion MediaSink typemap is based on https://codereview.chromium.org/2666873006/. Will merge when that lands. BUG=687365 ========== to ========== [Media Router] Add MediaSink subtypes - Introduce subclasses of MediaSink (DialMediaSink and CastMediaSink) in C++ - Introduce Mojo struct definitions - Add typemap for MediaSink subclasses - Add unit tests for typemap conversion MediaSink typemap is based on https://codereview.chromium.org/2666873006/. Will merge when that lands. BUG=687365 Review-Url: https://codereview.chromium.org/2675033002 Cr-Commit-Position: refs/heads/master@{#457123} Committed: https://chromium.googlesource.com/chromium/src/+/88c63227018cd614c0eab270dbc2... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:420001) as https://chromium.googlesource.com/chromium/src/+/88c63227018cd614c0eab270dbc2...
Message was sent while issue was closed.
On 2017/03/14 at 01:23:24, zhaobin wrote: > Shall we change media_router.mojom to use optional field instead of union? So we can keep mojo consistent with c++ objects and remove extra code in MediaSinkInternal. No, I think a union is the right way to represent the data being exchanged in the mojom. > https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... > File chrome/browser/media/router/discovery/media_sink_internal.cc (right): > > https://codereview.chromium.org/2675033002/diff/400001/chrome/browser/media/r... > chrome/browser/media/router/discovery/media_sink_internal.cc:39: if (this != &other) { > On 2017/03/13 17:47:24, mark a. foltz wrote: > > The conversion to union seems to require all this extra code to do copy and > > assignment. What is the benefit? > > dcheng@ suggests that .mojom should be consistent with c++ object (if we use union in media_router.mojom, we need to use union in c++ object). Maybe we can change extra data to optional instead of union in mojom? I agree that by default the .mojom and c++ should be consistent, unless that introduces unnecessary complexity as a side effect, as (IMO) is the case here. In MediaSinkInternal, the representation in C++ is completely hidden behind the C++ API, so the choice should be driven by the simplest implementation. I didn't see a material difference in the complexity of the type maps themselves going one way or the other. There's maybe a small benefit in object size for having one base::ManualConstructor vs. two base::Optional, probably offset by the additional code size for the Internal() functions in the .cc. It's not really worth going back and forth on this further, but if I see this pattern suggested again I plan to raise the issue on chromium-mojo. m. |
