Issue 1020743003: [Media Router] Design MediaRouter interface with stub implementation.

imcheng

Patchset #1 (id:1) has been deleted

5 years, 9 months ago (2015-03-26 18:01:30 UTC) #1

imcheng

imcheng@chromium.org changed reviewers: + imcheng@chromium.org, kmarshall@chromium.org, mfoltz@chromium.org, xhwang@chromium.org

5 years, 9 months ago (2015-03-26 18:46:54 UTC) #3

imcheng

imcheng@chromium.org changed reviewers: - xhwang@chromium.org

5 years, 9 months ago (2015-03-26 18:53:20 UTC) #5

imcheng

-Xiaohan for now until Kevin and mark has looked through it first.

5 years, 9 months ago (2015-03-26 18:53:20 UTC) #6

Kevin M

https://codereview.chromium.org/1020743003/diff/60001/chrome/browser/media/router/extension_media_route_provider_manager_host.cc File chrome/browser/media/router/extension_media_route_provider_manager_host.cc (right): https://codereview.chromium.org/1020743003/diff/60001/chrome/browser/media/router/extension_media_route_provider_manager_host.cc#newcode32 chrome/browser/media/router/extension_media_route_provider_manager_host.cc:32: NOTIMPLEMENTED(); Add landing TODO to help out our other ...

5 years, 9 months ago (2015-03-26 20:50:15 UTC) #7

imcheng

Addressed Kevin's comments. Also I have removed the EMRPMH and factory files for now to ...

5 years, 9 months ago (2015-03-26 23:33:13 UTC) #8

mark a. foltz

Most of my comments are lots of documentation edits/suggestions. Recommend upstreaming the unit test for ...

5 years, 9 months ago (2015-03-27 21:21:59 UTC) #9

imcheng

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/router/media_route.cc File chrome/browser/media/router/media_route.cc (right): https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/router/media_route.cc#newcode25 chrome/browser/media/router/media_route.cc:25: MediaRoute::MediaRoute() On 2015/03/27 21:21:57, mark a. foltz wrote: > ...

