Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
949293004:
Add media router common classes and unittests. (Closed)
|
Created:
5 years, 10 months ago by Kevin M Modified:
5 years, 9 months ago CC:
chromium-reviews, posciak+watch_chromium.org, media-router+watch_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd media router common classes and unittests.
Create makefiles and integrate MR with GN build.
Add to GYP build.
BUG=460660
R=scherkus@chromium.org,DaleCurtis@chromium.org,xhwang@chromium.org,mfoltz@chromium.org,sky@chromium.org
Committed: https://crrev.com/d2f3bea0fef8b99360075e2483461ec075ceb013
Cr-Commit-Position: refs/heads/master@{#320186}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Chunkier CL #
Total comments: 6
Patch Set 3 : Resync with latest changes #
Total comments: 61
Patch Set 4 : Addressed code review feedback #Patch Set 5 : More typedef=>using conversions. #
Total comments: 19
Patch Set 6 : Code review feedback. #
Total comments: 16
Patch Set 7 : Code review feedback #
Total comments: 52
Patch Set 8 : MediaSource is now a class. #Patch Set 9 : MediaSource struct is now a class #
Total comments: 21
Patch Set 10 : Code review feedback #Patch Set 11 : Code review feedback. #
Total comments: 6
Patch Set 12 : XH's LGTM feedback. #
Total comments: 10
Patch Set 13 : Resync with master #Patch Set 14 : GN target fix. #Patch Set 15 : Move Media Router sources from common gyp locations to their own files #Patch Set 16 : Fixed Chromium copyright statement. #
Total comments: 2
Patch Set 17 : Bring GN files up to date. #Messages
Total messages: 58 (16 generated)
lgtm, but it might make sense to start with a slightly larger chunk of code to give the reviewers a bit more context (maybe combining 1.1 and 1.2 in your document)?
not lgtm Would like to have a discussion around this. I feel like collating these in one file could be premature optimization because most code that deals with MR objects is going to include headers declaring those objects anyway. Also it's an extra hop to go find this file when you're looking for all declarations relevant to e.g. MediaSource. How many files include only media_router_defines.h without also including media_sink.h, media_source.h, media_route.h, ...? https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... File chrome/browser/media/router/media_router_defines.h (right): https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... chrome/browser/media/router/media_router_defines.h:16: typedef std::string MediaSinkId; I would like to see a comment explaining the syntax of each of these and giving an example value. https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... chrome/browser/media/router/media_router_defines.h:16: typedef std::string MediaSinkId; Replace typedefs with using-declarations. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... chrome/browser/media/router/media_router_defines.h:18: typedef std::string MediaSource; Mention that this is always a URI and I would be in favor of renaming to MediaSourceUri. https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... chrome/browser/media/router/media_router_defines.h:20: typedef base::Callback<void(const MediaRouteResponse&)> I would put this at the end as it's not aliasing a simple type like the other declarations. https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... chrome/browser/media/router/media_router_defines.h:21: MediaRouteResponseCallback; Please verify this is really used in multiple headers before including it in the common defines. https://codereview.chromium.org/949293004/diff/1/chrome/browser/media/router/... chrome/browser/media/router/media_router_defines.h:22: typedef int64 RouteRequestId; Add newlines consistently between entries (after comments are added).
I talked with imcheng@ about this. He used existing code in the Chromium codebase as a model and there isn't a strong reason to retain this structure, so I'll go ahead and push the types into the relevant header files.
kmarshall@chromium.org changed reviewers: - dalecurtis@chromium.org, scherkus@chromium.org, sky@chromium.org, xhwang@chromium.org
Removed the other reviewers from the R= list until I've restructured the code to mfoltz's satisfaction
OK, this CL now includes more base classes and should be fully integrated with the build and test systems for GYP and GN.
Some suggestions to whittle down this patch to the files we know we'll need. media_source.* doesn't look like the current one in the repo. Can you check that you've pulled all commits? I'll look again at remaining files tomorrow. https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... File chrome/browser/media/router/media_cast_mode.h (right): https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... chrome/browser/media/router/media_cast_mode.h:13: enum MediaCastMode { Per discussion with imcheng@ today this is really something that is managed by the UI, not the backend; I would prefer it live in the webui/folder (or a separate component). I would be inclined to leave it out of this patch until its eventual location is set. https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... File chrome/browser/media/router/media_controller.h (right): https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... chrome/browser/media/router/media_controller.h:16: class MediaStatus { This entire file is based on the assumption that there will be common controls shared among the MRPs. Since Cast is planning on going the custom route, so far we don't have any requirements for that, any hookup to the MRP or WebUI or even mocks of what it would look like :) I would suggest leaving media_controller.* out of this patch and deciding among ourselves if it is still part of the product plan. https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:5: #include "chrome/browser/media/router/media_source.h" This file doesn't look current.
https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... File chrome/browser/media/router/media_cast_mode.h (right): https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... chrome/browser/media/router/media_cast_mode.h:13: enum MediaCastMode { On 2015/02/27 05:48:10, mark a. foltz wrote: > Per discussion with imcheng@ today this is really something that is managed by > the UI, not the backend; I would prefer it live in the webui/folder (or a > separate component). > > I would be inclined to leave it out of this patch until its eventual location is > set. Done. https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... File chrome/browser/media/router/media_controller.h (right): https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... chrome/browser/media/router/media_controller.h:16: class MediaStatus { On 2015/02/27 05:48:10, mark a. foltz wrote: > This entire file is based on the assumption that there will be common controls > shared among the MRPs. Since Cast is planning on going the custom route, so > far we don't have any requirements for that, any hookup to the MRP or WebUI or > even mocks of what it would look like :) > > I would suggest leaving media_controller.* out of this patch and deciding among > ourselves if it is still part of the product plan. Done. https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/20001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:5: #include "chrome/browser/media/router/media_source.h" On 2015/02/27 05:48:10, mark a. foltz wrote: > This file doesn't look current. Fetched the latest changes and pulled them in and made sure all the other files are current too. Also updated the Chromium copyright. :)
https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:12: const int64 kInvalidRouteRequestId = -1; s/int64/RouteRequestId/ https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:24: state_(MEDIA_ROUTE_STATE_NEW) { description_ and state_ would seem to be mutable over the lifetime of a route. Do they need setters? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:31: return media_route_id_ == other.media_route_id_ && The declaration states that route_ids are unique among all instances, so comparison-by-id should be sufficient? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:19: typedef int64 RouteRequestId; using is now preferred for type aliasing. http://en.cppreference.com/w/cpp/language/type_alias https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:20: extern const int64 kInvalidRouteRequestId; s/int64/RouteRequestId/ https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:32: // TODO(mfoltz): Add errors as needed. Please touch base with Derek and see if there is an error reporting path from the service that needs this. If so define error values that make sense, otherwise I would remove it. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:42: // TODO(mfoltz): Make private and friend MRF Can we do this now? I feel we should do this to control allocation of media_route_ids. The MediaRouter implementation should really be the only thing that can create MediaRoute objects, and it could instantiate a factory object that is friended by this. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:43: // |media_route_id|: Uniquely identifies the route among active For this to be true the (to be implemented) factory will need to be a singleton and shared among the MediaRouter implementations (which are becoming profile-keyed). https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:49: // |is_local|: true if the route was created initiated from this Chrome "created instantiated" seems redundant. Can you clarify "this instance"? Is this per-browser process or per-profile? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:68: // The sink being routed to. The ID of the sink... https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:71: // The description of the media route activity. An example would be helpful. Will this need to be localized in any way? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:74: // Returns |true| if the route is initiated locally. I might elaborate a little: "if the route was created locally (versus discovered by a media route provider)." https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:87: // TODO(imcheng): Add icon URL. Still valid? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:90: // Response to a media route request to the Media Route Provider. "...from a Media Route Provider" https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:93: // Creates a MediaRouteResponse object that indicates success. Only Media Router implementations should be creating these IIUC. Should they be declared protected and virtual in MediaRouter? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:94: // |request_id|: ID of original request. s/request_id/route_request_id/g to be explicit about the nature of the request. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:102: const RouteRequestId& request_id, const std::string& error); Should we use a MediaRouteError instead of the error string? If we want to have any intelligent error handling from a client, we need an enum. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:124: scoped_ptr<MediaRoute> route_; It's odd that the response object owns the lifetime of the media route. That implies that the caller has to keep the response around as long as it wants to use the route, or explicitly take ownership, which doesn't seem to be supported. How should callers keep |route_| alive? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_id.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route_id.h:14: typedef std::string MediaRouteId; using https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route_id.h:14: typedef std::string MediaRouteId; Any downside to placing this into media_route.h (or are there headers that need only the id declared)? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.cc:35: return sink_id_ == other.sink_id_ && Is comparison by sink_id sufficient? Since this has mutable state we don't want multiple objects representing the same MediaSink. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.cc:41: return !(*this == other); Or more simply sink_id_ != other.sink_id_ https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:19: enum Status { It would be a good idea to document the state transitions for this object. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:25: REQUEST_PENDING Can a sink ever have a request pending while it is also active? This feels like it could be an independent boolean attribute (separate from active/idle status). https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:29: // |name|: Descriptive name of the MediaSink. Does this represent a localized string? If so, is std::string the right type for conveying it? (I haven't dealt with UI strings personally - miu@ would be able to tell you right away.) https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:31: // TODO(mfoltz): Remove? This is only used for tests. This is probably a good time as ever to do this. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:37: // MediaSourceManager::GetDefaultMediaSource is resolved. IIRC this will need to be retained until more Presentation API functionality is complete.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:39: class MediaRoute { You'll likely need EXPORT definitions if you plan to use these in unit tests.
https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:12: const int64 kInvalidRouteRequestId = -1; On 2015/03/02 21:40:08, mark a. foltz wrote: > s/int64/RouteRequestId/ Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:24: state_(MEDIA_ROUTE_STATE_NEW) { On 2015/03/02 21:40:08, mark a. foltz wrote: > description_ and state_ would seem to be mutable over the lifetime of a route. > > Do they need setters? The Media Route object is used to communicate the state of a MR at a given point of time, so they should be immutable. If the state of a MR changes, then the component extension will simply send a new MediaRoute object instead of mutating a preexisting object. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:31: return media_route_id_ == other.media_route_id_ && On 2015/03/02 21:40:08, mark a. foltz wrote: > The declaration states that route_ids are unique among all instances, so > comparison-by-id should be sufficient? Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:19: typedef int64 RouteRequestId; On 2015/03/02 21:40:09, mark a. foltz wrote: > using is now preferred for type aliasing. > > http://en.cppreference.com/w/cpp/language/type_alias Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:19: typedef int64 RouteRequestId; On 2015/03/02 21:40:09, mark a. foltz wrote: > using is now preferred for type aliasing. > > http://en.cppreference.com/w/cpp/language/type_alias Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:20: extern const int64 kInvalidRouteRequestId; On 2015/03/02 21:40:09, mark a. foltz wrote: > s/int64/RouteRequestId/ Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:32: // TODO(mfoltz): Add errors as needed. On 2015/03/02 21:40:09, mark a. foltz wrote: > Please touch base with Derek and see if there is an error reporting path from > the service that needs this. If so define error values that make sense, > otherwise I would remove it. Removed. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:39: class MediaRoute { On 2015/03/02 23:14:33, DaleCurtis wrote: > You'll likely need EXPORT definitions if you plan to use these in unit tests. I'm building with component=shared_library and haven't seen any errors yet. How do I tell if this is necessary or not? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:42: // TODO(mfoltz): Make private and friend MRF On 2015/03/02 21:40:09, mark a. foltz wrote: > Can we do this now? > > I feel we should do this to control allocation of media_route_ids. The > MediaRouter implementation should really be the only thing that can create > MediaRoute objects, and it could instantiate a factory object that is friended > by this. Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:43: // |media_route_id|: Uniquely identifies the route among active On 2015/03/02 21:40:09, mark a. foltz wrote: > For this to be true the (to be implemented) factory will need to be a singleton > and shared among the MediaRouter implementations (which are becoming > profile-keyed). Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:49: // |is_local|: true if the route was created initiated from this Chrome On 2015/03/02 21:40:09, mark a. foltz wrote: > "created instantiated" seems redundant. > > Can you clarify "this instance"? Is this per-browser process or per-profile? Per browser process. Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:68: // The sink being routed to. On 2015/03/02 21:40:09, mark a. foltz wrote: > The ID of the sink... Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:71: // The description of the media route activity. On 2015/03/02 21:40:08, mark a. foltz wrote: > An example would be helpful. > > Will this need to be localized in any way? Good question - do you think we should push UI locale down to the component extension, or should we take locale/message pairs here? I'm thinking the former might be preferable. We could change the Mojo call GetMediaRouterInstanceId() to GetMediaRouterInstanceInfo(), which would return locale in addition to the instance ID GUID. GetMediaRouterInstanceId() => (string instance_id, string locale); https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:74: // Returns |true| if the route is initiated locally. On 2015/03/02 21:40:09, mark a. foltz wrote: > I might elaborate a little: > > "if the route was created locally (versus discovered by a media route > provider)." Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:87: // TODO(imcheng): Add icon URL. On 2015/03/02 21:40:09, mark a. foltz wrote: > Still valid? Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:90: // Response to a media route request to the Media Route Provider. On 2015/03/02 21:40:09, mark a. foltz wrote: > "...from a Media Route Provider" Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:93: // Creates a MediaRouteResponse object that indicates success. On 2015/03/02 21:40:08, mark a. foltz wrote: > Only Media Router implementations should be creating these IIUC. Should they be > declared protected and virtual in MediaRouter? Done. Removed it from here. Will just ack the comments here and defer them to the CL that lands media_router.h. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:94: // |request_id|: ID of original request. On 2015/03/02 21:40:09, mark a. foltz wrote: > s/request_id/route_request_id/g to be explicit about the nature of the request. Acknowledged. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:102: const RouteRequestId& request_id, const std::string& error); On 2015/03/02 21:40:09, mark a. foltz wrote: > Should we use a MediaRouteError instead of the error string? If we want to have > any intelligent error handling from a client, we need an enum. Acknowledged. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:124: scoped_ptr<MediaRoute> route_; On 2015/03/02 21:40:09, mark a. foltz wrote: > It's odd that the response object owns the lifetime of the media route. That > implies that the caller has to keep the response around as long as it wants to > use the route, or explicitly take ownership, which doesn't seem to be supported. > How should callers keep |route_| alive? Acknowledged. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_id.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route_id.h:14: typedef std::string MediaRouteId; On 2015/03/02 21:40:09, mark a. foltz wrote: > Any downside to placing this into media_route.h (or are there headers that need > only the id declared)? A cycle is introduced if we fold media_route_id.h into media_route.h: media_route.h --depends--> media_controller.h --depends--> media_route_id.h https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.cc:35: return sink_id_ == other.sink_id_ && On 2015/03/02 21:40:09, mark a. foltz wrote: > Is comparison by sink_id sufficient? > > Since this has mutable state we don't want multiple objects representing the > same MediaSink. Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.cc:41: return !(*this == other); On 2015/03/02 21:40:09, mark a. foltz wrote: > Or more simply sink_id_ != other.sink_id_ Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:19: enum Status { On 2015/03/02 21:40:10, mark a. foltz wrote: > It would be a good idea to document the state transitions for this object. Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:25: REQUEST_PENDING On 2015/03/02 21:40:10, mark a. foltz wrote: > Can a sink ever have a request pending while it is also active? > > This feels like it could be an independent boolean attribute (separate from > active/idle status). I talked this over with imcheng and he's thinking that Status might be worth removing entirely, as it hasn't proved to be useful so far. Removed it. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:29: // |name|: Descriptive name of the MediaSink. On 2015/03/02 21:40:09, mark a. foltz wrote: > Does this represent a localized string? If so, is std::string the right type > for conveying it? (I haven't dealt with UI strings personally - miu@ would be > able to tell you right away.) The string data we get from Mojo is a UTF-8 encoded std::string, so the character encodings should remain intact if we pass it around as a std::string. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:31: // TODO(mfoltz): Remove? This is only used for tests. On 2015/03/02 21:40:10, mark a. foltz wrote: > This is probably a good time as ever to do this. Done. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:37: // MediaSourceManager::GetDefaultMediaSource is resolved. On 2015/03/02 21:40:10, mark a. foltz wrote: > IIRC this will need to be retained until more Presentation API functionality is > complete. Acknowledged.
https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:71: // The description of the media route activity. On 2015/03/03 21:53:18, Kevin M wrote: > On 2015/03/02 21:40:08, mark a. foltz wrote: > > An example would be helpful. > > > > Will this need to be localized in any way? > > Good question - do you think we should push UI locale down to the component > extension, or should we take locale/message pairs here? > > I'm thinking the former might be preferable. We could change the Mojo call > GetMediaRouterInstanceId() to GetMediaRouterInstanceInfo(), which would return > locale in addition to the instance ID GUID. > > GetMediaRouterInstanceId() => (string instance_id, string locale); Talked with haibinlu@ about this - the description is free text sent by the receiver, which is spawned with the user's locale and is responsible for returning locale-appropriate strings. So no, it shouldn't need localization.
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
drive-by review https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:35: return media_route_id_ != other.media_route_id_; I think we should define this as !(*this == other); . https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:49: const MediaSource& media_source() const { return media_source_; } It's not hard to misuse a getter that returns by reference to a member variable. What do you think if we just return by value all the time and make it consistent throughout the code? https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_factory.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_factory.cc:17: next_id_ = 0; this can be initialized using the member initializer list syntax. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_factory.h:32: MediaRoute* Create(const MediaSource& media_source, I think the fact that this returns a dynamically allocated MediaRoute pointer kind of strange. Because everywhere else in the media router code we pass these immutable objects by const ref, and we can copy MediaRoute objects by copy constructor / copy assignment anyways. So why not just return by value here. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.cc:24: return sink_id_ != other.sink_id_; !(*this == other);
https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:35: return media_route_id_ != other.media_route_id_; On 2015/03/03 22:53:09, imcheng1 wrote: > I think we should define this as !(*this == other); . Done. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:49: const MediaSource& media_source() const { return media_source_; } On 2015/03/03 22:53:09, imcheng1 wrote: > It's not hard to misuse a getter that returns by reference to a member variable. > What do you think if we just return by value all the time and make it consistent > throughout the code? I think returning field const references is widespread and generally accepted as safe... I did a pass to ensure that we're being consistent with our use of references for non-POD data. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_factory.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_factory.cc:17: next_id_ = 0; On 2015/03/03 22:53:09, imcheng1 wrote: > this can be initialized using the member initializer list syntax. Done. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_factory.h:32: MediaRoute* Create(const MediaSource& media_source, On 2015/03/03 22:53:10, imcheng1 wrote: > I think the fact that this returns a dynamically allocated MediaRoute pointer > kind of strange. Because everywhere else in the media router code we pass these > immutable objects by const ref, and we can copy MediaRoute objects by copy > constructor / copy assignment anyways. So why not just return by value here. Done. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.cc:24: return sink_id_ != other.sink_id_; On 2015/03/03 22:53:10, imcheng1 wrote: > !(*this == other); Done.
Getting close. Here's a bit of context for my comments about locale: http://stackoverflow.com/questions/26110405/polymer-rtl-text-based-on-an-attr... https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:24: state_(MEDIA_ROUTE_STATE_NEW) { On 2015/03/03 21:53:18, Kevin M wrote: > On 2015/03/02 21:40:08, mark a. foltz wrote: > > description_ and state_ would seem to be mutable over the lifetime of a route. > > > > Do they need setters? > > The Media Route object is used to communicate the state of a MR at a given point > of time, so they should be immutable. If the state of a MR changes, then the > component extension will simply send a new MediaRoute object instead of mutating > a preexisting object. (1) This semantic should be documented in the header - something along the lines of this object capturing the state of the route at a given point in time. (2) How does code holding references to existing routes get notified/updated when they change state? https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:29: // |name|: Descriptive name of the MediaSink. On 2015/03/03 21:53:20, Kevin M wrote: > On 2015/03/02 21:40:09, mark a. foltz wrote: > > Does this represent a localized string? If so, is std::string the right type > > for conveying it? (I haven't dealt with UI strings personally - miu@ would be > > able to tell you right away.) > > The string data we get from Mojo is a UTF-8 encoded std::string, so the > character encodings should remain intact if we pass it around as a std::string. We should touch base about rendering requirements and again see if we should pass locale information here. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:56: // TODO(kmarshall): Does this need to be localized? Per discussion, the browser is not responsible for localizing this, the extension is. However, without the locale included, we may not be able to render it correctly (e.g. LTR or RTL). I might update this TODO to say "Do we need to pass locale information?" https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:59: // Returns |true| if the route is created locally (versus diescovered discovered https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_factory.h:32: MediaRoute* Create(const MediaSource& media_source, On 2015/03/03 23:28:05, Kevin M wrote: > On 2015/03/03 22:53:10, imcheng1 wrote: > > I think the fact that this returns a dynamically allocated MediaRoute pointer > > kind of strange. Because everywhere else in the media router code we pass > these > > immutable objects by const ref, and we can copy MediaRoute objects by copy > > constructor / copy assignment anyways. So why not just return by value here. > > Done. Doesn't look that way from my diff. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_unittest.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_unittest.cc:23: ForCastAppMediaSource("DialApp"), "sinkId", "Description", false); Can we use ForPresentationUrl or ForTabId instead? This would make it easier to remove this method down the line. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:31: // TODO(mfoltz): Remove? This is only used for tests. Can this be done now?
https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_route.cc:24: state_(MEDIA_ROUTE_STATE_NEW) { On 2015/03/04 06:22:58, mark a. foltz wrote: > On 2015/03/03 21:53:18, Kevin M wrote: > > On 2015/03/02 21:40:08, mark a. foltz wrote: > > > description_ and state_ would seem to be mutable over the lifetime of a > route. > > > > > > Do they need setters? > > > > The Media Route object is used to communicate the state of a MR at a given > point > > of time, so they should be immutable. If the state of a MR changes, then the > > component extension will simply send a new MediaRoute object instead of > mutating > > a preexisting object. > > (1) This semantic should be documented in the header - something along the lines > of this object capturing the state of the route at a given point in time. > > (2) How does code holding references to existing routes get notified/updated > when they change state? 1. See the new comment for MediaRoute. 2. Also see the new comment. There is no code that holds references to routes; MediaRoute objects are short lived and are only used to carry route data between the MRPM and the WebUI. https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/40001/chrome/browser/media/rou... chrome/browser/media/router/media_sink.h:29: // |name|: Descriptive name of the MediaSink. On 2015/03/04 06:22:58, mark a. foltz wrote: > On 2015/03/03 21:53:20, Kevin M wrote: > > On 2015/03/02 21:40:09, mark a. foltz wrote: > > > Does this represent a localized string? If so, is std::string the right > type > > > for conveying it? (I haven't dealt with UI strings personally - miu@ would > be > > > able to tell you right away.) > > > > The string data we get from Mojo is a UTF-8 encoded std::string, so the > > character encodings should remain intact if we pass it around as a > std::string. > > We should touch base about rendering requirements and again see if we should > pass locale information here. Sure, I'll talk to you in person, maybe including apacible@? https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:56: // TODO(kmarshall): Does this need to be localized? On 2015/03/04 06:22:59, mark a. foltz wrote: > Per discussion, the browser is not responsible for localizing this, the > extension is. However, without the locale included, we may not be able to render > it correctly (e.g. LTR or RTL). > > I might update this TODO to say "Do we need to pass locale information?" I hadn't considered the bidi aspect. OK, TODO added. Thanks. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route.h:59: // Returns |true| if the route is created locally (versus diescovered On 2015/03/04 06:22:59, mark a. foltz wrote: > discovered Done. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_route_factory.h:32: MediaRoute* Create(const MediaSource& media_source, On 2015/03/04 06:22:59, mark a. foltz wrote: > On 2015/03/03 23:28:05, Kevin M wrote: > > On 2015/03/03 22:53:10, imcheng1 wrote: > > > I think the fact that this returns a dynamically allocated MediaRoute > pointer > > > kind of strange. Because everywhere else in the media router code we pass > > these > > > immutable objects by const ref, and we can copy MediaRoute objects by copy > > > constructor / copy assignment anyways. So why not just return by value here. > > > > Done. > > Doesn't look that way from my diff. I neglected to git cl upload. Sorry. https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/80001/chrome/browser/media/rou... chrome/browser/media/router/media_source.cc:31: // TODO(mfoltz): Remove? This is only used for tests. On 2015/03/04 06:22:59, mark a. foltz wrote: > Can this be done now? Done.
lgtm % a few remaining comments https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:39: // needs to be pulled from the Media Route Provider Manager, which is the The MRPM is a component in another process (the extension) so someone writing code here wouldn't be able to access it directly. Maybe it would be better to just say the Media Router here and leave that as an implementation detail? https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:59: // TODO(kmarshall): Do we need to pass locale information? pass locale information for bidi rendering? https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:79: // |is_local|: true if the route was created from this browser. Since these parameters map 1:1 to the public and documented getters above, it should be clear what they mean from context and the individual descriptions can be omitted. No harm in leaving it in and I don't feel strongly about it. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.cc (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.cc:13: const char kRoutePrefix[] = "route-"; Per our conversation yesterday, route-local- so we can distinguish routes created by this browser vs. activities running on other devices. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.cc:21: return output; Can this function be a one-liner? https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:21: // MediaRoute objects. This is a static singleton that uses a non-threadsafe way to maintain the IDs. It should become bound to a thread on first instantiation and check that only thread is used to access it with e.g. ThreadChecker. I don't see a strong need now to make it thread safe unless there is something I'm missing in the rest of the implementation. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:18: const char* kDialUrnPrefix = "urn:dial-multiscreen-org:dial:application:"; This can be removed. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:48: StartsWithASCII(source, kDialUrnPrefix, true) || As can this line.
kmarshall@chromium.org changed reviewers: + scherkus@chromium.org, sky@chromium.org, xhwang@chromium.org - imcheng@chromium.org
Thanks for the detailed review, Mark. Hello Andrew, Dale, and Xiaohan! Please take a look at this CL. This CL contains the foundation classes that are used by the MediaRouter, but not the MR itself. That will come in a followup CL. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:39: // needs to be pulled from the Media Route Provider Manager, which is the On 2015/03/05 16:58:41, mark a. foltz wrote: > The MRPM is a component in another process (the extension) so someone writing > code here wouldn't be able to access it directly. Maybe it would be better to > just say the Media Router here and leave that as an implementation detail? Done. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:59: // TODO(kmarshall): Do we need to pass locale information? On 2015/03/05 16:58:41, mark a. foltz wrote: > pass locale information for bidi rendering? Done. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:79: // |is_local|: true if the route was created from this browser. On 2015/03/05 16:58:41, mark a. foltz wrote: > Since these parameters map 1:1 to the public and documented getters above, it > should be clear what they mean from context and the individual descriptions can > be omitted. No harm in leaving it in and I don't feel strongly about it. I'll remove it so there's less to keep in sync. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.cc (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.cc:13: const char kRoutePrefix[] = "route-"; On 2015/03/05 16:58:41, mark a. foltz wrote: > Per our conversation yesterday, route-local- so we can distinguish routes > created by this browser vs. activities running on other devices. Done. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.cc:21: return output; On 2015/03/05 16:58:41, mark a. foltz wrote: > Can this function be a one-liner? Sure, if you're comfortable with an inline postincrement. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:21: // MediaRoute objects. On 2015/03/05 16:58:41, mark a. foltz wrote: > This is a static singleton that uses a non-threadsafe way to maintain the IDs. > It should become bound to a thread on first instantiation and check that only > thread is used to access it with e.g. ThreadChecker. > > I don't see a strong need now to make it thread safe unless there is something > I'm missing in the rest of the implementation. Done. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:18: const char* kDialUrnPrefix = "urn:dial-multiscreen-org:dial:application:"; On 2015/03/05 16:58:41, mark a. foltz wrote: > This can be removed. Done. https://codereview.chromium.org/949293004/diff/100001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:48: StartsWithASCII(source, kDialUrnPrefix, true) || On 2015/03/05 16:58:41, mark a. foltz wrote: > As can this line. Done.
A lot of nits.... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:22: extern const RouteRequestId kInvalidRouteRequestId; kInvalidRouteRequestId is not used in this CL. How is related to MediaRoute? Can we add it in a later CL where this will actually be used? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:40: class MediaRoute { It seems this class only carries data. How about using a struct instead of a class? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:66: bool operator!=(const MediaRoute& other) const; operator overloading is not recommended by the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Over... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:33: MediaRoute Create(const MediaSource& media_source, Will it work if you have a static Create() function in MediaRoute, and have a static member of next_id_? Then you don't need this factory class. But I think I am missing something... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_id.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_id.h:10: #include "base/callback.h" not needed? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_unittest.cc:21: TEST_F(MediaRouteTest, Equals) { You probably don't need the MediaRouteTest at all. Just use TEST here. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_unittest.cc:44: } You may also want to cover the case where is_local is true. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.cc:7: #include "base/logging.h" no needed? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:17: class MediaSink { ditto: should this be a struct? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:21: // |status|: Current status of the MediaSink. There's no |status| in the constructor. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:28: std::string StatusAsString() const; This is not defined? Remove or define it? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:31: bool operator!=(const MediaSink& other) const; ditto about operator overloading... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_unittest.cc:19: TEST_F(MediaSinkTest, Equals) { You probably don't need MediaSinkTest at all. Just use TEST here. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:16: const char* kTabMediaUrnPrefix = "urn:x-org.chromium.media:source:tab"; nit. We prefer const char kFoo[] to const char* kFoo In theory, the latter allows you to assign a new value to kFoo, which is not desired. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:18: const char* kCastUrnPrefix = "urn:x-com.google.cast:application:"; It's confusing that some prefix ends with ":" and some doesn't. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:20: } // namespace not needed. const variables are static by default. http://stackoverflow.com/questions/177437/const-static https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:10: #include "chrome/browser/media/router/media_route_id.h" not needed? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:20: MediaSource ForPresentationUrl(const std::string& presentation_url); nit: How about MediaSourceForTab or GetMediaSourceForTab? or you can probably make MediaSource a real struct/class, then have static methods returning these, e.g. class MediaSource { public: static MediaSrouce ForTab(int tab_id); ... private: std::string source_; } Currently it's not very consistent, e.g. you should have ForPresentationUrlMediaSource, but that's too long :( https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:23: ForCastAppMediaSource("DEADBEEF")); Does this has to be a hex string? What anout XYZ? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:29: EXPECT_TRUE(IsValidMediaSource(ForTabMediaSource(123))); How about urn:x-org.chromium.media:source:tab:foo? https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:32: EXPECT_TRUE(IsValidMediaSource(ForPresentationUrl("http://theoatmeal.com/"))); Also test https?
Thanks Xiaohan. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:22: extern const RouteRequestId kInvalidRouteRequestId; On 2015/03/05 22:28:49, xhwang wrote: > kInvalidRouteRequestId is not used in this CL. How is related to MediaRoute? Can > we add it in a later CL where this will actually be used? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:40: class MediaRoute { On 2015/03/05 22:28:49, xhwang wrote: > It seems this class only carries data. How about using a struct instead of a > class? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C... Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:66: bool operator!=(const MediaRoute& other) const; On 2015/03/05 22:28:49, xhwang wrote: > operator overloading is not recommended by the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Over... Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:33: MediaRoute Create(const MediaSource& media_source, On 2015/03/05 22:28:49, xhwang wrote: > Will it work if you have a static Create() function in MediaRoute, and have a > static member of next_id_? Then you don't need this factory class. But I think I > am missing something... The purpose of this class is more about enforcing unique IDs for MediaRoutes (see class comment) than just creating new MediaRoutes. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_id.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_id.h:10: #include "base/callback.h" On 2015/03/05 22:28:49, xhwang wrote: > not needed? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_unittest.cc:21: TEST_F(MediaRouteTest, Equals) { On 2015/03/05 22:28:50, xhwang wrote: > You probably don't need the MediaRouteTest at all. Just use TEST here. Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_unittest.cc:44: } On 2015/03/05 22:28:49, xhwang wrote: > You may also want to cover the case where is_local is true. Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.cc:7: #include "base/logging.h" On 2015/03/05 22:28:50, xhwang wrote: > no needed? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:17: class MediaSink { On 2015/03/05 22:28:50, xhwang wrote: > ditto: should this be a struct? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:21: // |status|: Current status of the MediaSink. On 2015/03/05 22:28:50, xhwang wrote: > There's no |status| in the constructor. Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:28: std::string StatusAsString() const; On 2015/03/05 22:28:50, xhwang wrote: > This is not defined? Remove or define it? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:31: bool operator!=(const MediaSink& other) const; On 2015/03/05 22:28:50, xhwang wrote: > ditto about operator overloading... Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_unittest.cc:19: TEST_F(MediaSinkTest, Equals) { On 2015/03/05 22:28:50, xhwang wrote: > You probably don't need MediaSinkTest at all. Just use TEST here. Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:16: const char* kTabMediaUrnPrefix = "urn:x-org.chromium.media:source:tab"; On 2015/03/05 22:28:50, xhwang wrote: > nit. We prefer > > const char kFoo[] > > to > > const char* kFoo > > In theory, the latter allows you to assign a new value to kFoo, which is not > desired. Good point, done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:18: const char* kCastUrnPrefix = "urn:x-com.google.cast:application:"; On 2015/03/05 22:28:50, xhwang wrote: > It's confusing that some prefix ends with ":" and some doesn't. It's totally media type dependent here. For tab, the media ID is the concatenated form "tabX" where X is the ID of a tab. For desktopMediaUrnPrefix, it's not meant to be prepended to anything, so that's a misnomer. I'll fix it. For CastURN, the cast app ID is used as-is and not concatenated with any other tokens. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:20: } // namespace On 2015/03/05 22:28:50, xhwang wrote: > not needed. const variables are static by default. > > http://stackoverflow.com/questions/177437/const-static I'm not sure about that. const variables can definitely be referenced from outside the module via use of externs. http://stackoverflow.com/questions/2190919/mixing-extern-and-const The SO article you shared applies to const statics, but these aren't static. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:10: #include "chrome/browser/media/router/media_route_id.h" On 2015/03/05 22:28:50, xhwang wrote: > not needed? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:20: MediaSource ForPresentationUrl(const std::string& presentation_url); On 2015/03/05 22:28:50, xhwang wrote: > nit: How about MediaSourceForTab or GetMediaSourceForTab? > > or you can probably make MediaSource a real struct/class, then have static > methods returning these, e.g. > > class MediaSource { > public: > static MediaSrouce ForTab(int tab_id); > ... > private: > std::string source_; > } > > Currently it's not very consistent, e.g. you should have > ForPresentationUrlMediaSource, but that's too long :( Good suggestion, this provides better organization. Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:23: ForCastAppMediaSource("DEADBEEF")); On 2015/03/05 22:28:50, xhwang wrote: > Does this has to be a hex string? What anout XYZ? Yes it does. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:29: EXPECT_TRUE(IsValidMediaSource(ForTabMediaSource(123))); On 2015/03/05 22:28:50, xhwang wrote: > How about urn:x-org.chromium.media:source:tab:foo? Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:32: EXPECT_TRUE(IsValidMediaSource(ForPresentationUrl("http://theoatmeal.com/"))); On 2015/03/05 22:28:50, xhwang wrote: > Also test https? Done.
imcheng/mfoltz, please note the changes to MediaSource.
More nits. Some comments are in PS7. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:33: MediaRoute Create(const MediaSource& media_source, On 2015/03/06 23:12:44, Kevin M wrote: > On 2015/03/05 22:28:49, xhwang wrote: > > Will it work if you have a static Create() function in MediaRoute, and have a > > static member of next_id_? Then you don't need this factory class. But I think > I > > am missing something... > > The purpose of this class is more about enforcing unique IDs for MediaRoutes > (see class comment) than just creating new MediaRoutes. The static Create() function and static member of next_id_ can achieve the same goal of enforcing unique IDs... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:18: const char* kCastUrnPrefix = "urn:x-com.google.cast:application:"; On 2015/03/06 23:12:45, Kevin M wrote: > On 2015/03/05 22:28:50, xhwang wrote: > > It's confusing that some prefix ends with ":" and some doesn't. > > It's totally media type dependent here. > > For tab, the media ID is the concatenated form "tabX" where X is the ID of a > tab. > For desktopMediaUrnPrefix, it's not meant to be prepended to anything, so that's > a misnomer. I'll fix it. > For CastURN, the cast app ID is used as-is and not concatenated with any other > tokens. sg, add a comment? It's easy for people to omit this. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:20: } // namespace On 2015/03/06 23:12:45, Kevin M wrote: > On 2015/03/05 22:28:50, xhwang wrote: > > not needed. const variables are static by default. > > > > http://stackoverflow.com/questions/177437/const-static > > I'm not sure about that. const variables can definitely be referenced from > outside the module via use of externs. > http://stackoverflow.com/questions/2190919/mixing-extern-and-const > > The SO article you shared applies to const statics, but these aren't static. I am talking about const char kFoo[] = "..." without extern and without the anonymous namespace. In that case kFoo is "static" by default which means it can only be referenced from this module. So the anonymous namespace doesn't do anything here. An example of a previous discussion: https://codereview.chromium.org/24649002/diff/16001/cc/trees/thread_proxy.cc#... https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:23: ForCastAppMediaSource("DEADBEEF")); On 2015/03/06 23:12:45, Kevin M wrote: > On 2015/03/05 22:28:50, xhwang wrote: > > Does this has to be a hex string? What anout XYZ? > > Yes it does. Add a test case for XYZ then? https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:38: struct MediaRoute { For struct, you should make the member variables public and drop the accessors. Now I just realized you want to make the class immutable. Also you have an Equals() function. With that, maybe a "class" is the right option. Sorry for going back and forth on this. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:67: FRIEND_TEST_ALL_PREFIXES(MediaRouteTest, Equals); It seems MediaRouteTest is not accessing any private stuff. Not needed? https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:28: // |type|: Type of media route. There's no |type| in the signature. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:34: const MediaSinkId& sink_id, OOC, can this be a MediaSink instead of a MediaSinkId? https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:39: FRIEND_TEST_ALL_PREFIXES(MediaRouteFactoryTest, Create); MediaRouteFactoryTest is not using any private methods/members. Not needed? https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:17: struct MediaSink { ditto about class vs struct. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:16: virtual ~MediaSource(); Is MediaSources a base class? Wondering whether we need "virtual". https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:27: bool Parse(const std::string& id); Hmm, the default ctor and this function provide another way to create a MediaSource. In which case should we prefer this to the four static For* methods above? Should the caller make sure the |id| is valid? Someone looking at this class may wonder whether he/she should use ForCastApp(), or MediaSource() + Parse()... https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:39: static bool IsValid(); not defined? https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_unittest.cc (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:36: MediaSource::ForPresentationUrl("http://theoatmeal.com/").id())); hmm, http://theoatmeal.com is actually a real site... not sure whether we should use a real one in test code...
https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:33: MediaRoute Create(const MediaSource& media_source, On 2015/03/09 17:37:30, xhwang wrote: > On 2015/03/06 23:12:44, Kevin M wrote: > > On 2015/03/05 22:28:49, xhwang wrote: > > > Will it work if you have a static Create() function in MediaRoute, and have > a > > > static member of next_id_? Then you don't need this factory class. But I > think > > I > > > am missing something... > > > > The purpose of this class is more about enforcing unique IDs for MediaRoutes > > (see class comment) than just creating new MediaRoutes. > > The static Create() function and static member of next_id_ can achieve the same > goal of enforcing unique IDs... OK, I've turned this class into a "Route ID manager" and made MediaRoute's ctor publicly visible. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:18: const char* kCastUrnPrefix = "urn:x-com.google.cast:application:"; On 2015/03/09 17:37:30, xhwang wrote: > On 2015/03/06 23:12:45, Kevin M wrote: > > On 2015/03/05 22:28:50, xhwang wrote: > > > It's confusing that some prefix ends with ":" and some doesn't. > > > > It's totally media type dependent here. > > > > For tab, the media ID is the concatenated form "tabX" where X is the ID of a > > tab. > > For desktopMediaUrnPrefix, it's not meant to be prepended to anything, so > that's > > a misnomer. I'll fix it. > > For CastURN, the cast app ID is used as-is and not concatenated with any other > > tokens. > > sg, add a comment? It's easy for people to omit this. Done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source.cc:20: } // namespace On 2015/03/09 17:37:30, xhwang wrote: > On 2015/03/06 23:12:45, Kevin M wrote: > > On 2015/03/05 22:28:50, xhwang wrote: > > > not needed. const variables are static by default. > > > > > > http://stackoverflow.com/questions/177437/const-static > > > > I'm not sure about that. const variables can definitely be referenced from > > outside the module via use of externs. > > http://stackoverflow.com/questions/2190919/mixing-extern-and-const > > > > The SO article you shared applies to const statics, but these aren't static. > > I am talking about > > const char kFoo[] = "..." > > without extern and without the anonymous namespace. In that case kFoo is > "static" by default which means it can only be referenced from this module. So > the anonymous namespace doesn't do anything here. > > An example of a previous discussion: > https://codereview.chromium.org/24649002/diff/16001/cc/trees/thread_proxy.cc#... OK, done. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_unittest.cc (right): https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:8: namespace media_router { Please take a look at media_sources.h. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:23: ForCastAppMediaSource("DEADBEEF")); On 2015/03/09 17:37:30, xhwang wrote: > On 2015/03/06 23:12:45, Kevin M wrote: > > On 2015/03/05 22:28:50, xhwang wrote: > > > Does this has to be a hex string? What anout XYZ? > > > > Yes it does. > > Add a test case for XYZ then? Protocol-specific ID syntax validation takes place within the JS module running inside the extension, which is outside the scope of this component. The MR here is meant to be agnostic to protocol level details. https://codereview.chromium.org/949293004/diff/120001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:29: EXPECT_TRUE(IsValidMediaSource(ForTabMediaSource(123))); On 2015/03/06 23:12:45, Kevin M wrote: > On 2015/03/05 22:28:50, xhwang wrote: > > How about urn:x-org.chromium.media:source:tab:foo? > > Done. Sorry, didn't mean to mark this as done. This is another example of URN validation that should be performed at the extension side. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:38: struct MediaRoute { On 2015/03/09 17:37:31, xhwang wrote: > For struct, you should make the member variables public and drop the accessors. > Now I just realized you want to make the class immutable. Also you have an > Equals() function. With that, maybe a "class" is the right option. Sorry for > going back and forth on this. Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:67: FRIEND_TEST_ALL_PREFIXES(MediaRouteTest, Equals); On 2015/03/09 17:37:31, xhwang wrote: > It seems MediaRouteTest is not accessing any private stuff. Not needed? Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_factory.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:28: // |type|: Type of media route. On 2015/03/09 17:37:31, xhwang wrote: > There's no |type| in the signature. Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:34: const MediaSinkId& sink_id, On 2015/03/09 17:37:31, xhwang wrote: > OOC, can this be a MediaSink instead of a MediaSinkId? Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_route_factory.h:39: FRIEND_TEST_ALL_PREFIXES(MediaRouteFactoryTest, Create); On 2015/03/09 17:37:31, xhwang wrote: > MediaRouteFactoryTest is not using any private methods/members. Not needed? Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_sink.h:17: struct MediaSink { On 2015/03/09 17:37:31, xhwang wrote: > ditto about class vs struct. Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:16: virtual ~MediaSource(); On 2015/03/09 17:37:31, xhwang wrote: > Is MediaSources a base class? Wondering whether we need "virtual". Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:16: virtual ~MediaSource(); On 2015/03/09 17:37:31, xhwang wrote: > Is MediaSources a base class? Wondering whether we need "virtual". Done. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:27: bool Parse(const std::string& id); On 2015/03/09 17:37:31, xhwang wrote: > Hmm, the default ctor and this function provide another way to create a > MediaSource. In which case should we prefer this to the four static For* methods > above? Should the caller make sure the |id| is valid? Someone looking at this > class may wonder whether he/she should use ForCastApp(), or MediaSource() + > Parse()... I agree that this is unclear. I've moved the creation methods into a standalone .cc/.h along with a comment that proposes that we move them out of c/b/m/r entirely, since all but one are meant to be invoked by the UI only. The class is much more basic now. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:39: static bool IsValid(); On 2015/03/09 17:37:31, xhwang wrote: > not defined? Acknowledged. https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_unittest.cc (right): https://codereview.chromium.org/949293004/diff/160001/chrome/browser/media/ro... chrome/browser/media/router/media_source_unittest.cc:36: MediaSource::ForPresentationUrl("http://theoatmeal.com/").id())); On 2015/03/09 17:37:31, xhwang wrote: > hmm, http://theoatmeal.com is actually a real site... not sure whether we should > use a real one in test code... Good point, but test code is removed anyway.
lgtm % nits. Thanks for your patience! https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... File chrome/browser/media/router/media_sources.h (right): https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... chrome/browser/media/router/media_sources.h:6: #define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SOURCES_H_ media_sources.h isn't very informative... maybe media_source_helper or something you like? https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... File chrome/browser/media/router/route_id_manager.h (right): https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... chrome/browser/media/router/route_id_manager.h:21: class RouteIDManager : public base::NonThreadSafe { Is NonThreadSafe needed? You are not checking CalledOnValidThread()... https://codereview.chromium.org/949293004/diff/200001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/949293004/diff/200001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2329: 'browser/media/router/route_id_manager_unittest.cc', sort in alphabetic order
https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... File chrome/browser/media/router/media_sources.h (right): https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... chrome/browser/media/router/media_sources.h:6: #define CHROME_BROWSER_MEDIA_ROUTER_MEDIA_SOURCES_H_ On 2015/03/10 04:27:07, xhwang wrote: > media_sources.h isn't very informative... maybe media_source_helper or something > you like? Done. https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... File chrome/browser/media/router/route_id_manager.h (right): https://codereview.chromium.org/949293004/diff/200001/chrome/browser/media/ro... chrome/browser/media/router/route_id_manager.h:21: class RouteIDManager : public base::NonThreadSafe { On 2015/03/10 04:27:07, xhwang wrote: > Is NonThreadSafe needed? You are not checking CalledOnValidThread()... An omission that crept in when I rewrote this from the previous factory impl. Fixed, thanks. https://codereview.chromium.org/949293004/diff/200001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/949293004/diff/200001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:2329: 'browser/media/router/route_id_manager_unittest.cc', On 2015/03/10 04:27:07, xhwang wrote: > sort in alphabetic order Done.
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/route_id_manager.h (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/route_id_manager.h:22: class RouteIDManager : public base::NonThreadSafe { Wouldn't it be better to use a base::ThreadChecker instead? (Composition over inheritence)
Mostly comments on comments ;) https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_route.cc:28: bool MediaRoute::Equals(const MediaRoute& other) const { Why Equals() instead of implementing operator==? https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_route.h:72: // by a media route provider.) Nit: provider). https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/media_route_unittest.cc (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_route_unittest.cc:12: // Tests the == operator to ensure that only route ID equality is being checked. Right now, you're testing Equals(). https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/media_source.h (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_source.h:12: class MediaSource { Can we add some documentation describing the meaning and syntax of a MediaSource id? In principle the MR doesn't care, but we do have code that validates them to prevent garbage-in-garbage-out semantics. https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_helper.h (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.h:17: // (excluding Presentation API) is their only caller? The MR base code can be This is a sentence fragment. https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.h:18: // Also, consider generating an is_mirroring state boolean at the time of URN If we are going to classify the sources on creation time it would be better to use an enum. https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.h:22: // type agnostic fashion vs. having to format and parse URNs and track which The MR shouldn't really behave differently for tab/desktop capture sources vs. other types of sources. I think the TODO is to find out why we have IsMirroringMediaSource and remove it. https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.h:34: // Checks that |source| is a parseable URN and is of a known type. s/URN/URI/ https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... File chrome/browser/media/router/route_id_manager.h (right): https://codereview.chromium.org/949293004/diff/220001/chrome/browser/media/ro... chrome/browser/media/router/route_id_manager.h:22: class RouteIDManager : public base::NonThreadSafe { On 2015/03/10 17:43:33, imcheng1 wrote: > Wouldn't it be better to use a base::ThreadChecker instead? (Composition over > inheritence) +1, see the comments here. https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... But do keep the comments about thread safety so the reader knows that it is not thread-safe.
sky or jhawkins, could you review for gyp & GN changes? Thanks.
kmarshall@chromium.org changed reviewers: + jhawkins@chromium.org
LGTM
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/949293004/#ps240001 (title: "Resync with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949293004/240001
kmarshall@chromium.org changed reviewers: + jam@chromium.org - jhawkins@chromium.org
+jam@chromium.org, would you mind reviewing the change to /src/BUILD.gn? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949293004/240001
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, mfoltz@chromium.org, jam@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/949293004/#ps260001 (title: "GN target fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949293004/260001
The CQ bit was unchecked by kmarshall@chromium.org
sky@, please take a look - I have moved the Media Router sources and unittests into their own gyp files. We will be landing a lot of code in the chrome/browser/media/router directory over the coming weeks and this change will hopefully reduce your review workload. :)
Fix the following and LGTM https://codereview.chromium.org/949293004/diff/300001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/949293004/diff/300001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:20: "route_id_manager.cc", I don't see media_source_helper.* listed here.
https://codereview.chromium.org/949293004/diff/300001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/949293004/diff/300001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:20: "route_id_manager.cc", On 2015/03/11 19:53:56, sky wrote: > I don't see media_source_helper.* listed here. Done.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, jam@chromium.org, xhwang@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/949293004/#ps320001 (title: "Bring GN files up to date.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949293004/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/d2f3bea0fef8b99360075e2483461ec075ceb013 Cr-Commit-Position: refs/heads/master@{#320186} |
