[Media Router] Add Media Router Mojo impl code.

[Media Router] Add Media Router Mojo impl code.

Took over Kevin's patch 1126923002.

This code bridges calls from the Media Router to the Media Router
Provider Manager. CL comprises:
- Logic for marshaling data across internal and Mojo structs via
Type Converters already upstreamed.
- Awareness of the MRPM extension's suspend state
- A BrowserContextKeyedServiceFactory for constructing and
managing Mojo service objects.

Also included are unit tests, supporting mock classes, and some trivial
miscellaneous name changes.

BUG=461815, 464205
Committed: https://crrev.com/7f1dfe3b436ae1df300867f7dab3c8e0569ed95d
Cr-Commit-Position: refs/heads/master@{#331868}

[imcheng (use chromium acct), 2015-05-18 21:14:34]

Patchset #1 (id:1) has been deleted

[imcheng (use chromium acct), 2015-05-18 22:16:55]

Patchset #2 (id:40001) has been deleted

[imcheng (use chromium acct), 2015-05-18 22:17:42]

imcheng@google.com changed reviewers:
+ wez@chromium.org

[imcheng (use chromium acct), 2015-05-18 22:17:42]

I updated the patch description per your suggestion. PTAL.

[imcheng (use chromium acct), 2015-05-19 21:56:59]

Ping. Also ping for https://codereview.chromium.org/1139203003/

[Wez, 2015-05-20 23:40:02]

On 2015/05/19 21:56:59, imcheng wrote:
> Ping. Also ping for https://codereview.chromium.org/1139203003/

Gah, my comments ended up on https://codereview.chromium.org/1126923002/...
sorry :(

Can we close Kevin's patch out, to avoid confusion?

[imcheng (use chromium acct), 2015-05-21 19:29:14]

Addressed comments from https://codereview.chromium.org/1126923002/. PTAL.

[Wez, 2015-05-22 00:05:39]

Sorry, there's a load more comments in replies on the original CL - if you can
put your response replies in this CL then that should help me remember not to
comment in the wrong place again... :-/

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:83: // It is invalid to request the
same observer more than once and will result
s/request/register

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:97: // It is invalid to request the
same observer more than once and will result
s/request/register

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:106: // It is invalid to request the
same observer more than once and will result
s/request/register

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:43: // all pending
requests with failure.
nit: Add a bug# as you have done below?

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:225: // MediaSinks update
yet. Alternatively we could implement caching in this
I don't understand the "even if..." bit - basically we call it so that we'll get
a fresh set of sink notifications to populate the new observer with, right?

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:245: // is not inside the
ObserverList iteration.
That in turn relies on the observers not unregistering themselves from within
observer callbacks - that's something you need to document on the relevant APIs,
not here.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:334:
DVLOG_WITH_INSTANCE(2) << "EnqueueTask (size=" << pending_requests_.size()
nit: This logging is misleading; it's not the size of the task, but the length
of the queue you're logging - suggest rewording to e.g. "queue-length:<n>"

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:38: class
MediaRouterMojoImpl : public MediaRouter,
Sorry, I meant to mention this fundamental question earlier...

Why is MediaRouterMojoImpl a MediaRouter, rather than _containing_ a MediaRouter
instance? It seems that the latter gives the right separation between the roles
- this class is really only intended to expose MediaRouter functions to the
component extension via Mojo, right?

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:106: void
Bind(mojo::InterfaceRequest<interfaces::MediaRouterObserver> request);
I think you're right that there's no real value in breaking this and
InitExtensionInfo out of BindToRequest - you basically just want a non-static
equivalent of BindToRequest on this class, for BindToRequest to call.

Or, since BindToRequest is a member of this class, it could reasonably set the
fields in the Impl directly, removing the need for a non-static helper.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:111: // instance and
extension are keyed on.
Does "keyed on" mean "belong to", in effect?

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:122: // Runs the closures
queued in |pending_requests_|.
nit: Suggest "Dispatches the Mojo requests queued in |pending_requests_|."

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:149: // This binding exists
so that MediaRouterObserver calls from the
That is fairly intuitive from the class names; the question is what it achieves
- what _are_ MediaRouterObserver calls? Is MediaRouterObserver the API the
extension uses to watch for requests from MediaRouter, for example?

I'd expect a comment like:
"Binds |this| to service Mojo requests for <some sort of purpose> from the
component extension."

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:157: // extension becomes
suspended or if there is a connection error.
So this is basically the API through which we send requests to the Media Route
Provider, in which case I still don't understand the MediaRouter naming
convention. :(

[imcheng (use chromium acct), 2015-05-22 00:54:07]

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:83: // It is invalid to request the
same observer more than once and will result
On 2015/05/22 00:05:38, Wez wrote:
> s/request/register

Done.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:97: // It is invalid to request the
same observer more than once and will result
On 2015/05/22 00:05:38, Wez wrote:
> s/request/register

Done.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:106: // It is invalid to request the
same observer more than once and will result
On 2015/05/22 00:05:38, Wez wrote:
> s/request/register

Done.

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:43: // all pending
requests with failure.
On 2015/05/22 00:05:38, Wez wrote:
> nit: Add a bug# as you have done below?

Done.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:225: // MediaSinks update
yet. Alternatively we could implement caching in this
On 2015/05/22 00:05:38, Wez wrote:
> I don't understand the "even if..." bit - basically we call it so that we'll
get
> a fresh set of sink notifications to populate the new observer with, right?

Yes. Reworded comments a bit.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:245: // is not inside the
ObserverList iteration.
On 2015/05/22 00:05:38, Wez wrote:
> That in turn relies on the observers not unregistering themselves from within
> observer callbacks - that's something you need to document on the relevant
APIs,
> not here.

Since Unregister*Observer are private APIs only used by the base observer
classes, it should be impossible for that to happen.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:334:
DVLOG_WITH_INSTANCE(2) << "EnqueueTask (size=" << pending_requests_.size()
On 2015/05/22 00:05:38, Wez wrote:
> nit: This logging is misleading; it's not the size of the task, but the length
> of the queue you're logging - suggest rewording to e.g. "queue-length:<n>"

Done. Also moved it after the push_back() so it logs the size after the
enqueueing

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:38: class
MediaRouterMojoImpl : public MediaRouter,
On 2015/05/22 00:05:38, Wez wrote:
> Sorry, I meant to mention this fundamental question earlier...
> 
> Why is MediaRouterMojoImpl a MediaRouter, rather than _containing_ a
MediaRouter
> instance? It seems that the latter gives the right separation between the
roles
> - this class is really only intended to expose MediaRouter functions to the
> component extension via Mojo, right?

I think "really only intended to expose MediaRouter functions to the
component extension via Mojo" is correct. However, the callers of Media Router
within Chrome (PresentationServiceDelegate, WebUI) does not know/care about this
detail. All they know is that there is a MediaRouter in Chrome that they talk
to. Which is why this implements MediaRouter.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:106: void
Bind(mojo::InterfaceRequest<interfaces::MediaRouterObserver> request);
On 2015/05/22 00:05:39, Wez wrote:
> I think you're right that there's no real value in breaking this and
> InitExtensionInfo out of BindToRequest - you basically just want a non-static
> equivalent of BindToRequest on this class, for BindToRequest to call.
> 
> Or, since BindToRequest is a member of this class, it could reasonably set the
> fields in the Impl directly, removing the need for a non-static helper.

When I tried to change it I realized why it was structured this way. It's
because there is test setup code that only calls Bind() but set extension info
via other means, and then there are unit tests that call them both but where
using the static method isn't an option. So the cleanest way would be to have
these 2 member functions separately.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:111: // instance and
extension are keyed on.
On 2015/05/22 00:05:38, Wez wrote:
> Does "keyed on" mean "belong to", in effect?

Yes

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:122: // Runs the closures
queued in |pending_requests_|.
On 2015/05/22 00:05:38, Wez wrote:
> nit: Suggest "Dispatches the Mojo requests queued in |pending_requests_|."

Done.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:149: // This binding exists
so that MediaRouterObserver calls from the
On 2015/05/22 00:05:38, Wez wrote:
> That is fairly intuitive from the class names; the question is what it
achieves
> - what _are_ MediaRouterObserver calls? Is MediaRouterObserver the API the
> extension uses to watch for requests from MediaRouter, for example?
> 
> I'd expect a comment like:
> "Binds |this| to service Mojo requests for <some sort of purpose> from the
> component extension."

Yes. The component extension is a MediaRouter. Just as this class delegates
requests to the component extension MediaRouter, the component extension
MediaRouter sends responses back to this class via the MediaRouterObserver API.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:157: // extension becomes
suspended or if there is a connection error.
On 2015/05/22 00:05:38, Wez wrote:
> So this is basically the API through which we send requests to the Media Route
> Provider, in which case I still don't understand the MediaRouter naming
> convention. :(
> 

We should really get rid of any references to the Provider Manager here. All
this class knows is that it is talking to a component extension implementation
of the Media Router API via mojo.

[Wez, 2015-05-23 00:45:47]

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:245: // is not inside the
ObserverList iteration.
On 2015/05/22 00:54:07, imcheng wrote:
> On 2015/05/22 00:05:38, Wez wrote:
> > That in turn relies on the observers not unregistering themselves from
within
> > observer callbacks - that's something you need to document on the relevant
> APIs,
> > not here.
> 
> Since Unregister*Observer are private APIs only used by the base observer
> classes, it should be impossible for that to happen.

Surely a MediaSinksObserver-derived class could "delete |this|" from within
OnsinksReceived(), and violate that assumption, though?

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:38: class
MediaRouterMojoImpl : public MediaRouter,
On 2015/05/22 00:54:07, imcheng wrote:
> On 2015/05/22 00:05:38, Wez wrote:
> > Sorry, I meant to mention this fundamental question earlier...
> > 
> > Why is MediaRouterMojoImpl a MediaRouter, rather than _containing_ a
> MediaRouter
> > instance? It seems that the latter gives the right separation between the
> roles
> > - this class is really only intended to expose MediaRouter functions to the
> > component extension via Mojo, right?
> 
> I think "really only intended to expose MediaRouter functions to the
> component extension via Mojo" is correct. However, the callers of Media Router
> within Chrome (PresentationServiceDelegate, WebUI) does not know/care about
this
> detail. All they know is that there is a MediaRouter in Chrome that they talk
> to. Which is why this implements MediaRouter.

Right - thinking about this last night, I realised that I've got myself confused
about what this class is - I'd seen it as a component that sits alongside a core
MediaRouter impl, to expose that to the extension. But it actually is _the_
MediaRouter in Chrome, and all it really does is adapt between the C++ Media
Router interface and delegate calls on to the extension - and inside the
extension the Media Route Provider Manager presents an MRPM interface to the
MRPs, and the MR interface to this class, hence the naming.

So... we need to make sure that the comments in this header accurately describe
the role this class plays - the most fundamental thing is for the comment on the
class to describe the fact that it delegates the MR API calls to an extension
and copes with suspend/wakeup of that extension.

With regard to the MediaRouter C++ interface vs the Mojo MediaRouter interface,
it's really strange and confusing to have them named the same thing - it begs
the question of why you don't just replace the C++ MediaRouter with the Mojo
version - aren't they basically the same thing? If they are not then is that
because the Mojo version is really an internal implementation detail of the
MediaRouterMojoImpl <-> Media Route Provider extension? If so then consider
renaming the Mojo MediaRouter to e.g. MediaRouteProvider, so that the
distinction is clearer?

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:106: void
Bind(mojo::InterfaceRequest<interfaces::MediaRouterObserver> request);
On 2015/05/22 00:54:07, imcheng wrote:
> On 2015/05/22 00:05:39, Wez wrote:
> > I think you're right that there's no real value in breaking this and
> > InitExtensionInfo out of BindToRequest - you basically just want a
non-static
> > equivalent of BindToRequest on this class, for BindToRequest to call.
> > 
> > Or, since BindToRequest is a member of this class, it could reasonably set
the
> > fields in the Impl directly, removing the need for a non-static helper.
> 
> When I tried to change it I realized why it was structured this way. It's
> because there is test setup code that only calls Bind() but set extension info
> via other means, and then there are unit tests that call them both but where
> using the static method isn't an option. So the cleanest way would be to have
> these 2 member functions separately.

That's fine, but in that case you need to make it clear in the comment why it is
that they can't be folded together. And ideally you should have the API that
tests call be a public method with a *ForTest name, so that pre-submit will
catch if it's (ab)used elsewhere.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:111: // instance and
extension are keyed on.
On 2015/05/22 00:54:07, imcheng wrote:
> On 2015/05/22 00:05:38, Wez wrote:
> > Does "keyed on" mean "belong to", in effect?
> 
> Yes

OK - in that case I'd suggest using "belong to", since that conveys the object
ownership implicitly.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:149: // This binding exists
so that MediaRouterObserver calls from the
On 2015/05/22 00:54:07, imcheng wrote:
> On 2015/05/22 00:05:38, Wez wrote:
> > That is fairly intuitive from the class names; the question is what it
> achieves
> > - what _are_ MediaRouterObserver calls? Is MediaRouterObserver the API the
> > extension uses to watch for requests from MediaRouter, for example?
> > 
> > I'd expect a comment like:
> > "Binds |this| to service Mojo requests for <some sort of purpose> from the
> > component extension."
> 
> Yes. The component extension is a MediaRouter. Just as this class delegates
> requests to the component extension MediaRouter, the component extension
> MediaRouter sends responses back to this class via the MediaRouterObserver
API.

See above re the class level comment :)

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:157: // extension becomes
suspended or if there is a connection error.
On 2015/05/22 00:54:07, imcheng wrote:
> On 2015/05/22 00:05:38, Wez wrote:
> > So this is basically the API through which we send requests to the Media
Route
> > Provider, in which case I still don't understand the MediaRouter naming
> > convention. :(
> > 
> 
> We should really get rid of any references to the Provider Manager here. All
> this class knows is that it is talking to a component extension implementation
> of the Media Router API via mojo.

Acknowledged.

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:225: // Alternatively we
could implement caching in this class and returned
s/returned/return the

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:49: // Also stores a
reference to the extension's ProcessManager using
Let's re-word this to make clear what "binding to a request" actually means -
forget about the Mojo internals; what is it that this actually handles, e.g. is
it the handler for when the extension requests the MediaRouterObserver
interface, through which to call back to the MediaRouterMojoImpl?

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:64: //
interfaces::MediaRouterObserver implementation.
So IIUC this is the implementation of the Mojo interface from extension->MR, so
it's not intended to be called directly by C++ code - I'd therefore move it to
the protected or private section. Same for mojo::ErrorHandler. That will make it
clearer that this object's public interface is MediaRouter.

[imcheng (use chromium acct), 2015-05-26 17:58:32]

PTAL. I would like to add an external reviewer if it looks good to you.

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:245: // is not inside the
ObserverList iteration.
On 2015/05/23 00:45:46, Wez wrote:
> On 2015/05/22 00:54:07, imcheng wrote:
> > On 2015/05/22 00:05:38, Wez wrote:
> > > That in turn relies on the observers not unregistering themselves from
> within
> > > observer callbacks - that's something you need to document on the relevant
> > APIs,
> > > not here.
> > 
> > Since Unregister*Observer are private APIs only used by the base observer
> > classes, it should be impossible for that to happen.
> 
> Surely a MediaSinksObserver-derived class could "delete |this|" from within
> OnsinksReceived(), and violate that assumption, though?

You're right. I will add a contract in Media*Observer base classes that
operations that modify the sink list, such as invoking the destructor (which
invokes Unregister*Observer), are not allowed.

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

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:38: class
MediaRouterMojoImpl : public MediaRouter,
On 2015/05/23 00:45:46, Wez wrote:
> On 2015/05/22 00:54:07, imcheng wrote:
> > On 2015/05/22 00:05:38, Wez wrote:
> > > Sorry, I meant to mention this fundamental question earlier...
> > > 
> > > Why is MediaRouterMojoImpl a MediaRouter, rather than _containing_ a
> > MediaRouter
> > > instance? It seems that the latter gives the right separation between the
> > roles
> > > - this class is really only intended to expose MediaRouter functions to
the
> > > component extension via Mojo, right?
> > 
> > I think "really only intended to expose MediaRouter functions to the
> > component extension via Mojo" is correct. However, the callers of Media
Router
> > within Chrome (PresentationServiceDelegate, WebUI) does not know/care about
> this
> > detail. All they know is that there is a MediaRouter in Chrome that they
talk
> > to. Which is why this implements MediaRouter.
> 
> Right - thinking about this last night, I realised that I've got myself
confused
> about what this class is - I'd seen it as a component that sits alongside a
core
> MediaRouter impl, to expose that to the extension. But it actually is _the_
> MediaRouter in Chrome, and all it really does is adapt between the C++ Media
> Router interface and delegate calls on to the extension - and inside the
> extension the Media Route Provider Manager presents an MRPM interface to the
> MRPs, and the MR interface to this class, hence the naming.
> 
> So... we need to make sure that the comments in this header accurately
describe
> the role this class plays - the most fundamental thing is for the comment on
the
> class to describe the fact that it delegates the MR API calls to an extension
> and copes with suspend/wakeup of that extension.
> 
> With regard to the MediaRouter C++ interface vs the Mojo MediaRouter
interface,
> it's really strange and confusing to have them named the same thing - it begs
> the question of why you don't just replace the C++ MediaRouter with the Mojo
> version - aren't they basically the same thing? If they are not then is that
> because the Mojo version is really an internal implementation detail of the
> MediaRouterMojoImpl <-> Media Route Provider extension? If so then consider
> renaming the Mojo MediaRouter to e.g. MediaRouteProvider, so that the
> distinction is clearer?

ok, I changed the class level comments as you suggested. Re naming: I think in a
way it makes sense because the two (C++ and mojom) provide almost identical
interfaces. The way it stands now, the component extension is an implementation
of Media Router, and MediaRouterMojoImpl is a delegating / adaptation layer for
component extension wakeup/suspension. I would suggest we revisit the naming
issue when Kevin comes back. He had a long discussion with xhwang@ in his last
patch and will be able to provide some insight as to why they settled with this
name.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:106: void
Bind(mojo::InterfaceRequest<interfaces::MediaRouterObserver> request);
On 2015/05/23 00:45:46, Wez wrote:
> On 2015/05/22 00:54:07, imcheng wrote:
> > On 2015/05/22 00:05:39, Wez wrote:
> > > I think you're right that there's no real value in breaking this and
> > > InitExtensionInfo out of BindToRequest - you basically just want a
> non-static
> > > equivalent of BindToRequest on this class, for BindToRequest to call.
> > > 
> > > Or, since BindToRequest is a member of this class, it could reasonably set
> the
> > > fields in the Impl directly, removing the need for a non-static helper.
> > 
> > When I tried to change it I realized why it was structured this way. It's
> > because there is test setup code that only calls Bind() but set extension
info
> > via other means, and then there are unit tests that call them both but where
> > using the static method isn't an option. So the cleanest way would be to
have
> > these 2 member functions separately.
> 
> That's fine, but in that case you need to make it clear in the comment why it
is
> that they can't be folded together. And ideally you should have the API that
> tests call be a public method with a *ForTest name, so that pre-submit will
> catch if it's (ab)used elsewhere.

Both are used in production code, so *ForTest isn't really an option. But I
updated comments.

https://codereview.chromium.org/1143603004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:111: // instance and
extension are keyed on.
On 2015/05/23 00:45:46, Wez wrote:
> On 2015/05/22 00:54:07, imcheng wrote:
> > On 2015/05/22 00:05:38, Wez wrote:
> > > Does "keyed on" mean "belong to", in effect?
> > 
> > Yes
> 
> OK - in that case I'd suggest using "belong to", since that conveys the object
> ownership implicitly.

Done.

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:225: // Alternatively we
could implement caching in this class and returned
On 2015/05/23 00:45:47, Wez wrote:
> s/returned/return the

Done.

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:49: // Also stores a
reference to the extension's ProcessManager using
On 2015/05/23 00:45:47, Wez wrote:
> Let's re-word this to make clear what "binding to a request" actually means -
> forget about the Mojo internals; what is it that this actually handles, e.g.
is
> it the handler for when the extension requests the MediaRouterObserver
> interface, through which to call back to the MediaRouterMojoImpl?

Done.

https://codereview.chromium.org/1143603004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:64: //
interfaces::MediaRouterObserver implementation.
On 2015/05/23 00:45:47, Wez wrote:
> So IIUC this is the implementation of the Mojo interface from extension->MR,
so
> it's not intended to be called directly by C++ code - I'd therefore move it to
> the protected or private section. Same for mojo::ErrorHandler. That will make
it
> clearer that this object's public interface is MediaRouter.

Done.

[mark a. foltz, 2015-05-26 20:14:28]

mfoltz@chromium.org changed reviewers:
+ mfoltz@chromium.org

[mark a. foltz, 2015-05-26 20:14:29]

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:36: source_set("keyed_service_factories") {
Why do we need a distinct library here?

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:59: ":core",
It looks like a dependency on :router should be okay here.  Is there a cycle
that prevents it?

[imcheng (use chromium acct), 2015-05-26 20:54:03]

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:36: source_set("keyed_service_factories") {
On 2015/05/26 20:14:29, mark a. foltz wrote:
> Why do we need a distinct library here?

Last time I tried to put everything into one target resulted in obscure link
failures when building unit_tests. Which I suspected was due to dependency on
"//components/keyed_service/content". I will try to remove that dependency while
combining the targets and see if it still builds in gyp/gn.

See also: https://codereview.chromium.org/1076973002/

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:59: ":core",
On 2015/05/26 20:14:29, mark a. foltz wrote:
> It looks like a dependency on :router should be okay here.  Is there a cycle
> that prevents it?

ditto

[mark a. foltz, 2015-05-26 22:19:57]

On 2015/05/26 20:54:03, imcheng wrote:
>
https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
> File chrome/browser/media/router/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
> chrome/browser/media/router/BUILD.gn:36: source_set("keyed_service_factories")
{
> On 2015/05/26 20:14:29, mark a. foltz wrote:
> > Why do we need a distinct library here?
> 
> Last time I tried to put everything into one target resulted in obscure link
> failures when building unit_tests. Which I suspected was due to dependency on
> "//components/keyed_service/content". I will try to remove that dependency
while
> combining the targets and see if it still builds in gyp/gn.
> 
> See also: https://codereview.chromium.org/1076973002/
> 
>
https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
> chrome/browser/media/router/BUILD.gn:59: ":core",
> On 2015/05/26 20:14:29, mark a. foltz wrote:
> > It looks like a dependency on :router should be okay here.  Is there a cycle
> > that prevents it?
> 
> ditto

https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...
looks pretty straightforward to me.  Maybe there is an undeclared dependency in
unit_tests that gets exposed?

[Wez, 2015-05-26 23:41:31]

LGTM once these comment nits are addressed.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:226: // cached result.
nit: I'd suggest removing the "Alternatively..." and just filing a bug for that
change - leaving the comment here doesn't really help the reader.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:94: // from this method,
but they are separated into two methods for testability.
nit: testability->testing

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:98: // Stores the
|extension_id| of the component extension in this instance.
nit: "in this instance" doesn't help here.

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

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:24: // This function is
invoked when the list of routes and their associated
nit: No need for "This function is"; can start with "Invoked when.." here.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:26: // locally or remotely.
nit: "Routes included in the list..." - not clear what this comment actually
means? Is it relevant to the caller's behaviour, or can we remove it?

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:27: // Implementations may
not perform operations that modifies the Media Router's
s/modifies/modify

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:28: // observer list. In
particular, invoking this instance's destructor within
nit: s/instance/observer

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:29: // this method will
result in undefined behavior.
nit: s/this method/OnRoutesUpdated

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

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:33: // this method will
result in undefined behavior.
See comments on wording in the OnRoutesChanged

[imcheng (use chromium acct), 2015-05-27 00:49:31]

Patchset #7 (id:160001) has been deleted

[imcheng (use chromium acct), 2015-05-27 01:06:04]

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:36: source_set("keyed_service_factories") {
On 2015/05/26 20:54:03, imcheng wrote:
> On 2015/05/26 20:14:29, mark a. foltz wrote:
> > Why do we need a distinct library here?
> 
> Last time I tried to put everything into one target resulted in obscure link
> failures when building unit_tests. Which I suspected was due to dependency on
> "//components/keyed_service/content". I will try to remove that dependency
while
> combining the targets and see if it still builds in gyp/gn.
> 
> See also: https://codereview.chromium.org/1076973002/

So I recombined the targets and it seems to be working according to trybots,
including the gn_dbg bots which failed on the original attempt. I am stumped as
to why that is. Looking at the logs again, it could have been due to the
extraneous unit_tests_main target that used to be in BUILD.gn.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:226: // cached result.
On 2015/05/26 23:41:30, Wez wrote:
> nit: I'd suggest removing the "Alternatively..." and just filing a bug for
that
> change - leaving the comment here doesn't really help the reader.

Done.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:94: // from this method,
but they are separated into two methods for testability.
On 2015/05/26 23:41:30, Wez wrote:
> nit: testability->testing

Done.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:98: // Stores the
|extension_id| of the component extension in this instance.
On 2015/05/26 23:41:30, Wez wrote:
> nit: "in this instance" doesn't help here.

Done.

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

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:24: // This function is
invoked when the list of routes and their associated
On 2015/05/26 23:41:30, Wez wrote:
> nit: No need for "This function is"; can start with "Invoked when.." here.

Done.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:26: // locally or remotely.
On 2015/05/26 23:41:30, Wez wrote:
> nit: "Routes included in the list..." - not clear what this comment actually
> means? Is it relevant to the caller's behaviour, or can we remove it?

Done.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:27: // Implementations may
not perform operations that modifies the Media Router's
On 2015/05/26 23:41:30, Wez wrote:
> s/modifies/modify

Done.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:28: // observer list. In
particular, invoking this instance's destructor within
On 2015/05/26 23:41:30, Wez wrote:
> nit: s/instance/observer

Done.

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_routes_observer.h:29: // this method will
result in undefined behavior.
On 2015/05/26 23:41:30, Wez wrote:
> nit: s/this method/OnRoutesUpdated

Done.

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

https://codereview.chromium.org/1143603004/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/media_sinks_observer.h:33: // this method will
result in undefined behavior.
On 2015/05/26 23:41:31, Wez wrote:
> See comments on wording in the OnRoutesChanged

Done.

[imcheng (use chromium acct), 2015-05-27 18:26:18]

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

[imcheng (use chromium acct), 2015-05-27 18:26:19]

+xhwang for review

[xhwang, 2015-05-28 06:53:22]

This is my first round of comments. Mostly nits.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:20: "//components/keyed_service/content",
order?

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

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:41: // Requests a media route from
|source| to |sink_id|.
s/Requests/Creates

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:44: const MediaSinkId& sink_id,
nit: Since you already use MediaSourceId and MediaSinkId, use either source and
sink, or source_id and sink_id.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:38:
callback.Run(make_scoped_ptr(new MediaRoute(media_route.To<MediaRoute>())),
You can have a converter converting interfaces::MediaRoutePtr directly into a
scoped_ptr<MediaRoute>, e.g.

https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:56: const std::string&
mrpm_extension_id,
In the header file this is called mojo_media_router_ext_id.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:62:
mojo_media_router_extension_id_ = mrpm_extension_id;
ditto about naming.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:108: }
OOC, will |this| get destructed upon connection error? It seems |this| is owned
by the |context|. When will |context| destroy |this|?

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:352:
DVLOG_WITH_INSTANCE(1) << "Extension is awake, awaiting MRPM connection.";
MRPM is not seen in this file; update the comment?

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:53: static void
BindToRequest(
nit: I found it more common to call this static method Create() or CreateXxx():

https://code.google.com/p/chromium/codesearch#search/&q=Create.*InterfaceRequ...

This method's main task is to create a service, which will bind itself to the
request....

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:89:
extensions::EventPageTracker* event_page_tracker_for_test);
Can we have separate calls like set_mojo_media_router_ext_id_for_testing()
instead of having an overloaded ctor? *for_testing methods will be guaranteed
not to be used in prod.

With that, do we still need the InitExtensionInfo() and BindToMojoRequest()
separation?

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_factory.cc:50: //
BrowserContext.
I don't understand this comment. It seems we always just return the input
|context|?

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:43: const char
kSinkName[] = "sinkName";
nit: move these into the media_router namespace?

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:308:
interfaces::MediaRouterObserverPtr media_router_proxy;
Can we be consistent about naming mojo::InterfacePtr<T> variables? I saw cases
where we call it "proxy", but I don't feel it's common now. This becomes even
more weird when you call GetProxy(&proxy)...

Also, media_router_proxy doesn't make sense. It should at least have "observer"
in the name.

How about just calling it media_router_observer_ptr, or
mojo_media_router_observer similar to |mojo_media_router_| you had above.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:311:
testing::StrictMock<MockMojoMediaRouterService> mrpm;
throughout this CL, we have mrpm in many places. Is this just old code that
needs to be updated, or we actually still have mrpm? Nevertheless, this naming
doesn't quite fit here.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:312:
interfaces::MediaRouterPtr mrpm_proxy;
ditto about mrpm and proxy.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_test.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.cc:36:
interfaces::MediaRouterPtr mrpm_host_proxy;
ditto about mrpm and proxy

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_test.h (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:28: // api_impl_ ->
mrpm_host_proxy_ -> mock_mrpm_host_
Do we still have the concept of mrpm? Seems like we need to update it to use the
new names.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:50:
MockMojoMediaRouterService mock_mrpm_host_;
ditto

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:55:
interfaces::MediaRouterPtr mrpm_host_proxy_;
ditto

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:57:
scoped_ptr<mojo::Binding<interfaces::MediaRouter>> binding;
binding_

[imcheng (use chromium acct), 2015-05-28 20:46:36]

Thanks for the quick review!

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/BUILD.gn:20: "//components/keyed_service/content",
On 2015/05/28 06:53:20, xhwang wrote:
> order?

Done.

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

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:41: // Requests a media route from
|source| to |sink_id|.
On 2015/05/28 06:53:20, xhwang wrote:
> s/Requests/Creates

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:44: const MediaSinkId& sink_id,
On 2015/05/28 06:53:20, xhwang wrote:
> nit: Since you already use MediaSourceId and MediaSinkId, use either source
and
> sink, or source_id and sink_id.

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:38:
callback.Run(make_scoped_ptr(new MediaRoute(media_route.To<MediaRoute>())),
On 2015/05/28 06:53:21, xhwang wrote:
> You can have a converter converting interfaces::MediaRoutePtr directly into a
> scoped_ptr<MediaRoute>, e.g.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:56: const std::string&
mrpm_extension_id,
On 2015/05/28 06:53:21, xhwang wrote:
> In the header file this is called mojo_media_router_ext_id.

Removed.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:62:
mojo_media_router_extension_id_ = mrpm_extension_id;
On 2015/05/28 06:53:20, xhwang wrote:
> ditto about naming.

Acknowledged.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:108: }
On 2015/05/28 06:53:21, xhwang wrote:
> OOC, will |this| get destructed upon connection error? It seems |this| is
owned
> by the |context|. When will |context| destroy |this|?

|this| will not be destroyed upon connection error because it is not a strong
binding. |this| will be destroyed when |context| is being destroyed.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:352:
DVLOG_WITH_INSTANCE(1) << "Extension is awake, awaiting MRPM connection.";
On 2015/05/28 06:53:20, xhwang wrote:
> MRPM is not seen in this file; update the comment?

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:53: static void
BindToRequest(
On 2015/05/28 06:53:21, xhwang wrote:
> nit: I found it more common to call this static method Create() or
CreateXxx():
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=Create.*InterfaceRequ...
> 
> This method's main task is to create a service, which will bind itself to the
> request....

It doesn't always create MediaRouterMojoImpl. It can be created via other means
(e.g., if WebUI requested it first), or this method can be called multiple times
for the same instance (e.g., to re-bind when extension wakes up). I clarified
the comment that it creates an instance if it does not already exist.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:89:
extensions::EventPageTracker* event_page_tracker_for_test);
On 2015/05/28 06:53:21, xhwang wrote:
> Can we have separate calls like set_mojo_media_router_ext_id_for_testing()
> instead of having an overloaded ctor? *for_testing methods will be guaranteed
> not to be used in prod.
> 
> With that, do we still need the InitExtensionInfo() and BindToMojoRequest()
> separation?

Ok, I got rid of the ctor. And then combined InitExtensionInfo() and
BindToMojoRequest(). And added a ForTest version, for tests that provide their
own EventPageTracker* directly.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_factory.cc:50: //
BrowserContext.
On 2015/05/28 06:53:21, xhwang wrote:
> I don't understand this comment. It seems we always just return the input
> |context|?

I clarified the comments a bit. Let me know if it sounds better?

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:43: const char
kSinkName[] = "sinkName";
On 2015/05/28 06:53:21, xhwang wrote:
> nit: move these into the media_router namespace?

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:308:
interfaces::MediaRouterObserverPtr media_router_proxy;
On 2015/05/28 06:53:21, xhwang wrote:
> Can we be consistent about naming mojo::InterfacePtr<T> variables? I saw cases
> where we call it "proxy", but I don't feel it's common now. This becomes even
> more weird when you call GetProxy(&proxy)...
> 
> Also, media_router_proxy doesn't make sense. It should at least have
"observer"
> in the name.
> 
> How about just calling it media_router_observer_ptr, or
> mojo_media_router_observer similar to |mojo_media_router_| you had above. 

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:311:
testing::StrictMock<MockMojoMediaRouterService> mrpm;
On 2015/05/28 06:53:21, xhwang wrote:
> throughout this CL, we have mrpm in many places. Is this just old code that
> needs to be updated, or we actually still have mrpm? Nevertheless, this naming
> doesn't quite fit here.

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:312:
interfaces::MediaRouterPtr mrpm_proxy;
On 2015/05/28 06:53:21, xhwang wrote:
> ditto about mrpm and proxy.

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_test.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.cc:36:
interfaces::MediaRouterPtr mrpm_host_proxy;
On 2015/05/28 06:53:21, xhwang wrote:
> ditto about mrpm and proxy

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_test.h (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:28: // api_impl_ ->
mrpm_host_proxy_ -> mock_mrpm_host_
On 2015/05/28 06:53:21, xhwang wrote:
> Do we still have the concept of mrpm? Seems like we need to update it to use
the
> new names.

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:50:
MockMojoMediaRouterService mock_mrpm_host_;
On 2015/05/28 06:53:21, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:55:
interfaces::MediaRouterPtr mrpm_host_proxy_;
On 2015/05/28 06:53:21, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_test.h:57:
scoped_ptr<mojo::Binding<interfaces::MediaRouter>> binding;
On 2015/05/28 06:53:21, xhwang wrote:
> binding_

Done. Also cleaned this file up a bit regarding protected vs private.

[xhwang, 2015-05-28 22:08:09]

Thanks! LGTM!

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:53: static void
BindToRequest(
On 2015/05/28 20:46:35, imcheng wrote:
> On 2015/05/28 06:53:21, xhwang wrote:
> > nit: I found it more common to call this static method Create() or
> CreateXxx():
> > 
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=Create.*InterfaceRequ...
> > 
> > This method's main task is to create a service, which will bind itself to
the
> > request....
> 
> It doesn't always create MediaRouterMojoImpl. It can be created via other
means
> (e.g., if WebUI requested it first), or this method can be called multiple
times
> for the same instance (e.g., to re-bind when extension wakes up). I clarified
> the comment that it creates an instance if it does not already exist.

Acknowledged.

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_factory.cc (right):

https://codereview.chromium.org/1143603004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_factory.cc:50: //
BrowserContext.
On 2015/05/28 20:46:35, imcheng wrote:
> On 2015/05/28 06:53:21, xhwang wrote:
> > I don't understand this comment. It seems we always just return the input
> > |context|?
> 
> I clarified the comments a bit. Let me know if it sounds better?

Acknowledged.

[imcheng (use chromium acct), 2015-05-28 22:18:12]

The CQ bit was checked by imcheng@google.com

[imcheng (use chromium acct), 2015-05-28 22:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1143603004/#ps210001
(title: "Addressed Xiaohan's comments")

[commit-bot: I haz the power, 2015-05-28 22:19:47]

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

[commit-bot: I haz the power, 2015-05-28 22:26:02]

Committed patchset #9 (id:210001)

[commit-bot: I haz the power, 2015-05-28 22:26:47]

Patchset 9 (id:??) landed as
https://crrev.com/7f1dfe3b436ae1df300867f7dab3c8e0569ed95d
Cr-Commit-Position: refs/heads/master@{#331868}

[dmurph, 2015-05-29 03:04:09]

On 2015/05/28 at 22:26:47, commit-bot wrote:
> Patchset 9 (id:??) landed as
https://crrev.com/7f1dfe3b436ae1df300867f7dab3c8e0569ed95d
> Cr-Commit-Position: refs/heads/master@{#331868}

Note: possible bot flakiness or failures from this:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...

[dmurph, 2015-05-29 03:13:10]

On 2015/05/29 at 03:04:09, dmurph wrote:
> On 2015/05/28 at 22:26:47, commit-bot wrote:
> > Patchset 9 (id:??) landed as
https://crrev.com/7f1dfe3b436ae1df300867f7dab3c8e0569ed95d
> > Cr-Commit-Position: refs/heads/master@{#331868}
> 
> Note: possible bot flakiness or failures from this:
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...

Nvm, found culprit https://code.google.com/p/chromium/issues/detail?id=493525