5 years, 8 months ago (2015-03-30 22:49:10 UTC) #10

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route.cc (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.cc:25: MediaRoute::MediaRoute()
On 2015/03/27 21:21:57, mark a. foltz wrote:
> What is the use case for this?  There's no setters for the fields, so is it
just
> used as a Null Object?
> 
> http://en.wikipedia.org/wiki/Null_Object_pattern
> 
> It might be worth adding a brief comment to explain.

Kevin suggested that instead of using a nullptr to represent a situation where a
route is absent (such as the error case of MediaRouteResponse). I added comments
in the header file.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_provider_manager_host.h:20: // An
interface acting as the Chrome endpoint for communication between
On 2015/03/27 21:21:57, mark a. foltz wrote:
> Acts as the endpoint for communication...

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_provider_manager_host.h:21: // Media
Router in Chrome and Media Route Provider Manager (MRPM).
On 2015/03/27 21:21:57, mark a. foltz wrote:
> the media router and media route providers (via the Media Route Provider
> Manager).

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_provider_manager_host.h:26: // Interface
for handling responses from the MRPM.
On 2015/03/27 21:21:57, mark a. foltz wrote:
> Delegate called by the MediaRouteProviderManagerHost when a response is
received
> from the MRPM.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_provider_manager_host.h:31: // Called
from MediaRouteProviderManagerHost when results from a media
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Eliminate "from MediaRouteProviderMangerHost" here and below, since you can
> explain this once above.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_provider_manager_host.h:43: virtual void
OnMessage(const MediaRouteId& route_id,
On 2015/03/27 21:21:57, mark a. foltz wrote:
> Is there an API for this in media_router.h?

MediaRouterImpl provides an override.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_provider_manager_host.h:72: // The
following functions are called from Media Router to MRPM.
On 2015/03/27 21:21:57, mark a. foltz wrote:
> The following API allows the media router to send commands to the MRPM.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route_response.h (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_response.h:15: // Response to a media
route request to the Media Route Provider.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Response from the media route provider to a media route request.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_response.h:21: static
scoped_ptr<MediaRouteResponse> Success(
On 2015/03/27 21:21:58, mark a. foltz wrote:
> CreateFromRoute?

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_response.h:24: // Creates a
MediaRouteResponse object that indicates failure.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> A newline here would help IMO.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_response.h:28: const std::string&
error);
On 2015/03/27 21:21:58, mark a. foltz wrote:
> CreateFromError

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_route_response.h:36: const std::string&
error() const { return error_; }
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Might mention that this returns an empty string on success.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:27: // as well as handling route
requests/responses.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Update docstring to include PostMessage behavior.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:27: // as well as handling route
requests/responses.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Threading assumptions should be documented here in relation to the observer
and
> callback objects passed in.

I will do that in media_router_impl.h.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:33: // |callback| is invoked with a
response indicating success or failure.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Explain use of return value (i.e. it can be used to later unregister).

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:39: // Unregisters a pending media
route request, e.g. when the MR UI is closed.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> e.g., when the user has canceled presentation by closing the media router
> dialog.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:39: // Unregisters a pending media
route request, e.g. when the MR UI is closed.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Should this have a return value?  As long as it guarantees that the request is
> discarded (if it exists) probably not, but wanted to check.

It will always discard the request, so no return value is needed.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:40: virtual void
UnregisterMediaRouteResponseCallback(
On 2015/03/27 21:21:58, mark a. foltz wrote:
> - A better name might be CancelRouteRequest?
> - Mention the request_id must be a value returned from StartRouteRequest
> - What is the fate of the callback registered with StartRouteRequest -  is it
> invoked with an error or never run?
> - If an error is returned, should the caller here pass a reason for the
> cancellation (i.e., USER_CANCELLED)?

This function does NOT cancel a route request, only the callback. I updated the
comments to clarify that.
This function is typically used when a registered callback is about to go out of
scope.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:43: // Closes a media route specified
by |route_id|.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> s/a/the/

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:46: // Registers |observer| with
MediaRouter so that it will receive updates on
On 2015/03/27 21:21:58, mark a. foltz wrote:
> I might write this as:
> 
> Registers |observer| with this MediaRouter.  |observer| specifies a media
source
> and will receive updates with media sinks that are compatible with that
source. 
> The initial update may happen synchronously.
> <newline>
> 

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:51: // before the observer is
destroyed.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Is Unregister() always called immediately before Observer destruction?  Maybe
> the dtor for Observer should be responsible for unregistering it from the
router
> (by the router providing a callback to run in the dtor).
> 
> If not, is the idea that the observer can be unregistered and re-registered if
> needed?  Are observers ever shared between instances of the router?  If it's
1:1
> at a minimum I would keep a 'registered' bit in the observer and DCHECK on it
in
> the observer dtor.
> 
It is typically the case that destruction follows from unregistration, but
lifetime and (un)registration are isolated. The idea is as you mentioned. The
owner of the observers may have better context so it's responible for issuing
the MediaRouter commands. 

I suppose we can combine lifetime with registration. One way is to inject
MediaRouter (or 2 callbacks) to the observer's ctor.
RegisterObserver/UnregisterObserver will be called in ctor/dtor respectively.
One drawback will be that you can no longer create an observer without having a
MediaRouter first. Will that approach be more preferrable?

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:57: // Unregisters |observer| from
MediaRouter.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> What are the side effects? I assume that |observer| will stop receiving
further
> updates?

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:62: virtual void PostMessage(const
MediaRouteId& route_id,
On 2015/03/27 21:21:58, mark a. foltz wrote:
> This (at a minimum) needs a TODO to add overrides for the additional data
types
> that are now available from the spec: Blob, ArrayBuffer, ArrayBufferView.  See
> the pending changes for postMessage() in Blink.

Does the API and downstream need to differentiate the between the different data
types? If so, would it be sufficient to use a std::string to hold the data and
provide an extra enum arg to specify the data type?

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:64: const std::string&
extra_info_json) = 0;
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Is extra_info_json being used?

No it's not being used. I can remove it for now to simplify things.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:68: // be called before |observer| is
destroyed.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Similar comments to the pattern above for Register/Unregister.

See other comment.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:69: virtual void
AddMediaRoutesObserver(MediaRoutesObserver* observer) = 0;
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Would prefer similar terminology to the sinks case, maybe:
> 
> [Un]RegisterMediaSinksObserver()
> [Un]RegisterMediaRoutesObserver()
> 

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_impl.cc (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_impl.cc:30: 
On 2015/03/27 21:21:58, mark a. foltz wrote:
> // TODO(imcheng): Implement NOTIMPLEMENTED methods

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_impl.h (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_impl.h:38: // with the Media Route
Provider Manager (MRPM) in the component extension.
On 2015/03/27 21:21:59, mark a. foltz wrote:
> I wouldn't mention the component extension here, since that is specific to the
> type of MRPMH passed in by the caller.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_impl.h:39: // In addition, responses
from MRPM are delegated to this class which then
On 2015/03/27 21:21:58, mark a. foltz wrote:
> s/MRPM/MediaRouteProviderManagerHost/

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_impl.h:41: // Instances of this class
are created through MediaRouterImplFactory.
On 2015/03/27 21:21:58, mark a. foltz wrote:
> s/through/by/
> 
> Mention this is a profile-keyed service.
> 
> Discuss thread safety.

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_impl.h:52: void
SetMRPMHost(MediaRouteProviderManagerHost* mrpm_host);
On 2015/03/27 21:21:58, mark a. foltz wrote:
> Maybe this should just be Initialize()?

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_impl.h:92: // Does not own this object.
On 2015/03/27 21:21:59, mark a. foltz wrote:
> I think you meant to say "Not owned by this object."

Done.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:15: struct SinksQueryResult {
On 2015/03/27 21:21:59, mark a. foltz wrote:
> Should this be a using declaration for std::vector<MediaSink> instead of a
> struct?
> Or do we want the flexibility to add additional data over time?

The latter.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:22: // Interface for
observing when the collection of sinks compatible with
On 2015/03/27 21:21:59, mark a. foltz wrote:
> Not an interface.

Ok, let's call it base class and provide a no-op implementation for
OnSinksReceived.

https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_sinks_observer.h:35: virtual void
OnSinksReceived(const SinksQueryResult& result) = 0;
On 2015/03/27 21:21:59, mark a. foltz wrote:
> The API is a little limiting in that there must be one observer object per
> source.  If we wanted a true observer interface it might look like:
> 
> virtual void OnSinksReceived(const MediaSource& source, const
SinksQueryResult&
> result)

Agreed. WebContentsObserver follow a similar model, vs. the older
NotificationRegistrar model. We can change it from single-target to the API you
described (with the observer implementation doing the filtering instead of the
MR) if it turns out to be more convenient.

imcheng

Regarding the dependency on chrome/browser target, I have decided to simply drop the use of ...

5 years, 8 months ago (2015-03-31 21:51:19 UTC) #11

mark a. foltz

lgtm % a few minor copy editing suggestions https://codereview.chromium.org/1020743003/diff/200001/chrome/browser/media/router/media_route_provider_manager_host.h File chrome/browser/media/router/media_route_provider_manager_host.h (right): https://codereview.chromium.org/1020743003/diff/200001/chrome/browser/media/router/media_route_provider_manager_host.h#newcode39 chrome/browser/media/router/media_route_provider_manager_host.h:39: // ...

5 years, 8 months ago (2015-03-31 22:10:25 UTC) #12

mark a. foltz

On 2015/03/30 22:49:10, imcheng1 wrote: > https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/router/media_router.h#newcode51 > chrome/browser/media/router/media_router.h:51: // before the observer is > ...

5 years, 8 months ago (2015-03-31 22:16:36 UTC) #13

On 2015/03/30 22:49:10, imcheng1 wrote:
>
https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router.h:51: // before the observer is
> destroyed.
> On 2015/03/27 21:21:58, mark a. foltz wrote:
> > Is Unregister() always called immediately before Observer destruction? 
Maybe
> > the dtor for Observer should be responsible for unregistering it from the
> router
> > (by the router providing a callback to run in the dtor).
> > 
> > If not, is the idea that the observer can be unregistered and re-registered
if
> > needed?  Are observers ever shared between instances of the router?  If it's
> 1:1
> > at a minimum I would keep a 'registered' bit in the observer and DCHECK on
it
> in
> > the observer dtor.
> > 
> It is typically the case that destruction follows from unregistration, but
> lifetime and (un)registration are isolated. The idea is as you mentioned. The
> owner of the observers may have better context so it's responible for issuing
> the MediaRouter commands. 
> 
> I suppose we can combine lifetime with registration. One way is to inject
> MediaRouter (or 2 callbacks) to the observer's ctor.
> RegisterObserver/UnregisterObserver will be called in ctor/dtor respectively.
> One drawback will be that you can no longer create an observer without having
a
> MediaRouter first. Will that approach be more preferrable?

Probably not.  Otherwise we create a coupling between the observer and the
MediaRouter that doesn't need to be there.

I would like to see a way to enforce no destruction of registered observers. 
Would the 'registered' bit I suggested be a good solution?

>
https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router.h:62: virtual void PostMessage(const
> MediaRouteId& route_id,
> On 2015/03/27 21:21:58, mark a. foltz wrote:
> > This (at a minimum) needs a TODO to add overrides for the additional data
> types
> > that are now available from the spec: Blob, ArrayBuffer, ArrayBufferView. 
See
> > the pending changes for postMessage() in Blink.
> 
> Does the API and downstream need to differentiate the between the different
data
> types? If so, would it be sufficient to use a std::string to hold the data and
> provide an extra enum arg to specify the data type?

The APIs downstream and upstream will need to know which type the data should be
exposed as.  A blob will definitely need a different declaration than
std::string.

The postMessage API is still under discussion in Blink/WG so you can add a TODO
for now.

>
https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/media_sinks_observer.h:35: virtual void
> OnSinksReceived(const SinksQueryResult& result) = 0;
> On 2015/03/27 21:21:59, mark a. foltz wrote:
> > The API is a little limiting in that there must be one observer object per
> > source.  If we wanted a true observer interface it might look like:
> > 
> > virtual void OnSinksReceived(const MediaSource& source, const
> SinksQueryResult&
> > result)
> 
> Agreed. WebContentsObserver follow a similar model, vs. the older
> NotificationRegistrar model. We can change it from single-target to the API
you
> described (with the observer implementation doing the filtering instead of the
> MR) if it turns out to be more convenient.

SGTM

imcheng

On 2015/03/31 22:16:36, mark a. foltz wrote: > On 2015/03/30 22:49:10, imcheng1 wrote: > > ...

5 years, 8 months ago (2015-04-01 00:45:50 UTC) #14

On 2015/03/31 22:16:36, mark a. foltz wrote:
> On 2015/03/30 22:49:10, imcheng1 wrote:
> >
>
https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router.h:51: // before the observer is
> > destroyed.
> > On 2015/03/27 21:21:58, mark a. foltz wrote:
> > > Is Unregister() always called immediately before Observer destruction? 
> Maybe
> > > the dtor for Observer should be responsible for unregistering it from the
> > router
> > > (by the router providing a callback to run in the dtor).
> > > 
> > > If not, is the idea that the observer can be unregistered and
re-registered
> if
> > > needed?  Are observers ever shared between instances of the router?  If
it's
> > 1:1
> > > at a minimum I would keep a 'registered' bit in the observer and DCHECK on
> it
> > in
> > > the observer dtor.
> > > 
> > It is typically the case that destruction follows from unregistration, but
> > lifetime and (un)registration are isolated. The idea is as you mentioned.
The
> > owner of the observers may have better context so it's responible for
issuing
> > the MediaRouter commands. 
> > 
> > I suppose we can combine lifetime with registration. One way is to inject
> > MediaRouter (or 2 callbacks) to the observer's ctor.
> > RegisterObserver/UnregisterObserver will be called in ctor/dtor
respectively.
> > One drawback will be that you can no longer create an observer without
having
> a
> > MediaRouter first. Will that approach be more preferrable?
> 
> Probably not.  Otherwise we create a coupling between the observer and the
> MediaRouter that doesn't need to be there.
> 
> I would like to see a way to enforce no destruction of registered observers. 
> Would the 'registered' bit I suggested be a good solution?
> 

Chatted offline. We will go with: have MR set/unset the bit on
Register/Unregister. To encapsulate it properly, the setters should be private
and we would have to make MediaRouterImpl a friend class of the observers
classes.

Also renamed the (Un)RegisterObserver API to (Un)RegisterMediaSinksObserver.

> >
>
https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router.h:62: virtual void
PostMessage(const
> > MediaRouteId& route_id,
> > On 2015/03/27 21:21:58, mark a. foltz wrote:
> > > This (at a minimum) needs a TODO to add overrides for the additional data
> > types
> > > that are now available from the spec: Blob, ArrayBuffer, ArrayBufferView. 
> See
> > > the pending changes for postMessage() in Blink.
> > 
> > Does the API and downstream need to differentiate the between the different
> data
> > types? If so, would it be sufficient to use a std::string to hold the data
and
> > provide an extra enum arg to specify the data type?
> 
> The APIs downstream and upstream will need to know which type the data should
be
> exposed as.  A blob will definitely need a different declaration than
> std::string.
> 
> The postMessage API is still under discussion in Blink/WG so you can add a
TODO
> for now.
> 

Done. 

> >
>
https://codereview.chromium.org/1020743003/diff/80001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_sinks_observer.h:35: virtual void
> > OnSinksReceived(const SinksQueryResult& result) = 0;
> > On 2015/03/27 21:21:59, mark a. foltz wrote:
> > > The API is a little limiting in that there must be one observer object per
> > > source.  If we wanted a true observer interface it might look like:
> > > 
> > > virtual void OnSinksReceived(const MediaSource& source, const
> > SinksQueryResult&
> > > result)
> > 
> > Agreed. WebContentsObserver follow a similar model, vs. the older
> > NotificationRegistrar model. We can change it from single-target to the API
> you
> > described (with the observer implementation doing the filtering instead of
the
> > MR) if it turns out to be more convenient.
> 
> SGTM

imcheng

Thanks for the review. I will add Xiaohan back now. https://codereview.chromium.org/1020743003/diff/200001/chrome/browser/media/router/media_route_provider_manager_host.h File chrome/browser/media/router/media_route_provider_manager_host.h (right): https://codereview.chromium.org/1020743003/diff/200001/chrome/browser/media/router/media_route_provider_manager_host.h#newcode39 ...

5 years, 8 months ago (2015-04-01 00:55:17 UTC) #15

imcheng

imcheng@chromium.org changed reviewers: + xhwang@chromium.org

5 years, 8 months ago (2015-04-01 00:56:57 UTC) #16

imcheng

Xiaohan, this patch is ready for your review now. Thanks.

5 years, 8 months ago (2015-04-01 00:56:58 UTC) #17

Kevin Marshall

marshallk@google.com changed reviewers: + marshallk@google.com

5 years, 8 months ago (2015-04-01 17:00:18 UTC) #18

Kevin Marshall

lgtm https://codereview.chromium.org/1020743003/diff/220001/chrome/browser/media/router/media_routes_observer.h File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1020743003/diff/220001/chrome/browser/media/router/media_routes_observer.h#newcode37 chrome/browser/media/router/media_routes_observer.h:37: // This field will be set by MediaRouterImpl, ...

5 years, 8 months ago (2015-04-01 17:00:19 UTC) #19

imcheng

https://codereview.chromium.org/1020743003/diff/220001/chrome/browser/media/router/media_routes_observer.h File chrome/browser/media/router/media_routes_observer.h (right): https://codereview.chromium.org/1020743003/diff/220001/chrome/browser/media/router/media_routes_observer.h#newcode37 chrome/browser/media/router/media_routes_observer.h:37: // This field will be set by MediaRouterImpl, and ...

5 years, 8 months ago (2015-04-01 18:10:09 UTC) #20

xhwang

I didn't review all details. Mostly style/design-pattern comments/nits. Here are some high level comments: 1. ...

5 years, 8 months ago (2015-04-02 17:13:03 UTC) #21

imcheng

Thanks Xiaohan! Please see inline. On 2015/04/02 17:13:03, xhwang wrote: > I didn't review all ...

5 years, 8 months ago (2015-04-02 23:05:11 UTC) #22

imcheng

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/router/media_route.h File chrome/browser/media/router/media_route.h (right): https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/router/media_route.h#newcode52 chrome/browser/media/router/media_route.h:52: // than nullptr in situations where route is absent. ...

5 years, 8 months ago (2015-04-02 23:05:24 UTC) #23

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route.h:52: // than nullptr in situations
where route is absent.
On 2015/04/02 17:13:02, xhwang wrote:
> What's the use case for this? Personally I feel nullptr is more clear and more
> efficient.

Reverted back to nullptr. Since a MediaRoute contains a few other objects, it
feels a bit wasteful to contruct an empty instance.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:28: class
Delegate {
On 2015/04/02 17:13:02, xhwang wrote:
> Please see comment below. For response of a particular call, usually I like to
> use callbacks. For calls initiated by MRPM, we can use a Delegate or permanent
> callbacks. I don't know your system well enough to suggest which is better,
but
> that's something worth considering.

Thanks for the suggestion. Please refer to comments below.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:35: // |routes|:
Currently active routes on |sinks|.
On 2015/04/02 17:13:02, xhwang wrote:
> I don't see |routes|.

Removed.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:48: // |route|:
Description of the established route.
On 2015/04/02 17:13:02, xhwang wrote:
> Can |route| be Empty()? In that case, is "a route has been established" still
> accurate?

Since the "emtpy MediaRoute" has been reverted, it can no longer be
empty/invalid.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:70: // The
following API allows the media router to send commands to the MRPM.
On 2015/04/02 17:13:02, xhwang wrote:
> s/API allows/APIs allow/

Done.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:76: virtual void
AddMediaSinksQuery(const MediaSource& source) = 0;
On 2015/04/02 17:13:02, xhwang wrote:
> Can we replace OnSinksReceived() with a callback here? Then in the callback
you
> don't need to pass |source| as you are doing in OnSinksReceived().

We considered using callbacks here. But since AddMediaSinksQuery and
OnSinksReceived aren't 1:1 (OnSinksReceived will be called multiple times as
updates come in), I feel OnSinksReceived() is more explicit in expressing the
relationship.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:84: virtual void
RequestRoute(const RouteRequestId& request_id,
On 2015/04/02 17:13:02, xhwang wrote:
> Will the response be either OnRouteResponseReceived() or
OnRouteResponseError()?
> If so, update the comment to make it clear.
> 
> Can you have multiple pending RequestRoute() calls?
> 
> Is it viable to use a response callback instead of the Delegate() call?

Updated comment. I think that this is a place where it might be fine to use a
callback, but since this call is async, it does require the implementation to
keep track of the callbacks. Perhaps we could revisit this we upstream the
implementation?

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:98: //
|Delegate::OnActiveSinksAndRoutesReceived|.
On 2015/04/02 17:13:02, xhwang wrote:
> This doesn't exist...

Updated comment.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:102: virtual
void StopMediaRoutesQuery() = 0;
On 2015/04/02 17:13:02, xhwang wrote:
> I am confused by Query vs Update...

Changed the comment here to match AddMediaSinksQuery. Both types of queries are
continuous, i.e. once Start is called, results can be received multiple times if
there are updates, until Stop is called.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_response.cc (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_response.cc:25: const RouteRequestId&
request_id, const MediaRoute& route)
On 2015/04/02 17:13:02, xhwang wrote:
> What if |route| is Empty()? Is that response valid or not? IMHO having a null
> route is more clear (see comment above)...

Changed back to nullptr

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_response.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_route_response.h:30: const std::string&
error);
On 2015/04/02 17:13:02, xhwang wrote:
> There two functions are not used anywhere in this CL... I wonder why you need
> scoped_ptr here. Can you just pass the const-ref over layers?

I can upstream these factory functions in a later patch if you prefer. These are
scoped_ptrs because we have made MediaRouteResponses not
copy-constructable/assignable (we've added back scoped_ptr<MediaRoute> as one of
the members). But once they are constructed, we do pass them to function calls
using const-ref.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:23: base::Callback<void(const
MediaRouteResponse&)>;
On 2015/04/02 17:13:02, xhwang wrote:
> Add include for base::Callback.

Done.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:36: // Otherwise a valid
RouteRequestId will be returned, which can be used with
On 2015/04/02 17:13:02, xhwang wrote:
> nit: What's a definition of a valid RouteRequestId?
> 
> id != kInvalidRouteRequestId
> 
> or 
> 
> id > kInvalidRouteRequestId
> 
> ?

id != kInvalidRouteRequestId. Added comment.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:48: // registered via
|StartRouteRequest|.
On 2015/04/02 17:13:02, xhwang wrote:
> This is a bit odd (from interface's perspective; haven't looked at the impl
> yet):
> 
> 1, if the MediaRouter impl actually CAN cancel the route request, should it do
> so?
> 
> 2, The callback may be posted already and waiting in the message loop. In that
> case, you have no way to cancel the callback. Are you saying the caller may
just
> ignore the callback?

1. It currently cannot cancel the route request, and there's no use case that
requires it.
2. The implementation does not post the callbacks to a message loop. Instead it
keeps the callbacks in a map and matched (by request id) and invoked when a
response comes back. Thus the callback can easily be cancelled by removing the
callback from the map.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:50: virtual void
UnregisterMediaRouteResponseCallback(
On 2015/04/02 17:13:02, xhwang wrote:
> How about just CancelRouteRequest()? Unregistering the callback is really just
> an implementation detail.

The call doesn't cancel the route request.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:62: // Returns true if registration
succeeded or the |observer| already exists.
On 2015/04/02 17:13:02, xhwang wrote:
> Should "|observer| already exists" ever happen? What's the legit use case for
> that?

It should not happen because each observer should only be registered at most
once (w/o unregistering of course). Would you prefer it if the function enforced
this and CHECK'ed instead?

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_impl.cc (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_impl.cc:42: observer->registered_ =
false;
On 2015/04/02 17:13:02, xhwang wrote:
> This doesn't look good. Why can't we return a bool so the caller of 
> UnregisterMediaSinksObserver() can do this?

The point of this is to make sure the caller is calling this function. If
UnregisterMediaSinksObserver is never called, there would be a dangling pointer
in MediaRouterImpl when the observer is destroyed.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_impl.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_impl.h:95:
MediaRouteProviderManagerHost* mrpm_host_;
On 2015/04/02 17:13:02, xhwang wrote:
> Most people have no idea what "mrpm" is, use full name?
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami...
> 
> BTW, a provider-manager-host sounds complicated ;)

renamed to provider_manager_host_.

Yes, the class name is a bit wordy... It's the browser-side host for the Media
Route Provider Manager (MRPM), which is a manager of Media Route Providers (MRP)

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_routes_observer.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:15: struct RoutesQueryResult
{
On 2015/04/02 17:13:03, xhwang wrote:
> Can we just pass two parameters in OnRoutesUpdated()? An observer is usually
> passive. It waits for updates/notifications. So "query" doesn't really fit in
> this file...

Done.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:20: std::vector<MediaSink>
sinks;
On 2015/04/02 17:13:03, xhwang wrote:
> A MediaRoute has one MediaSource and one MediaSink. Is that correct? In that
> case, what's the relationship between routes[i].media_sink() and sinks[j]?

Good point. Since we now have MediaSink in MediaRoute, we no longer need a
separate list of sinks.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:20: };
On 2015/04/02 17:13:03, xhwang wrote:
> Do you plan to add more values to the result? Can we just return
> std::vector<MediaSink> as the result?

Right now we don't plan to add any more. I am not sure on the likelihood that we
will add more in the future. So it should be fine to just return the vector.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:40: friend class
MediaRouterImpl;
On 2015/04/02 17:13:03, xhwang wrote:
> MediaSinksObserver seems to be an interface, but we are exposing a lot of
> details: source() which is public and registered_ through friend. Does
> MediaRouter care about these?

Yes. source() by media router in determining the sink query to send to media
route provider manager. registered_ is a safeguard that we don't have dangling
observer pointers.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_source.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:32: const MediaSource& source);
On 2015/04/02 17:13:03, xhwang wrote:
> Operator overloading is not recommended:
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove...
> 
> Can you do something like ToString()?

Done.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:35: MediaSourceId id_;
On 2015/04/02 17:13:03, xhwang wrote:
> Do we plan to add more members to MeidaSource? Can we just
> 
> using MeidaSource = std::string?
> 
> Then you don't need Equals/Empty/operator<</etc

It is possible that we will add more fields to MediaSource.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:44: }
On 2015/04/02 17:13:03, xhwang wrote:
> How will this be used?

It will be used in maps where MediaSource is used as keys. (e.g.
MediaSinksObservers by MediaSource)

xhwang

Sorry I missed your update. Here are some more comments/discussions. Some comments are in the ...

5 years, 8 months ago (2015-04-06 20:51:32 UTC) #26

imcheng

Thanks! PTAL https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/router/media_router.h#newcode36 chrome/browser/media/router/media_router.h:36: // Otherwise a valid RouteRequestId will be ...

5 years, 8 months ago (2015-04-06 21:54:21 UTC) #27

Thanks! PTAL

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:36: // Otherwise a valid
RouteRequestId will be returned, which can be used with
On 2015/04/06 20:51:30, xhwang wrote:
> On 2015/04/02 23:05:24, imcheng1 wrote:
> > On 2015/04/02 17:13:02, xhwang wrote:
> > > nit: What's a definition of a valid RouteRequestId?
> > > 
> > > id != kInvalidRouteRequestId
> > > 
> > > or 
> > > 
> > > id > kInvalidRouteRequestId
> > > 
> > > ?
> > 
> > id != kInvalidRouteRequestId. Added comment.
> 
> hmm, so the id can be negative?

Our implementation actually allocates non-negative route request IDs, so let's
say that it must be id > kInvalidRouteRequestId here.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:62: // Returns true if registration
succeeded or the |observer| already exists.
On 2015/04/06 20:51:29, xhwang wrote:
> On 2015/04/02 23:05:24, imcheng1 wrote:
> > On 2015/04/02 17:13:02, xhwang wrote:
> > > Should "|observer| already exists" ever happen? What's the legit use case
> for
> > > that?
> > 
> > It should not happen because each observer should only be registered at most
> > once (w/o unregistering of course). Would you prefer it if the function
> enforced
> > this and CHECK'ed instead?
> 
> Yes. Please make the interface simple and clean. Comment like this make it
hard
> to understand what's the expected behavior. Please update this comment, and
> DCHECK() in code.

Ok, I updated the comment to say it is invalid to register the same observer
twice.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_impl.cc (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_router_impl.cc:42: observer->registered_ =
false;
On 2015/04/06 20:51:30, xhwang wrote:
> On 2015/04/02 23:05:24, imcheng1 wrote:
> > On 2015/04/02 17:13:02, xhwang wrote:
> > > This doesn't look good. Why can't we return a bool so the caller of 
> > > UnregisterMediaSinksObserver() can do this?
> > 
> > The point of this is to make sure the caller is calling this function. If
> > UnregisterMediaSinksObserver is never called, there would be a dangling
> pointer
> > in MediaRouterImpl when the observer is destroyed.
> 
> Having one class checking the internal state of another class seems bad.
> 
> Can a weak pointer solve the problem you are trying solve?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

No, we don't want to use a weak pointer here. Adding the weak ptr property to
the observers' base classes forces all implementations to have the weak ptr
property as well. Also using weak pointer here only shadows the problem we're
trying to detect and will require extra mechanism to clean up those WeakPtrs.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:40: friend class
MediaRouterImpl;
On 2015/04/06 20:51:30, xhwang wrote:
> On 2015/04/02 23:05:24, imcheng1 wrote:
> > On 2015/04/02 17:13:03, xhwang wrote:
> > > MediaSinksObserver seems to be an interface, but we are exposing a lot of
> > > details: source() which is public and registered_ through friend. Does
> > > MediaRouter care about these?
> > 
> > Yes. source() by media router in determining the sink query to send to media
> > route provider manager. registered_ is a safeguard that we don't have
dangling
> > observer pointers.
> 
> How do you plan implement "MediaRouter send updates with media sinks that are
> compatible with that source"? Will you check call "source()" on each observer
> and compare it with the "source" of the update? In that case, can you pass
> source() to MediaRouter at registration time, so that MediaRouter can find the
> observers more quickly, and you don't have to expose source() here?
> 

MediaRouter will keep a map from source to a set of observers for that source.
When updates come in, the MediaRouter impl will look up the same for the set of
observers to notify.

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
File chrome/browser/media/router/media_source.h (right):

https://codereview.chromium.org/1020743003/diff/240001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:44: }
On 2015/04/06 20:51:30, xhwang wrote:
> On 2015/04/02 23:05:24, imcheng1 wrote:
> > On 2015/04/02 17:13:03, xhwang wrote:
> > > How will this be used?
> > 
> > It will be used in maps where MediaSource is used as keys. (e.g.
> > MediaSinksObservers by MediaSource)
> 
> Can you simply use MediaSource::id() as the key to this map?

Perhaps we can do that, but we might add more comparable fields to MediaSource
in the future, so prefer to keep this for now.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:100: //
|Delegate::OnRoutesUpdated|.
On 2015/04/06 20:51:31, xhwang wrote:
> nit: You are not using "Delegate::" in other comments.

Added Delegate:: to OnRouteResponse* comments above.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_response.h (right):

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_route_response.h:52: const std::string error_;
On 2015/04/06 20:51:31, xhwang wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
File chrome/browser/media/router/media_sink.cc (right):

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_sink.cc:13: MediaSink::MediaSink() :
MediaSink("", "") {}
On 2015/04/06 20:51:31, xhwang wrote:
> You don't need the MediaSink("", "") part...

Done.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_sink.cc:19: }
On 2015/04/06 20:51:31, xhwang wrote:
> Can we have multiple sinks with the same ID, but different names?

No, it is not possible.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
File chrome/browser/media/router/media_sink.h (right):

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_sink.h:26: MediaSink();
On 2015/04/06 20:51:31, xhwang wrote:
> What's the use case of an empty MediaSink?

Removed for now.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_sink.h:38: std::string name_;
On 2015/04/06 20:51:31, xhwang wrote:
> Here and for all other classes, consider DISALLOW_COPY_AND_ASSIGN wherever
> applicable.

These need to be copyable because they are put into stl containers.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_sink.h:38: std::string name_;
On 2015/04/06 20:51:31, xhwang wrote:
> Why drop the "const", is MediaSink() mutable now?

From the API perspective MediaSink is still immutable. However I had to remove
the const qualifiers in order to support copy-assignment, which appears to be
used by STL implementation.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_sink.h:38: std::string name_;
On 2015/04/06 20:51:31, xhwang wrote:
> These are both strings. What's the difference?

The ID is internal to the system and is what the code uses to identify a sink.
The name is a description of the sink and is intended to be user-facing.

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
File chrome/browser/media/router/media_source.h (right):

https://codereview.chromium.org/1020743003/diff/300001/chrome/browser/media/r...
chrome/browser/media/router/media_source.h:42: return lhs.id() < rhs.id();
On 2015/04/06 20:51:31, xhwang wrote:
> Comparing strings isn't efficient. Consider using hash tables:
> base/containers/hash_tables.h

Replaced this class with a struct Hash.

imcheng

Hey Xiaohan, per our discussion about the RegisterObserver usage, I experimented with the change as ...

5 years, 8 months ago (2015-04-07 22:01:04 UTC) #29

imcheng

Updated CL using private MediaRouter interface for RegisterObserver APIs and adding the base observer classes ...

5 years, 8 months ago (2015-04-08 00:17:01 UTC) #30

pamelamaronato95

pamelamaronato95@gmail.com changed reviewers: + pamelamaronato95@gmail.com

5 years, 8 months ago (2015-04-08 17:45:19 UTC) #31

xhwang

xhwang@chromium.org changed reviewers: - pamelamaronato95@gmail.com

5 years, 8 months ago (2015-04-08 17:46:23 UTC) #33

xhwang

Thanks! I added more comments about the consistency between MediaRouteProviderManagerHost and MediaRouter. Also, next time ...

5 years, 8 months ago (2015-04-08 17:46:25 UTC) #34

Thanks!

I added more comments about the consistency between
MediaRouteProviderManagerHost and MediaRouter.

Also, next time can you split big changes into smaller CLs? It's hard to review
CLs of this size with all new code.

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:44: const MediaRouteResponseCallback&
callback) = 0;
Instead of a customized callback class, can you just use

base::Callback<void(int, scopted_ptr<MediaRoute>, const std::string&)> ?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route.h:81: MediaRouteState state_;
MediaRoute is non-trivial: it has 6 members. I would prefer to make this
immutable (as you have in your comment above at l.34), keep these fields const
and make the class DISALLOW_COPY_AND_ASSIGN.

For using MediaRoute in stl containers, you can try using ScopedVector instead
of std::vector.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:22: class
MediaRouteProviderManagerHost {
We don't have MediaRouteProvider and MediaRouteProviderManager yet. I assume
they will come in a later CL?

Could you please add more comments/TODO about what MediaRouteProvider and
MediaRouteProviderManager will do, and what's the relation between them and
MediaRouteProviderManagerHost?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:28: class
Delegate {
It seems not all methods of this class are responses. For example, OnMessage()
is initiated from the sink. Please make the comments of each method clear about
whether it's a response or not. If it is, which method is it responding to? Is
it a one-to-one mapping between the call and the response, or you could have
multiple responses for one call?

Either way, is using callbacks an option? You can have permanent callbacks which
could be called multiple times (e.g. for all following updates).

Also, this MRPMH interface looks very similar to MediaRouter interface. But
there are a lot of differences between them. For example: StartRouteRequest vs
RequestRoute. Also, we use a Delegate here. But we are using callbacks and
observers in MediaRouter. Is there any particular reason for these differences?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:55: const
std::string& error_text) = 0;
OnRouteResponseReceived() or OnRouteResponseError() will be called only once as
a result of RequestRoute(). Is that correct?

Can you combine these into one response? something like:

virtual void OnRouteResponseReceived(const RouteRequestId& request_id,
                                     scoped_ptr<MediaRoute> route,
                                     const std::string& error_text) = 0;

Also, see my comment above about the possibility of using callbacks instead of
delegate methods.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:57: // Called
when the list of currently MediaRoutes and their MediaSinks
nit: s/currently/current

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:62: const
std::vector<MediaSink>& sinks) = 0;
Ditto. |routes| contains sinks. What's the relationship between
routes[i].media_sink() and sinks[j]?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:87: const
MediaSinkId& sink_id) = 0;
If you use a callback for the response, you shouldn't need a |request_id|...

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:99: //
|Delegate::OnRoutesUpdated|.
Please see my comments above on callbacks.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:41: virtual RouteRequestId
StartRouteRequest(
(See comment above.) 

Does this correspond to RequestRoute() on the MediaRouteProviderManagerHost
interface? If so, can we make the naming more consistent?

In MediaRouteProviderManagerHost you use a delegate, but here you use a
callback. Can we be consistent?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:53: const RouteRequestId& request_id)
= 0;
Since we don't actually cancel the request, what's the value of unregistering
the callback? Are we trying to avoid access violation because maybe the caller
of StartRouteRequest() has been destructed?

Is this what you'd expect in implementation? After this call, the request will
still finish, but then we found the callback has already been unregistered, so
we'll drop the result on the floor and do nothing.

If this is the case, usually we don't need to unregister the callback
explicitly. You can use a weak pointer to bind the callback. When the caller of
StartRouteRequest() is destroyed, all callbacks will do nothing. This solves the
lifetime issue and unregistering callbacks automatically.

let me know if I am missing something though.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:66: friend class MediaRoutesObserver;
Does MediaSinkObserver and MediaRoutesObserver correspond to OnSinksReceived()
and OnRoutesUpdated() respectively?

ditto for Delegate vs Observer pattern.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_sinks_observer.cc (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.cc:17: DVLOG(1) <<
"RegisterMediaSinksObserver failed: " << source.ToString();
Probably LOG(ERROR).

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:26:
MediaSinksObserver(MediaRouter* router, const MediaSource& source);
This is much cleaner. Thanks!

imcheng

Thanks! https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/router/media_router.h#newcode44 chrome/browser/media/router/media_router.h:44: const MediaRouteResponseCallback& callback) = 0; On 2015/04/08 17:46:24, ...

5 years, 8 months ago (2015-04-08 19:21:31 UTC) #35

Thanks!

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:44: const MediaRouteResponseCallback&
callback) = 0;
On 2015/04/08 17:46:24, xhwang wrote:
> Instead of a customized callback class, can you just use
> 
> base::Callback<void(int, scopted_ptr<MediaRoute>, const std::string&)> ?

IMO it is cleaner to encapsulate the 3 fields (some of which are optional) with
some helper logic to interpret the results.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route.h:81: MediaRouteState state_;
On 2015/04/08 17:46:24, xhwang wrote:
> MediaRoute is non-trivial: it has 6 members. I would prefer to make this
> immutable (as you have in your comment above at l.34), keep these fields const
> and make the class DISALLOW_COPY_AND_ASSIGN.
> 
> For using MediaRoute in stl containers, you can try using ScopedVector instead
> of std::vector.

MediaRoute is still a data class as it stands today; is there a reason why we
want to DISALLOW_COPY_AND_ASSIGN, other than that this object is not so small?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:22: class
MediaRouteProviderManagerHost {
On 2015/04/08 17:46:25, xhwang wrote:
> We don't have MediaRouteProvider and MediaRouteProviderManager yet. I assume
> they will come in a later CL?
> 
> Could you please add more comments/TODO about what MediaRouteProvider and
> MediaRouteProviderManager will do, and what's the relation between them and
> MediaRouteProviderManagerHost?

Our implementation of MediaRouteProvider and MediaRouteProviderManager are
actually extension code not in Chromium repository. Added more comments to
clarify the relationship between MR, MRPMH, and MRPM/MRP.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:28: class
Delegate {
On 2015/04/08 17:46:24, xhwang wrote:
> It seems not all methods of this class are responses. For example, OnMessage()
> is initiated from the sink. Please make the comments of each method clear
about
> whether it's a response or not. If it is, which method is it responding to? Is
> it a one-to-one mapping between the call and the response, or you could have
> multiple responses for one call?
> 

I changed the class level comment here. But, since we already described the
relationship in the MediaRouteProviderManagerHost APIs below, I think it's fine
to not include the comments here as we would be duplicating comments.

> Either way, is using callbacks an option? You can have permanent callbacks
which
> could be called multiple times (e.g. for all following updates).
> 

I think this is a matter of preference. To me, a Delegate/Observer pattern makes
more sense than callbacks if it can be invoked multiple times. I also prefer to
have one interface/pattern rather than a mix of observer/callbacks so it's more
uniform and easier to understand.

> Also, this MRPMH interface looks very similar to MediaRouter interface. But
> there are a lot of differences between them. For example: StartRouteRequest vs
> RequestRoute. Also, we use a Delegate here. But we are using callbacks and
> observers in MediaRouter. Is there any particular reason for these
differences?

Re: StartRouteRequest vs RequestRoute. This is one where it's better to have
consistent naming, so I've changed it to RequestRoute in both places. The others
are named differently for valid reasons.

Re: Delegate vs. cbs/observers in MR. A MediaRouter instance corresponds 1:1 to
a MediaRouteProviderManagerHost instance and throughout its lifetime it may
receive many calls forwarded from MediaRouteProviderManagerHost. A Delegate
interface is used because of the preference over callbacks for this situation,
as mentioned above. In MR, Observers are used for receiving continuous updates
for queries and callbacks are used for one-time responses. As for naming of
Delegate vs Observer, I find Delegate more appropriate when the two components
are 1:1, whereas Observer is used for 1:n.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:55: const
std::string& error_text) = 0;
On 2015/04/08 17:46:24, xhwang wrote:
> OnRouteResponseReceived() or OnRouteResponseError() will be called only once
as
> a result of RequestRoute(). Is that correct?
> 
> Can you combine these into one response? something like:
> 
> virtual void OnRouteResponseReceived(const RouteRequestId& request_id,
>                                      scoped_ptr<MediaRoute> route,
>                                      const std::string& error_text) = 0;
> 
> Also, see my comment above about the possibility of using callbacks instead of
> delegate methods.

Done.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:57: // Called
when the list of currently MediaRoutes and their MediaSinks
On 2015/04/08 17:46:24, xhwang wrote:
> nit: s/currently/current

Done.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:62: const
std::vector<MediaSink>& sinks) = 0;
On 2015/04/08 17:46:24, xhwang wrote:
> Ditto. |routes| contains sinks. What's the relationship between
> routes[i].media_sink() and sinks[j]?

Removed |sinks|.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:87: const
MediaSinkId& sink_id) = 0;
On 2015/04/08 17:46:24, xhwang wrote:
> If you use a callback for the response, you shouldn't need a |request_id|...

See uniformity comment above.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:99: //
|Delegate::OnRoutesUpdated|.
On 2015/04/08 17:46:24, xhwang wrote:
> Please see my comments above on callbacks.

Acknowledged.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:41: virtual RouteRequestId
StartRouteRequest(
On 2015/04/08 17:46:25, xhwang wrote:
> (See comment above.) 
> 
> Does this correspond to RequestRoute() on the MediaRouteProviderManagerHost
> interface? If so, can we make the naming more consistent?
> 
> In MediaRouteProviderManagerHost you use a delegate, but here you use a
> callback. Can we be consistent?

Renamed to RequestRoute(). For delegate vs. cb, please see comment above.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:53: const RouteRequestId& request_id)
= 0;
On 2015/04/08 17:46:25, xhwang wrote:
> Since we don't actually cancel the request, what's the value of unregistering
> the callback? Are we trying to avoid access violation because maybe the caller
> of StartRouteRequest() has been destructed?
> 
> Is this what you'd expect in implementation? After this call, the request will
> still finish, but then we found the callback has already been unregistered, so
> we'll drop the result on the floor and do nothing.
> 
> If this is the case, usually we don't need to unregister the callback
> explicitly. You can use a weak pointer to bind the callback. When the caller
of
> StartRouteRequest() is destroyed, all callbacks will do nothing. This solves
the
> lifetime issue and unregistering callbacks automatically.
> 
> let me know if I am missing something though.

What you described is correct. However removing this API also means it will
force all users of RequestRoutes to support WeakPtrs. Is that an ok thing to do?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:66: friend class MediaRoutesObserver;
On 2015/04/08 17:46:25, xhwang wrote:
> Does MediaSinkObserver and MediaRoutesObserver correspond to OnSinksReceived()
> and OnRoutesUpdated() respectively?
> 
> ditto for Delegate vs Observer pattern.

Yes.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_sinks_observer.cc (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.cc:17: DVLOG(1) <<
"RegisterMediaSinksObserver failed: " << source.ToString();
On 2015/04/08 17:46:25, xhwang wrote:
> Probably LOG(ERROR).
> 

Done here and in MediaRoutesObserver.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_sinks_observer.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:26:
MediaSinksObserver(MediaRouter* router, const MediaSource& source);
On 2015/04/08 17:46:25, xhwang wrote:
> This is much cleaner. Thanks!

Acknowledged.

xhwang

I just found my comments were split into multiple PSs. Sorry about that. https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/router/media_router.h File ...

5 years, 8 months ago (2015-04-08 20:32:52 UTC) #36

I just found my comments were split into multiple PSs. Sorry about that.

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:44: const MediaRouteResponseCallback&
callback) = 0;
On 2015/04/08 19:21:30, imcheng1 wrote:
> On 2015/04/08 17:46:24, xhwang wrote:
> > Instead of a customized callback class, can you just use
> > 
> > base::Callback<void(int, scopted_ptr<MediaRoute>, const std::string&)> ?
> 
> IMO it is cleaner to encapsulate the 3 fields (some of which are optional)
with
> some helper logic to interpret the results.

hmm, you are actually using 3 fields directly in
MediaRouteProviderManagerHost::Delegate::OnRouteResponseReceived().

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route.h:81: MediaRouteState state_;
On 2015/04/08 19:21:30, imcheng1 wrote:
> On 2015/04/08 17:46:24, xhwang wrote:
> > MediaRoute is non-trivial: it has 6 members. I would prefer to make this
> > immutable (as you have in your comment above at l.34), keep these fields
const
> > and make the class DISALLOW_COPY_AND_ASSIGN.
> > 
> > For using MediaRoute in stl containers, you can try using ScopedVector
instead
> > of std::vector.
> 
> MediaRoute is still a data class as it stands today; is there a reason why we
> want to DISALLOW_COPY_AND_ASSIGN, other than that this object is not so small?

Isn't "this class is not so small" a strong reason to use
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:28: class
Delegate {
On 2015/04/08 19:21:31, imcheng1 wrote:
> On 2015/04/08 17:46:24, xhwang wrote:
> > It seems not all methods of this class are responses. For example,
OnMessage()
> > is initiated from the sink. Please make the comments of each method clear
> about
> > whether it's a response or not. If it is, which method is it responding to?
Is
> > it a one-to-one mapping between the call and the response, or you could have
> > multiple responses for one call?
> > 
> 
> I changed the class level comment here. But, since we already described the
> relationship in the MediaRouteProviderManagerHost APIs below, I think it's
fine
> to not include the comments here as we would be duplicating comments.

Ack. Thanks!
> 
> > Either way, is using callbacks an option? You can have permanent callbacks
> which
> > could be called multiple times (e.g. for all following updates).
> > 
> 
> I think this is a matter of preference. To me, a Delegate/Observer pattern
makes
> more sense than callbacks if it can be invoked multiple times. I also prefer
to
> have one interface/pattern rather than a mix of observer/callbacks so it's
more
> uniform and easier to understand.

Hmm, you are actually having a mix of observer/callbacks in MediaRouter
interface.

> > Also, this MRPMH interface looks very similar to MediaRouter interface. But
> > there are a lot of differences between them. For example: StartRouteRequest
vs
> > RequestRoute. Also, we use a Delegate here. But we are using callbacks and
> > observers in MediaRouter. Is there any particular reason for these
> differences?
> 
> Re: StartRouteRequest vs RequestRoute. This is one where it's better to have
> consistent naming, so I've changed it to RequestRoute in both places. The
others
> are named differently for valid reasons.
> 
> Re: Delegate vs. cbs/observers in MR. A MediaRouter instance corresponds 1:1
to
> a MediaRouteProviderManagerHost instance and throughout its lifetime it may
> receive many calls forwarded from MediaRouteProviderManagerHost. A Delegate
> interface is used because of the preference over callbacks for this situation,
> as mentioned above. In MR, Observers are used for receiving continuous updates
> for queries and callbacks are used for one-time responses. As for naming of
> Delegate vs Observer, I find Delegate more appropriate when the two components
> are 1:1, whereas Observer is used for 1:n.

I see your point. But I still wonder why we make different choices on
MediaRouteProviderManagerHost and MediaRouter interfaces. Maybe you can help on
understand this better.

If you take query sinks as an example. On MRPMH, you have:

AddMediaSinksQuery(const MediaSource& source);
Delegate::OnSinksReceived(const MediaSource& source, const
std::vector<MediaSink>& sinks) = 0;

Then on MR, you have:

RegisterMediaSinksObserver(MediaSinksObserver* observer); where observer has
source()
MediaSinksObserver::OnSinksReceived(const std::vector<MediaSink>& sinks);

I don't really see any fundamental differences here. They are pretty much
interchangable. Then the question is, why do we have two patterns?

This also makes me wonder why we have both MediaRouteProviderManagerHost and
MediaRouter interfaces? Can we just have one interface?

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:53: const RouteRequestId& request_id)
= 0;
On 2015/04/08 19:21:31, imcheng1 wrote:
> On 2015/04/08 17:46:25, xhwang wrote:
> > Since we don't actually cancel the request, what's the value of
unregistering
> > the callback? Are we trying to avoid access violation because maybe the
caller
> > of StartRouteRequest() has been destructed?
> > 
> > Is this what you'd expect in implementation? After this call, the request
will
> > still finish, but then we found the callback has already been unregistered,
so
> > we'll drop the result on the floor and do nothing.
> > 
> > If this is the case, usually we don't need to unregister the callback
> > explicitly. You can use a weak pointer to bind the callback. When the caller
> of
> > StartRouteRequest() is destroyed, all callbacks will do nothing. This solves
> the
> > lifetime issue and unregistering callbacks automatically.
> > 
> > let me know if I am missing something though.
> 
> What you described is correct. However removing this API also means it will
> force all users of RequestRoutes to support WeakPtrs. Is that an ok thing to
do?

We use WeakPtrs in many many places in Chromium and media/:

https://code.google.com/p/chromium/codesearch#search/&q=weak_ptr&sq=package:c...

I don't see any problem having all callers of RequestRoute() to manage the
lifetime themselves.

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:21: // for
handling cross-process communication between the Media Router (in browser
s/Media Router/MediaRouter

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:22: // process)
and the Media Route Provider Manager (outside of browser process).
s/Media Route Provider Manager/MediaRouteProviderManager if that's the real
class name.

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:30: // received
from the Media Route Provider Manager.
ditto

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:60: // |sinks|:
List of MediaSinks associated with the routes.
Also drop MediaSinks and |sinks| here.

imcheng

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/router/media_router.h#newcode44 chrome/browser/media/router/media_router.h:44: const MediaRouteResponseCallback& callback) = 0; On 2015/04/08 20:32:51, xhwang ...

5 years, 8 months ago (2015-04-08 23:28:09 UTC) #37

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/320001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:44: const MediaRouteResponseCallback&
callback) = 0;
On 2015/04/08 20:32:51, xhwang wrote:
> On 2015/04/08 19:21:30, imcheng1 wrote:
> > On 2015/04/08 17:46:24, xhwang wrote:
> > > Instead of a customized callback class, can you just use
> > > 
> > > base::Callback<void(int, scopted_ptr<MediaRoute>, const std::string&)> ?
> > 
> > IMO it is cleaner to encapsulate the 3 fields (some of which are optional)
> with
> > some helper logic to interpret the results.
> 
> hmm, you are actually using 3 fields directly in
> MediaRouteProviderManagerHost::Delegate::OnRouteResponseReceived().

per discussion, removing MediaRouteResponse since the logic is quite simple.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route.h:81: MediaRouteState state_;
On 2015/04/08 20:32:51, xhwang wrote:
> On 2015/04/08 19:21:30, imcheng1 wrote:
> > On 2015/04/08 17:46:24, xhwang wrote:
> > > MediaRoute is non-trivial: it has 6 members. I would prefer to make this
> > > immutable (as you have in your comment above at l.34), keep these fields
> const
> > > and make the class DISALLOW_COPY_AND_ASSIGN.
> > > 
> > > For using MediaRoute in stl containers, you can try using ScopedVector
> instead
> > > of std::vector.
> > 
> > MediaRoute is still a data class as it stands today; is there a reason why
we
> > want to DISALLOW_COPY_AND_ASSIGN, other than that this object is not so
small?
> 
> Isn't "this class is not so small" a strong reason to use
> DISALLOW_COPY_AND_ASSIGN?

Per discussion, keep this for now. It is still immutable.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:28: class
Delegate {
On 2015/04/08 20:32:51, xhwang wrote:
> On 2015/04/08 19:21:31, imcheng1 wrote:
> > On 2015/04/08 17:46:24, xhwang wrote:
> > > It seems not all methods of this class are responses. For example,
> OnMessage()
> > > is initiated from the sink. Please make the comments of each method clear
> > about
> > > whether it's a response or not. If it is, which method is it responding
to?
> Is
> > > it a one-to-one mapping between the call and the response, or you could
have
> > > multiple responses for one call?
> > > 
> > 
> > I changed the class level comment here. But, since we already described the
> > relationship in the MediaRouteProviderManagerHost APIs below, I think it's
> fine
> > to not include the comments here as we would be duplicating comments.
> 
> Ack. Thanks!
> > 
> > > Either way, is using callbacks an option? You can have permanent callbacks
> > which
> > > could be called multiple times (e.g. for all following updates).
> > > 
> > 
> > I think this is a matter of preference. To me, a Delegate/Observer pattern
> makes
> > more sense than callbacks if it can be invoked multiple times. I also prefer
> to
> > have one interface/pattern rather than a mix of observer/callbacks so it's
> more
> > uniform and easier to understand.
> 
> Hmm, you are actually having a mix of observer/callbacks in MediaRouter
> interface.
> 
> > > Also, this MRPMH interface looks very similar to MediaRouter interface.
But
> > > there are a lot of differences between them. For example:
StartRouteRequest
> vs
> > > RequestRoute. Also, we use a Delegate here. But we are using callbacks and
> > > observers in MediaRouter. Is there any particular reason for these
> > differences?
> > 
> > Re: StartRouteRequest vs RequestRoute. This is one where it's better to have
> > consistent naming, so I've changed it to RequestRoute in both places. The
> others
> > are named differently for valid reasons.
> > 
> > Re: Delegate vs. cbs/observers in MR. A MediaRouter instance corresponds 1:1
> to
> > a MediaRouteProviderManagerHost instance and throughout its lifetime it may
> > receive many calls forwarded from MediaRouteProviderManagerHost. A Delegate
> > interface is used because of the preference over callbacks for this
situation,
> > as mentioned above. In MR, Observers are used for receiving continuous
updates
> > for queries and callbacks are used for one-time responses. As for naming of
> > Delegate vs Observer, I find Delegate more appropriate when the two
components
> > are 1:1, whereas Observer is used for 1:n.
> 
> I see your point. But I still wonder why we make different choices on
> MediaRouteProviderManagerHost and MediaRouter interfaces. Maybe you can help
on
> understand this better.
> 
> If you take query sinks as an example. On MRPMH, you have:
> 
> AddMediaSinksQuery(const MediaSource& source);
> Delegate::OnSinksReceived(const MediaSource& source, const
> std::vector<MediaSink>& sinks) = 0;
> 
> Then on MR, you have:
> 
> RegisterMediaSinksObserver(MediaSinksObserver* observer); where observer has
> source()
> MediaSinksObserver::OnSinksReceived(const std::vector<MediaSink>& sinks);
> 
> I don't really see any fundamental differences here. They are pretty much
> interchangable. Then the question is, why do we have two patterns?
> 
> This also makes me wonder why we have both MediaRouteProviderManagerHost and
> MediaRouter interfaces? Can we just have one interface?

Discussed offline. Will merged the interfaces into one interface "MediaRouter",
and remove MediaRouteProviderManagerHost. Trying to map the new APIs in my head,
I think that should work, but we will know for sure when this patch is merged
back into private repo.

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1020743003/diff/410001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:53: const RouteRequestId& request_id)
= 0;
On 2015/04/08 20:32:51, xhwang wrote:
> On 2015/04/08 19:21:31, imcheng1 wrote:
> > On 2015/04/08 17:46:25, xhwang wrote:
> > > Since we don't actually cancel the request, what's the value of
> unregistering
> > > the callback? Are we trying to avoid access violation because maybe the
> caller
> > > of StartRouteRequest() has been destructed?
> > > 
> > > Is this what you'd expect in implementation? After this call, the request
> will
> > > still finish, but then we found the callback has already been
unregistered,
> so
> > > we'll drop the result on the floor and do nothing.
> > > 
> > > If this is the case, usually we don't need to unregister the callback
> > > explicitly. You can use a weak pointer to bind the callback. When the
caller
> > of
> > > StartRouteRequest() is destroyed, all callbacks will do nothing. This
solves
> > the
> > > lifetime issue and unregistering callbacks automatically.
> > > 
> > > let me know if I am missing something though.
> > 
> > What you described is correct. However removing this API also means it will
> > force all users of RequestRoutes to support WeakPtrs. Is that an ok thing to
> do?
> 
> We use WeakPtrs in many many places in Chromium and media/:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=weak_ptr&sq=package:c...
> 
> I don't see any problem having all callers of RequestRoute() to manage the
> lifetime themselves.

Removed API.

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
File chrome/browser/media/router/media_route_provider_manager_host.h (right):

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:21: // for
handling cross-process communication between the Media Router (in browser
On 2015/04/08 20:32:51, xhwang wrote:
> s/Media Router/MediaRouter

File removed.

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:22: // process)
and the Media Route Provider Manager (outside of browser process).
On 2015/04/08 20:32:52, xhwang wrote:
> s/Media Route Provider Manager/MediaRouteProviderManager if that's the real
> class name.

"Media Route Provider Manager" is correct. We don't have a
MediaRouteProviderManager class in Chromium.

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:30: // received
from the Media Route Provider Manager.
On 2015/04/08 20:32:52, xhwang wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1020743003/diff/430001/chrome/browser/media/r...
chrome/browser/media/router/media_route_provider_manager_host.h:60: // |sinks|:
List of MediaSinks associated with the routes.
On 2015/04/08 20:32:52, xhwang wrote:
> Also drop MediaSinks and |sinks| here.

Done.

imcheng

Per discussion, moved On*() functions into a separate Delegate interface.

5 years, 8 months ago (2015-04-09 00:03:49 UTC) #38

xhwang

Thanks! It seems we still have some cleanup to do. See comment below. https://codereview.chromium.org/1020743003/diff/490001/chrome/browser/media/router/media_router.h File ...

5 years, 8 months ago (2015-04-09 16:54:27 UTC) #39

imcheng

https://codereview.chromium.org/1020743003/diff/490001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/490001/chrome/browser/media/router/media_router.h#newcode55 chrome/browser/media/router/media_router.h:55: const std::vector<MediaSink>& sinks) = 0; On 2015/04/09 16:54:26, xhwang ...

5 years, 8 months ago (2015-04-09 17:06:37 UTC) #40

xhwang

https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h#newcode24 chrome/browser/media/router/media_router.h:24: base::Callback<void(scoped_ptr<MediaRoute>, const std::string&)>; Since this is part of the ...

5 years, 8 months ago (2015-04-09 17:33:34 UTC) #41

imcheng

https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h#newcode24 chrome/browser/media/router/media_router.h:24: base::Callback<void(scoped_ptr<MediaRoute>, const std::string&)>; On 2015/04/09 17:33:34, xhwang wrote: > ...

5 years, 8 months ago (2015-04-09 17:49:10 UTC) #42

Kevin Marshall

https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h#newcode51 chrome/browser/media/router/media_router.h:51: // Called when there is a message from a ...

5 years, 8 months ago (2015-04-09 18:17:00 UTC) #43

Kevin Marshall

https://codereview.chromium.org/1020743003/diff/550001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/550001/chrome/browser/media/router/media_router.h#newcode82 chrome/browser/media/router/media_router.h:82: virtual bool RegisterMediaSinksObserver(MediaSinksObserver* observer) = 0; Since these are ...

5 years, 8 months ago (2015-04-09 18:32:38 UTC) #44

imcheng

https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1020743003/diff/510001/chrome/browser/media/router/media_router.h#newcode51 chrome/browser/media/router/media_router.h:51: // Called when there is a message from a ...

5 years, 8 months ago (2015-04-09 18:44:27 UTC) #45

xhwang

Thanks! This CL is much cleaner now. Thanks! Do you want me to review it ...

5 years, 8 months ago (2015-04-09 19:39:39 UTC) #46

imcheng

Hey Xiaohan, I am about half way with merging this patch back to private repo, ...

5 years, 8 months ago (2015-04-09 21:04:44 UTC) #47

xhwang

Thanks for the patience! I only have a few more nits. Otherwise, LGTM~~~ https://codereview.chromium.org/1020743003/diff/570001/chrome/browser/media/router/media_router_impl.h File ...

5 years, 8 months ago (2015-04-09 21:33:07 UTC) #48

imcheng

https://codereview.chromium.org/1020743003/diff/570001/chrome/browser/media/router/media_router_impl.h File chrome/browser/media/router/media_router_impl.h (right): https://codereview.chromium.org/1020743003/diff/570001/chrome/browser/media/router/media_router_impl.h#newcode52 chrome/browser/media/router/media_router_impl.h:52: void RequestRoute(const MediaSource& source, On 2015/04/09 21:33:06, xhwang wrote: ...

5 years, 8 months ago (2015-04-09 22:02:18 UTC) #49

imcheng

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, marshallk@google.com, imcheng@chromium.org, xhwang@chromium.org ...

5 years, 8 months ago (2015-04-09 22:23:07 UTC) #51

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1020743003/590001

5 years, 8 months ago (2015-04-09 22:23:41 UTC) #52

commit-bot: I haz the power

Patchset 27 (id:??) landed as https://crrev.com/45aa4cbbb775de85003187a8b1371d7922dc0194 Cr-Commit-Position: refs/heads/master@{#324541}

5 years, 8 months ago (2015-04-09 23:52:36 UTC) #54

Sorin Jianu

5 years, 8 months ago (2015-04-10 01:04:04 UTC) #55

Message was sent while issue was closed.

A revert of this CL (patchset #27 id:590001) has been created in
https://codereview.chromium.org/1077963002/ by sorin@chromium.org.

The reason for reverting is: Suspecting it breaks building unit_tests_main on
Mac GN and Win8 GN

http://build.chromium.org/p/chromium.win/builders/Win8%20GN/builds/6186

FAILED: E:/b/depot_tools/python276_bin/python.exe gyp-win-tool link-wrapper
environment.x86 False link.exe /nologo /OUT:unit_tests_main.exe
/PDB:unit_tests_main.exe.pdb @unit_tests_main.exe.rsp
browser.browser_ppapi_host_impl.obj : error LNK2019: unresolved external symbol
"public: __thiscall ppapi::host::PpapiHost::PpapiHost(class IPC::Sender *,class
ppapi::PpapiPermissions const &)"
(??0PpapiHost@host@ppapi@@QAE@PAVSender@IPC@@ABVPpapiPermissions@2@@Z)
referenced in function "public: __thiscall
content::BrowserPpapiHostImpl::BrowserPpapiHostImpl(class IPC::Sender *,class
ppapi::PpapiPermissions const &,class std::basic_string<char,struct
std::char_traits<char>,class std::allocator<char> > const &,class base::FilePath
const &,class base::FilePath const &,bool,bool)"
(??0BrowserPpapiHostImpl@content@@QAE@PAVSender@IPC@@ABVPpapiPermissions@ppapi@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@ABVFilePath@base@@3_N4@Z)

browser.browser_ppapi_host_impl.obj : error LNK2019: unresolved external symbol
"public: void __thiscall ppapi::host::PpapiHost::AddHostFactoryFilter(class
scoped_ptr<class ppapi::host::HostFactory,struct base::DefaultDeleter<class
ppapi::host::HostFactory> >)"
(?AddHostFactoryFilter@PpapiHost@host@ppapi@@QAEXV?$scoped_ptr@VHostFactory@host@ppapi@@U?$DefaultDeleter@VHostFactory@host@ppapi@@@base@@@@@Z)
referenced in function "public: __thiscall
content::BrowserPpapiHostImpl::BrowserPpapiHostImpl(class IPC::Sender *,class
ppapi::PpapiPermissions const &,class std::basic_string<char,struct
std::char_traits<char>,class std::allocator<char> > const &,class base::FilePath
const &,class base::FilePath const &,bool,bool)"
(??0BrowserPpapiHostImpl@content@@QAE@PAVSender@IPC@@ABVPpapiPermissions@ppapi@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@ABVFilePath@base@@3_N4@Z)

browser.pepper_tcp_socket_message_filter.obj : error LNK2001: unresolved
external symbol "public: static void __cdecl
ppapi::host::internal::ResourceMessageFilterDeleteTraits::Destruct(class
ppapi::host::ResourceMessageFilter const *)"
(?Destruct@ResourceMessageFilterDeleteTraits@internal@host@ppapi@@SAXPBVResourceMessageFilter@34@@Z)

browser.pepper_truetype_font_list_host.obj : error LNK2001: unresolved external
symbol "public: static void __cdecl
ppapi::host::internal::ResourceMessageFilterDeleteTraits::Destruct(class
ppapi::host::ResourceMessageFilter const *)"
(?Destruct@ResourceMessageFilterDeleteTraits@internal@host@ppapi@@SAXPBVResourceMessageFilter@34@@Z)

browser.pepper_udp_socket_message_filter.obj : error LNK2001: unresolved
external symbol "public: static void __cdecl
ppapi::host::internal::ResourceMessageFilterDeleteTraits::Destruct(class
ppapi::host::ResourceMessageFilter const *)"
(?Destruct@ResourceMessageFilterDeleteTraits@internal@host@ppapi@@SAXPBVResourceMessageFilter@34@@Z)

=======================

FAILED: /Volumes/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -arch x86_64 -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
-mmacosx-version-min=10.6 -Wl,-search_paths_first -L. -Wl,-rpath,@loader_path/.
-Wl,-rpath,@loader_path/../../.. -Wl,-pie -o ./unit_tests_main
-Wl,-filelist,./unit_tests_main.rsp ./libffmpegsumo.dylib -framework AppKit
-framework ApplicationServices -framework Carbon -framework CoreFoundation
-framework Foundation -framework IOKit -framework Security -framework QuartzCore
-framework SystemConfiguration -lresolv -framework Accelerate -framework
AudioUnit -framework CoreVideo -framework Cocoa -framework IOSurface -framework
OpenGL -framework CoreMIDI -framework QTKit -framework AudioToolbox -framework
CoreAudio -lbsm -framework IOBluetooth
Undefined symbols for architecture x86_64:
  "SuddenMotionSensor::ReadSensorValues(float*)", referenced from:
      (anonymous namespace)::FetchOrientation(SuddenMotionSensor*,
content::SharedMemorySeqLockBuffer<blink::WebDeviceOrientationData>*) in
browser.data_fetcher_shared_memory_mac.o
      (anonymous namespace)::FetchMotion(SuddenMotionSensor*,
content::SharedMemorySeqLockBuffer<blink::WebDeviceMotionData>*) in
browser.data_fetcher_shared_memory_mac.o
  "SuddenMotionSensor::Create()", referenced from:
      content::DataFetcherSharedMemory::Start(content::ConsumerType, void*) in
browser.data_fetcher_shared_memory_mac.o
  "SuddenMotionSensor::~SuddenMotionSensor()", referenced from:
      base::DefaultDeleter<SuddenMotionSensor>::operator()(SuddenMotionSensor*)
const in browser.data_fetcher_shared_memory_mac.o
  "aura::WindowEventDispatcher::HoldPointerMoves()", referenced from:
      content::CompositorResizeLock::CompositorResizeLock(aura::WindowTreeHost*,
gfx::Size, bool, base::TimeDelta const&) in
browser.compositor_resize_lock_aura.o
  "aura::WindowEventDispatcher::ReleasePointerMoves()", referenced from:
      content::CompositorResizeLock::CancelLock() in
browser.compositor_resize_lock_aura.o
  "blink::OpenTypeVerticalData::OpenTypeVerticalData(blink::FontPlatformData
const&)", referenced from:
      blink::OpenTypeVerticalData::create(blink::FontPlatformData const&) in
blink_platform.FontCache.o
.

Issue 1020743003: [Media Router] Design MediaRouter interface with stub implementation. (Closed)

Description

Patch Set 1 : #

Patch Set 2 : move MRPMH::Delegate inheritence from MR to MRImpl #

Patch Set 3 : Addressed Kevin's comments, removed extension_* files for now #

Patch Set 4 : Addressed mark's comments #

Patch Set 5 : Rebase + fix compile' #

Patch Set 6 : Add MediaRouter empty virtual dtor #

Patch Set 7 : add dependencies to gn #

Patch Set 8 : rm c/b gn dep #

Patch Set 9 : rm dependency on helper function in chrome_browser #

Patch Set 10 : Addressed mark's comments #

Patch Set 11 : Addressed Kevin's comments #

Patch Set 12 : Addressed Xiaohan's comments #

Patch Set 13 : fix compile #

Patch Set 14 : Addressed Xiaohan's 2nd set of comments #

Patch Set 15 : Update MediaRoutesObserver comments #

Patch Set 16 : git cl format #

Patch Set 17 : Move RegisterObserver APIs to MediaRouter private interface #

Patch Set 18 : gcl format #

Patch Set 19 : Addressed Xiaohan's 3rd set of comments #

Patch Set 20 : Addressed Xiaohan's 4th comments #

Patch Set 21 : gcl format #

Patch Set 22 : Moved On*() functions into a separate Delegate interface #

Patch Set 23 : Removed OnSinksReceived/OnRoutesReceived from Delegate #

Patch Set 24 : Add comment #

Patch Set 25 : Add missing public: #

Patch Set 26 : change MediaRouter private APIs to protected #

Patch Set 27 : Updated comments and RequestRoute to take a MediaRouteId #

Messages