MediaRouterAction: Only observe Media Routes when there is a local route.

MediaRouterAction: Only observe Media Routes when there is a local route.

Currently, MediaRouterAction inherits MediaRoutesObserver, which means it maintains a routes query to the Media Router component extension for the entire lifetime.

Since MediaRouterAction only cares about local routes, this change adds a LocalMediaRoutesObserver for when there are any local routes. The MediaRouterAction will only register a MediaRoutesObserver if there is a local route since it wants updates on whether or not the local route is still active.

The Action's MediaRoutesObserver will be unregistered when there are no more local routes existing or at the end of the Action's lifetime.

BUG=538209
Committed: https://crrev.com/a6617b6c31f7013199edbc3ca1e82d920eeab722
Cr-Commit-Position: refs/heads/master@{#352510}

[apacible, 2015-10-01 01:08:34]

Patchset #1 (id:1) has been deleted

[apacible, 2015-10-01 01:27:50]

Patchset #1 (id:20001) has been deleted

[apacible, 2015-10-01 04:41:28]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-10-01 16:37:19]

apacible@chromium.org changed reviewers:
+ avayvod@chromium.org, imcheng@chromium.org, pkasting@chromium.org

[apacible, 2015-10-01 16:38:18]

PTAL, thanks!

+avayvod for chrome/browser/media/android/router/*
+imcheng for chrome/browser/media/router/*
+pkasting for chrome/browser/ui/toolbar/*

[imcheng, 2015-10-01 18:46:50]

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/local_media_routes_observer.h (right):

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/local_media_routes_observer.h:10: // #include
"base/macros.h"
remove these includes? they don't appear to be used.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/local_media_routes_observer.h:27: MediaRouter*
router_;
MediaRouter* const router_;

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:47:
action_->SetHasLocalRoute(has_local_route);
there should be a comment that |this| will be deleted beyond this point if
has_local_route is false.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:67: routes_observer_(new
ActionMediaRoutesObserver(
So we are still sending a routes query on construction. I want to see if we can
delay that until we know there is a local route. In fact, we can probably only
depend on LocalMediaRoutesObserver in this class. We may have to add an
HasLocalRoutes() to MediaRouter to determine the initial state.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:78: routes_observer_.reset();
not needed?

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:228: media_router::MediaRouter*
MediaRouterAction::GetMediaRouter(Browser* browser) {
Can this be a non-member function in anonymous namespace?

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:230:
static_cast<content::BrowserContext*>(browser->profile()));
Is the static_cast necessary?

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:83: MediaRouterAction* action_;
 MediaRouterAction* const action_;

[apacible, 2015-10-01 23:21:47]

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/local_media_routes_observer.h (right):

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/local_media_routes_observer.h:10: // #include
"base/macros.h"
On 2015/10/01 18:46:50, imcheng1 wrote:
> remove these includes? they don't appear to be used.

Done.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/local_media_routes_observer.h:27: MediaRouter*
router_;
On 2015/10/01 18:46:49, imcheng1 wrote:
> MediaRouter* const router_;

Done.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:47:
action_->SetHasLocalRoute(has_local_route);
On 2015/10/01 18:46:50, imcheng1 wrote:
> there should be a comment that |this| will be deleted beyond this point if
> has_local_route is false.

Done (after moving this to MediaRouterMojoImpl).

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:67: routes_observer_(new
ActionMediaRoutesObserver(
On 2015/10/01 18:46:50, imcheng1 wrote:
> So we are still sending a routes query on construction. I want to see if we
can
> delay that until we know there is a local route. In fact, we can probably only
> depend on LocalMediaRoutesObserver in this class. We may have to add an
> HasLocalRoutes() to MediaRouter to determine the initial state.

Talked offline. Done.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:78: routes_observer_.reset();
On 2015/10/01 18:46:50, imcheng1 wrote:
> not needed?

Done.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:228: media_router::MediaRouter*
MediaRouterAction::GetMediaRouter(Browser* browser) {
On 2015/10/01 18:46:50, imcheng1 wrote:
> Can this be a non-member function in anonymous namespace?

Done.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:230:
static_cast<content::BrowserContext*>(browser->profile()));
On 2015/10/01 18:46:50, imcheng1 wrote:
> Is the static_cast necessary?

Removed.

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/1383653002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:83: MediaRouterAction* action_;
On 2015/10/01 18:46:50, imcheng1 wrote:
>  MediaRouterAction* const action_;

Done (moved to MediaRouterMojoImpl).

[apacible, 2015-10-01 23:33:07]

Patchset #2 (id:80001) has been deleted

[imcheng, 2015-10-02 17:21:11]

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:54: bool
GetHasLocalRoute() override;
naming: Why not just HasLocalRoute() ? Also, let's mark this method as const for
now.

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/local_media_routes_observer.h:14: // Base class for
observing when a new route has been created locally.
Please update comments.

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/local_media_routes_observer.h:22: virtual void
UpdateHasLocalRoute(bool has_local_route) {}
naming: The method name is a little strange. Usually, observer will have method
names like OnXYZ().

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:119: virtual bool GetHasLocalRoute()
= 0;
Please add comments.

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:223: has_local_route_ =
has_local_route;
Can we skip the logic below if has_local_route_ == has_local_route?

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:110: class
MediaRouterMediaRoutesObserver :
Consider moving this into an anonymous namespace in .cc.

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:151: void
MediaRouterAction::UpdateHasLocalRoute(bool has_local_route) {
It seems to me that this method is not needed. Callers of this method can just
call SetHasLocalRoute.

[apacible, 2015-10-02 18:47:42]

Patchset #4 (id:140001) has been deleted

[apacible, 2015-10-02 18:48:20]

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.h:54: bool
GetHasLocalRoute() override;
On 2015/10/02 17:21:11, imcheng1 wrote:
> naming: Why not just HasLocalRoute() ? Also, let's mark this method as const
for
> now.

Done.

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/local_media_routes_observer.h:14: // Base class for
observing when a new route has been created locally.
On 2015/10/02 17:21:11, imcheng1 wrote:
> Please update comments.

Done.

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/local_media_routes_observer.h:22: virtual void
UpdateHasLocalRoute(bool has_local_route) {}
On 2015/10/02 17:21:11, imcheng1 wrote:
> naming: The method name is a little strange. Usually, observer will have
method
> names like OnXYZ().

Done.

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router.h:119: virtual bool GetHasLocalRoute()
= 0;
On 2015/10/02 17:21:11, imcheng1 wrote:
> Please add comments.

Done.

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:223: has_local_route_ =
has_local_route;
On 2015/10/02 17:21:11, imcheng1 wrote:
> Can we skip the logic below if has_local_route_ == has_local_route?

Done.

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

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:110: class
MediaRouterMediaRoutesObserver :
On 2015/10/02 17:21:11, imcheng1 wrote:
> Consider moving this into an anonymous namespace in .cc.

Done.

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/100001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:151: void
MediaRouterAction::UpdateHasLocalRoute(bool has_local_route) {
On 2015/10/02 17:21:11, imcheng1 wrote:
> It seems to me that this method is not needed. Callers of this method can just
> call SetHasLocalRoute.

UpdateHasLocalRoute (now OnHasLocalRouteUpdated) is for the
LocalMediaRoutesObserver, so I removed SetHasLocalRoute instead.

[Peter Kasting, 2015-10-02 19:04:01]

LGTM

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:32: browser->profile());
This is very simple and only used in one place, so just inline it there.

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:194: // local routes.
Nit: Grammar; should this be "state updates based on whether"?

[imcheng, 2015-10-02 19:54:43]

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

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:218:
FOR_EACH_OBSERVER(LocalMediaRoutesObserver, local_routes_observers_,
Replace this with UpdateHasLocalRoute(true)

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:429: void
MediaRouterMojoImpl::RegisterLocalMediaRoutesObserver(
we should add a test for this. Maybe in a separate patch.

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:585:
OnHasLocalRouteUpdated(has_local_route_));
you would need to have updated has_local_route_ before you use it here.

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

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:97: void
UpdateHasLocalRoute(bool has_local_route);
ok, I see now why you had the MediaRoutesObserver implementation as a private
inner class. In this case it would be the better choice because then we won't
have to make this method public. Sorry about that.

[apacible, 2015-10-02 20:42:52]

Patchset #5 (id:180001) has been deleted

[apacible, 2015-10-02 20:45:48]

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

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:218:
FOR_EACH_OBSERVER(LocalMediaRoutesObserver, local_routes_observers_,
On 2015/10/02 19:54:43, imcheng1 wrote:
> Replace this with UpdateHasLocalRoute(true)

Done.

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:429: void
MediaRouterMojoImpl::RegisterLocalMediaRoutesObserver(
On 2015/10/02 19:54:43, imcheng1 wrote:
> we should add a test for this. Maybe in a separate patch.

Acknowledged.

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:585:
OnHasLocalRouteUpdated(has_local_route_));
On 2015/10/02 19:54:43, imcheng1 wrote:
> you would need to have updated has_local_route_ before you use it here.

Done.

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

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.h:97: void
UpdateHasLocalRoute(bool has_local_route);
On 2015/10/02 19:54:43, imcheng1 wrote:
> ok, I see now why you had the MediaRoutesObserver implementation as a private
> inner class. In this case it would be the better choice because then we won't
> have to make this method public. Sorry about that.

No problem, switched back.

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:32: browser->profile());
On 2015/10/02 19:04:01, Peter Kasting wrote:
> This is very simple and only used in one place, so just inline it there.

This is used three times; I'd rather keep GetMediaRouter() to be used there
rather than inlining in all three places.

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:194: // local routes.
On 2015/10/02 19:04:01, Peter Kasting wrote:
> Nit: Grammar; should this be "state updates based on whether"?

Done.

[imcheng, 2015-10-02 20:59:40]

lgtm + 1 comment

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

https://codereview.chromium.org/1383653002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:86: // |this| will be
deleted beyond this point if |has_local_route| is false.
As discussed offline, ObserverList supports removing an observer while iterating
through it, so this should be safe. Still it might be worthwhile to add a
comment here to note the behavior, so that we will be aware of it if we were to
change our implementation in the future .

[apacible, 2015-10-02 22:30:59]

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

https://codereview.chromium.org/1383653002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl.cc:86: // |this| will be
deleted beyond this point if |has_local_route| is false.
On 2015/10/02 20:59:40, imcheng1 wrote:
> As discussed offline, ObserverList supports removing an observer while
iterating
> through it, so this should be safe. Still it might be worthwhile to add a
> comment here to note the behavior, so that we will be aware of it if we were
to
> change our implementation in the future .

Done.

[Peter Kasting, 2015-10-05 03:20:26]

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1383653002/diff/160001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:32: browser->profile());
On 2015/10/02 20:45:48, apacible wrote:
> On 2015/10/02 19:04:01, Peter Kasting wrote:
> > This is very simple and only used in one place, so just inline it there.
> 
> This is used three times; I'd rather keep GetMediaRouter() to be used there
> rather than inlining in all three places.

Oh, sorry.  Since the function looked added I had only seen the added line that
called it, I hadn't looked at existing lines for calls since I hadn't been
thinking it was already around.  Yes, keeping it this way is fine.

[whywhat, 2015-10-05 21:46:37]

android/ lgtm

[apacible, 2015-10-05 23:57:02]

Patchset #8 (id:260001) has been deleted

[apacible, 2015-10-06 01:36:50]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-10-06 01:36:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1383653002/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-06 01:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1383653002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1383653002/240001

[commit-bot: I haz the power, 2015-10-06 01:44:56]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2015-10-06 01:45:32]

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