Upstreaming review for Media Router Mojo interface.

Upstreaming review for Media Router Mojo interface.

R=xhwang@chromium.org
BUG=464205
Committed: https://crrev.com/654644c800bf663c6a1c35e81c9d2db9759103d1
Cr-Commit-Position: refs/heads/master@{#327387}

[xhwang, 2015-04-27 17:52:02]

Thanks! I am recently involved in a mojo naming discussion so I have many naming
comments. Sorry about that :)

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.gyp (right):

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.gyp:42: # Mojo compiler for the Media
Router IPC API.
s/IPC//? We use mojo to hide IPC.... Or put it another way, this mojom interface
can also work in-process.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.gyp:46: 'media_router_api.mojom',
Naming is hard and we don't have a consensus on mojom naming yet.  But usually
we don't put "api" in the mojom file name. How about just "media_router.mojom"?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.gyp:47: ],
nit: indentation is off

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_api.mojom (right):

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:6: // Media Router Provider
Component Extension.
This interface should be generic, it's not limited to extensions. Sounds like
"extension" is just an implementation detail.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:8: module media_router.mojom;
Typically we don't put mojom in the module name. But without it you'll end up
with name conflict :( I am fighting with this in media/ as well and couldn't
find a good solution. See [1] for discussion.

If you like nested namespace, how about media_router.mojo or
media_router.interfaces? Using "mojom" here seems wrong because it's the file
type.

[1] https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/j6NXQXUvg8E

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:10: struct Provider {
This struct is not used anywhere in this mojom file. In that case can we drop
it?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:19: //                 
UNLOADED <======+
nit: Will it cause confusing that LOADING appears in two places?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:34: // Required.
Not needed? Optional/nullable members are marked with "?", which is clear.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:49: string? media_source;
Is this an id or a name?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:55: string? sink_name;
In MediaSink, both id and name are required. But here id is required but name is
optional? Why is this difference? Can we just use a MediaSink here?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:68: // Issue created by a
media route provider and shown to the user.
nit: Could you please define "issue"?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:80: // above route detail and
controls.
hmm, seems like these are just UI implementation details. You probably want to
explain what a "issue" is, and what is_blocking means.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:128: // to the Media Router
client.  For the component extension, this is the Media
ditto about "extension" is implementation detail.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:132: // at |source_urn| to
|sink_id|.
What's the relationship between this |source_run| and the |media_source| in
MediaRoute interface (l.49)?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:150: CloseRoute(string
route_id);
hmm... With CreateRoute/JoinRoute/CloseRoute/PostMessage, this
(ProviderManagerService) looks more like a "MediaRouter" interface. In
MediaRouterApi you have a lot of On* methods, which looks more like a "Client".
Does this make any sense?

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:176: interface MediaRouterApi
{
Just MediaRouter? You have nested namespace so there's no naming conflict.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:181: (string instance_id);
See naming comment above. In mojo, usually we have a Foo interface, then we have
a FooClient interface. Then this call will become something like SetClient().

[Kevin Marshall, 2015-04-27 19:23:50]

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

[Kevin Marshall, 2015-04-27 19:23:51]

Naming nits are good, so thanks for that.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.gyp (right):

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.gyp:42: # Mojo compiler for the Media
Router IPC API.
On 2015/04/27 17:52:01, xhwang wrote:
> s/IPC//? We use mojo to hide IPC.... Or put it another way, this mojom
interface
> can also work in-process.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.gyp:46: 'media_router_api.mojom',
On 2015/04/27 17:52:01, xhwang wrote:
> Naming is hard and we don't have a consensus on mojom naming yet.  But usually
> we don't put "api" in the mojom file name. How about just
"media_router.mojom"?

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.gyp:47: ],
On 2015/04/27 17:52:01, xhwang wrote:
> nit: indentation is off

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_api.mojom (right):

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:6: // Media Router Provider
Component Extension.
On 2015/04/27 17:52:02, xhwang wrote:
> This interface should be generic, it's not limited to extensions. Sounds like
> "extension" is just an implementation detail.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:8: module media_router.mojom;
On 2015/04/27 17:52:02, xhwang wrote:
> Typically we don't put mojom in the module name. But without it you'll end up
> with name conflict :( I am fighting with this in media/ as well and couldn't
> find a good solution. See [1] for discussion.
> 
> If you like nested namespace, how about media_router.mojo or
> media_router.interfaces? Using "mojom" here seems wrong because it's the file
> type.
> 
> [1]
https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/j6NXQXUvg8E

I'd prefer to use a nested type for naming conflicts, since there are so many
structures that are reimplemented in Mojo.

I'll go with "interfaces" because IMO a "mojo" prefix might cause some confusion
between the Mojo common components and our generated code.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:10: struct Provider {
On 2015/04/27 17:52:02, xhwang wrote:
> This struct is not used anywhere in this mojom file. In that case can we drop
> it?

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:19: //                 
UNLOADED <======+
On 2015/04/27 17:52:01, xhwang wrote:
> nit: Will it cause confusing that LOADING appears in two places?

Removed.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:34: // Required.
On 2015/04/27 17:52:02, xhwang wrote:
> Not needed? Optional/nullable members are marked with "?", which is clear.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:49: string? media_source;
On 2015/04/27 17:52:02, xhwang wrote:
> Is this an id or a name?

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:55: string? sink_name;
On 2015/04/27 17:52:02, xhwang wrote:
> In MediaSink, both id and name are required. But here id is required but name
is
> optional? Why is this difference? Can we just use a MediaSink here?

Good catch. In the past there was a call that returned sinks and routes
together, so the sink was made optional to reduce redundancy. That call was
taken out in the previous upstreaming CL, so this optimization is now N/A.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:68: // Issue created by a
media route provider and shown to the user.
On 2015/04/27 17:52:02, xhwang wrote:
> nit: Could you please define "issue"?

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:80: // above route detail and
controls.
On 2015/04/27 17:52:02, xhwang wrote:
> hmm, seems like these are just UI implementation details. You probably want to
> explain what a "issue" is, and what is_blocking means.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:128: // to the Media Router
client.  For the component extension, this is the Media
On 2015/04/27 17:52:01, xhwang wrote:
> ditto about "extension" is implementation detail.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:132: // at |source_urn| to
|sink_id|.
On 2015/04/27 17:52:02, xhwang wrote:
> What's the relationship between this |source_run| and the |media_source| in
> MediaRoute interface (l.49)?

The same thing.

Fixed everywhere: s/source_urn/media_source/g.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:150: CloseRoute(string
route_id);
On 2015/04/27 17:52:01, xhwang wrote:
> hmm... With CreateRoute/JoinRoute/CloseRoute/PostMessage, this
> (ProviderManagerService) looks more like a "MediaRouter" interface. In
> MediaRouterApi you have a lot of On* methods, which looks more like a
"Client".
> Does this make any sense?

I agree that the methods are the same as the MediaRouter, but the interface is
named this way because it's used by the MR to call the MRPM, not the other way
around.

Is "Client" in common parlance these days? I think it might be easiest to name
the interfaces by their backing component (you use an interfaces::MediaRouter to
talk to the MediaRouter; interfaces::ProviderManager to talk to the
ProviderManager, etc.)

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:176: interface MediaRouterApi
{
On 2015/04/27 17:52:02, xhwang wrote:
> Just MediaRouter? You have nested namespace so there's no naming conflict.

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:181: (string instance_id);
On 2015/04/27 17:52:02, xhwang wrote:
> See naming comment above. In mojo, usually we have a Foo interface, then we
have
> a FooClient interface. Then this call will become something like SetClient().

I'm unsure if "Client" is useful in this case, because the usual observer roles
are inverted for this API. The ProviderManagerService is the primary actor here,
and the MediaRouter is just receiving the results.

[xhwang, 2015-04-28 06:08:18]

Thanks. More naming/interface discussions.

Note that some comments are in the previous PS.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_api.mojom (right):

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:8: module media_router.mojom;
On 2015/04/27 19:23:51, Kevin Marshall wrote:
> On 2015/04/27 17:52:02, xhwang wrote:
> > Typically we don't put mojom in the module name. But without it you'll end
up
> > with name conflict :( I am fighting with this in media/ as well and couldn't
> > find a good solution. See [1] for discussion.
> > 
> > If you like nested namespace, how about media_router.mojo or
> > media_router.interfaces? Using "mojom" here seems wrong because it's the
file
> > type.
> > 
> > [1]
> https://groups.google.com/a/chromium.org/forum/#!topic/mojo-dev/j6NXQXUvg8E
> 
> I'd prefer to use a nested type for naming conflicts, since there are so many
> structures that are reimplemented in Mojo.
> 
> I'll go with "interfaces" because IMO a "mojo" prefix might cause some
confusion
> between the Mojo common components and our generated code.

Agreed. I might do the same for media mojom interfaces.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:150: CloseRoute(string
route_id);
On 2015/04/27 19:23:51, Kevin Marshall wrote:
> On 2015/04/27 17:52:01, xhwang wrote:
> > hmm... With CreateRoute/JoinRoute/CloseRoute/PostMessage, this
> > (ProviderManagerService) looks more like a "MediaRouter" interface. In
> > MediaRouterApi you have a lot of On* methods, which looks more like a
> "Client".
> > Does this make any sense?
> 
> I agree that the methods are the same as the MediaRouter, but the interface is
> named this way because it's used by the MR to call the MRPM, not the other way
> around.
> 
> Is "Client" in common parlance these days? I think it might be easiest to name
> the interfaces by their backing component (you use an interfaces::MediaRouter
to
> talk to the MediaRouter; interfaces::ProviderManager to talk to the
> ProviderManager, etc.)

Client is still pretty popular in the mojo world. In a lot of IPC/Mojo
communication, one side asks the other side to do something (e.g. to get
geolocation, to decode media streams, etc). So a mojo interface defines what to
do. For example, for Foo interface, the caller will hold a FooPtr, and in the
other side you'll have a Foo implementation. Sometimes the implementation needs
to send something back to the caller, and this is where we have a FooClient
interface.

Currently in C++, MediaRouter interface is the interface that provides and
manages routes, e.g. RequestRoute, CloseRoute. It would be nice that the mojom
interface is consistent with that model. 

The current interfaces' naming is very confusing:
- Mojo ProviderManagerService <-> C++ MediaRouter
- Mojo MediaRouter <-> C++ MediaRoutner::Delegate

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:181: (string instance_id);
On 2015/04/27 19:23:51, Kevin Marshall wrote:
> On 2015/04/27 17:52:02, xhwang wrote:
> > See naming comment above. In mojo, usually we have a Foo interface, then we
> have
> > a FooClient interface. Then this call will become something like
SetClient().
> 
> I'm unsure if "Client" is useful in this case, because the usual observer
roles
> are inverted for this API. The ProviderManagerService is the primary actor
here,
> and the MediaRouter is just receiving the results.

Ack. But from API's perspective, this is confusing. See comments above. Mabe I
am missing some big picture.

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
tip: Next time upload a PS with all changes without renaming the file. Then
upload a PS with renaming only. That way it's much easier to see what has been
changed since the last PS.

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:29: string sink_name;
Can you replace this two with

MediaSink media_sink;

?

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:93: interface
ProviderManagerService {
I am still not sure about this naming. Can you just name this
MediaRouterProviderManager, based on your file/interface level comments?

BTW, it's not recommended to name a mojo interface "Service" now based on the
naming discussion thread.

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:94: // Signals the client to
route the media located
What is a "client"? Maybe just s/client/MediaRouter?

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:101: (MediaRoute? route, string?
error_text);
Is this equivalent to c++ MediaRoutner::RequestRoute()? 

Also, we don't have JoinRoute() on the C++ interface. Is this expected?

[Kevin M, 2015-04-28 17:20:30]

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_api.mojom (right):

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:150: CloseRoute(string
route_id);
On 2015/04/28 06:08:18, xhwang wrote:
> On 2015/04/27 19:23:51, Kevin Marshall wrote:
> > On 2015/04/27 17:52:01, xhwang wrote:
> > > hmm... With CreateRoute/JoinRoute/CloseRoute/PostMessage, this
> > > (ProviderManagerService) looks more like a "MediaRouter" interface. In
> > > MediaRouterApi you have a lot of On* methods, which looks more like a
> > "Client".
> > > Does this make any sense?
> > 
> > I agree that the methods are the same as the MediaRouter, but the interface
is
> > named this way because it's used by the MR to call the MRPM, not the other
way
> > around.
> > 
> > Is "Client" in common parlance these days? I think it might be easiest to
name
> > the interfaces by their backing component (you use an
interfaces::MediaRouter
> to
> > talk to the MediaRouter; interfaces::ProviderManager to talk to the
> > ProviderManager, etc.)
> 
> Client is still pretty popular in the mojo world. In a lot of IPC/Mojo
> communication, one side asks the other side to do something (e.g. to get
> geolocation, to decode media streams, etc). So a mojo interface defines what
to
> do. For example, for Foo interface, the caller will hold a FooPtr, and in the
> other side you'll have a Foo implementation. Sometimes the implementation
needs
> to send something back to the caller, and this is where we have a FooClient
> interface.
> 
> Currently in C++, MediaRouter interface is the interface that provides and
> manages routes, e.g. RequestRoute, CloseRoute. It would be nice that the mojom
> interface is consistent with that model. 
> 
> The current interfaces' naming is very confusing:
> - Mojo ProviderManagerService <-> C++ MediaRouter
> - Mojo MediaRouter <-> C++ MediaRoutner::Delegate
> 
> 

Done.

https://codereview.chromium.org/1055403006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_api.mojom:181: (string instance_id);
On 2015/04/28 06:08:18, xhwang wrote:
> On 2015/04/27 19:23:51, Kevin Marshall wrote:
> > On 2015/04/27 17:52:02, xhwang wrote:
> > > See naming comment above. In mojo, usually we have a Foo interface, then
we
> > have
> > > a FooClient interface. Then this call will become something like
> SetClient().
> > 
> > I'm unsure if "Client" is useful in this case, because the usual observer
> roles
> > are inverted for this API. The ProviderManagerService is the primary actor
> here,
> > and the MediaRouter is just receiving the results.
> 
> Ack. But from API's perspective, this is confusing. See comments above. Mabe I
> am missing some big picture.

Done.

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:93: interface
ProviderManagerService {
On 2015/04/28 06:08:18, xhwang wrote:
> I am still not sure about this naming. Can you just name this
> MediaRouterProviderManager, based on your file/interface level comments?
> 
> BTW, it's not recommended to name a mojo interface "Service" now based on the
> naming discussion thread.

Based on our discussion about the "Client" naming - how is
MediaRouter+MediaRouterClient?

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:94: // Signals the client to
route the media located
On 2015/04/28 06:08:18, xhwang wrote:
> What is a "client"? Maybe just s/client/MediaRouter?

The call is originating from the MR and sent to the (client|provider).

I'll use "provider" here because changing the name of the interface to
MediaRouterClient means that the "client" target is implied.

https://codereview.chromium.org/1055403006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:101: (MediaRoute? route, string?
error_text);
On 2015/04/28 06:08:18, xhwang wrote:
> Is this equivalent to c++ MediaRoutner::RequestRoute()? 

Yes. We changed it to CreateRoute because "request" is ambiguous -- there are
two different modes of route establishment, create-new or join-existing.

> Also, we don't have JoinRoute() on the C++ interface. Is this expected?

That's a work in progress that Derek's working on now. Since that's not yet
fully baked, I'll omit Join from these upstreaming CLs from now on.

[xhwang, 2015-04-28 17:44:19]

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:6: // Media Router Provider
Manager.
"Media Router Provider Manager" is an implementation detail. In this mojom file,
we are only defining the MR and MRC interfaces.

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:89: // Media Router Provider
Manager.
ditto

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:90: interface MediaRouterClient {
I still feel we have the names inverted. From an design's perspective, it'll be
very confusing that a mojo MediaRouter interface is fundamentally different than
a C++ MediaRouter interface.

I understand that the real MediaRouter implementation (say MediaRouterImpl) will
receive messages back from the MediaRouterProviderManager. But this is
implementation detail. You can think it this way:

- The MediaRouterImpl provides services to callers in the browser process to
provide services like CreateRoute().
- The MediaRouterImpl needs a remote MediaRouter (via mojo, e.g. MediaRouterPtr)
to help to do some work (e.g. CreateRoute()).
- When working with the remote MediaRouter, the MediaRouterImpl serves as a
MediaRouterClient.

With this, MediaRouter is a consistent interface, you can have definitions like:
- MediaRouter: An interface to help create, manage (via PostMessage) and close
MediaRoutes.
- MediaRouterClient: A client interface of MediaRouter to receive MediaRoute
related updates.

WDYT?

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:91: // Signals the provider to
route the media located
Now looking at the interface, we only have MediaRouter and MediaRouterClient. We
don't need to introduce/mention a "provider" here, which is an implementation
detail.

Also now we do have a "client", and we can use "client" in the comments.

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:129: // Provider Manager.
ditto

[Kevin M, 2015-04-28 21:00:26]

Thanks for meeting with us in-person! Here's another go at naming.

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1055403006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:6: // Media Router Provider
Manager.
On 2015/04/28 17:44:18, xhwang wrote:
> "Media Router Provider Manager" is an implementation detail. In this mojom
file,
> we are only defining the MR and MRC interfaces.

Removed introductory comment since it seems redundant.

[xhwang, 2015-04-28 21:40:12]

This looks nice and clean. LGTM!

[chromium-reviews, 2015-04-28 21:42:45]

Cool, now treat yourself to a beer bonus. ;)

On Tue, Apr 28, 2015 at 2:40 PM, <xhwang@chromium.org> wrote:

> This looks nice and clean. LGTM!
>
> https://codereview.chromium.org/1055403006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[imcheng (use chromium acct), 2015-04-28 21:45:54]

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

[imcheng (use chromium acct), 2015-04-28 21:45:54]

lgtm

[Kevin Marshall, 2015-04-28 21:46:30]

The CQ bit was checked by marshallk@google.com

[commit-bot: I haz the power, 2015-04-28 21:48:49]

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

[commit-bot: I haz the power, 2015-04-28 23:23:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-28 23:24:16]

Patchset 4 (id:??) landed as
https://crrev.com/654644c800bf663c6a1c35e81c9d2db9759103d1
Cr-Commit-Position: refs/heads/master@{#327387}