[Media Router] Fix regression with icon not turning blue after casting.

[Media Router] Fix regression with icon not turning blue after casting.

Previously MR relies on the behavior of route response returning a
local displayable route on success. This is no longer the case, thus
the logic to turn the toolbar icon blue is no longer getting triggered.

The fix is to have the toolbar observe route list directly for local
displayable routes. Route queries have a much lower overhead than
before.

In a separate patch we will clean up the now obsolete
LocalMediaRoutesObserver API from MR and MR extension.

BUG=582278
Committed: https://crrev.com/7f297c082c6be8d8495c5b4fe22069e93ed92a73
Cr-Commit-Position: refs/heads/master@{#372750}

[imcheng, 2016-01-29 00:33:04]

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

[imcheng, 2016-01-29 00:33:05]

pkasting@, PTAL thanks!

[Peter Kasting, 2016-01-30 00:52:19]

LGTM

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

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:170:
std::find_if(routes.begin(), routes.end(),
Nit: Prefer std::any_of() to (std::find_if() != end())

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

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:59: void OnRoutesUpdated(const
std::vector<media_router::MediaRoute>& routes,
Nit: Separately, you may want to consider whether MediaRoutesObserver should
define type aliases for these param types for shorter and possibly more readable
code.

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:128: "routeId1",
fake_source1_, "sinkId1", "description", true, "", true));
Nit: "" -> std::string()

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:159:
std::vector<media_router::MediaRoute::Id> empty_route_id_list_;
Nit: The Google style guide discourages protected data members (they are banned
outside of tests, and the test exception is "for technical reasons", implying
that these should be avoided if possible).

Prefer to expose const getters for these.

[imcheng, 2016-02-01 18:47:52]

Patchset #3 (id:40001) has been deleted

[imcheng, 2016-02-01 18:48:07]

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

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:170:
std::find_if(routes.begin(), routes.end(),
On 2016/01/30 00:52:19, Peter Kasting wrote:
> Nit: Prefer std::any_of() to (std::find_if() != end())

I was going to use that, but it looks like it's a C++11 feature that's not
allowed yet.

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

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:59: void OnRoutesUpdated(const
std::vector<media_router::MediaRoute>& routes,
On 2016/01/30 00:52:19, Peter Kasting wrote:
> Nit: Separately, you may want to consider whether MediaRoutesObserver should
> define type aliases for these param types for shorter and possibly more
readable
> code.

Acknowledged.

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:128: "routeId1",
fake_source1_, "sinkId1", "description", true, "", true));
On 2016/01/30 00:52:19, Peter Kasting wrote:
> Nit: "" -> std::string()

Done.

https://codereview.chromium.org/1648713004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:159:
std::vector<media_router::MediaRoute::Id> empty_route_id_list_;
On 2016/01/30 00:52:19, Peter Kasting wrote:
> Nit: The Google style guide discourages protected data members (they are
banned
> outside of tests, and the test exception is "for technical reasons", implying
> that these should be avoided if possible).
> 
> Prefer to expose const getters for these.

Done.

[imcheng, 2016-02-01 20:35:52]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-02-01 20:35:53]

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

[commit-bot: I haz the power, 2016-02-01 20:36:37]

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

[commit-bot: I haz the power, 2016-02-01 20:44:30]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-01 20:45:22]

Description was changed from

==========
[Media Router] Fix regression with icon not turning blue after casting.

Previously MR relies on the behavior of route response returning a
local displayable route on success. This is no longer the case, thus
the logic to turn the toolbar icon blue is no longer getting triggered.

The fix is to have the toolbar observe route list directly for local
displayable routes. Route queries have a much lower overhead than
before.

In a separate patch we will clean up the now obsolete
LocalMediaRoutesObserver API from MR and MR extension.

BUG=582278
==========

to

==========
[Media Router] Fix regression with icon not turning blue after casting.

Previously MR relies on the behavior of route response returning a
local displayable route on success. This is no longer the case, thus
the logic to turn the toolbar icon blue is no longer getting triggered.

The fix is to have the toolbar observe route list directly for local
displayable routes. Route queries have a much lower overhead than
before.

In a separate patch we will clean up the now obsolete
LocalMediaRoutesObserver API from MR and MR extension.

BUG=582278

Committed: https://crrev.com/7f297c082c6be8d8495c5b4fe22069e93ed92a73
Cr-Commit-Position: refs/heads/master@{#372750}
==========

[commit-bot: I haz the power, 2016-02-01 20:45:23]

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