[Media Router] Assign each route a current cast mode if possible

[Media Router] Assign each route a current cast mode if possible

This change gives each media route in the WebUI a current cast mode
value if its media source corresponds to one of the currently available
cast modes. This is to prevent showing the 'cast' button that allows
replacing the route when the new route would effectively be a copy of
the original route.

BUG=614144
R=apacible@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a47d5b0506c1183d501767d17ffec02ca419ea76
Cr-Commit-Position: refs/heads/master@{#398687}

[btolsch, 2016-06-06 06:38:01]

Description was changed from

==========
[Media Router] Assign each route a current cast mode if possible

This change gives each media route in the WebUI a current cast mode
value if its media source corresponds to one of the currently available
cast modes. This is to prevent showing the 'cast' button that allows
replacing the route when the new route would effectively be a copy of
the original route.

BUG=614144
R=apacible@chromium.org
==========

to

==========
[Media Router] Assign each route a current cast mode if possible

This change gives each media route in the WebUI a current cast mode
value if its media source corresponds to one of the currently available
cast modes. This is to prevent showing the 'cast' button that allows
replacing the route when the new route would effectively be a copy of
the original route.

BUG=614144
R=apacible@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[btolsch, 2016-06-06 06:39:10]

I'll add javascript tests soon. PTAL and let me know what you think of the
general idea. Thanks!

[apacible, 2016-06-06 15:43:43]

On 2016/06/06 06:39:10, btolsch wrote:
> I'll add javascript tests soon. PTAL and let me know what you think of the
> general idea. Thanks!

I also noticed https://codereview.chromium.org/2040873002 on my incoming reviews
list; should I be reviewing this one (2040883002)? The differences are minor but
I want to make sure my comments are on the intended cl.

[btolsch, 2016-06-06 17:17:28]

Yes, sorry, I forgot to close that one. I noticed a minor mistake on that one
when double checking it before I sent it out.

[apacible, 2016-06-06 17:37:18]

On 2016/06/06 17:17:28, btolsch wrote:
> Yes, sorry, I forgot to close that one. I noticed a minor mistake on that one
> when double checking it before I sent it out.

Thanks! Taking a look.

[apacible, 2016-06-06 20:17:10]

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:89:
replace-route-available="[[computeReplaceRouteAvailable_(currentRoute_,
currentLaunchingSinkId_, shownCastModeValue_)]]"
nit: Can you split this into two lines? With a recent update to Polymer,
newlines/whitespace for compute bindings should now work.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:836:
computeReplaceRouteAvailable_: function(
Can we move this to <route-details>? The functionality makes more sense there.

Each of the params (and |sink|) can be bound to the <route-details> element,
then do computation there. The function also won't be loaded unless we've
stamped <route-details>.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:877:
auto current_cast_mode_entry =
Pull out lines 877-881 (and 277-281) into a helper function.

[imcheng, 2016-06-06 20:48:18]

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

[imcheng, 2016-06-06 20:48:19]

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:163: auto
available_source_map(source_cast_mode_callback_.Run());
I was wondering if we can move the computation of |current_cast_modes| to
MediaRouterUI. That way we don't have to call back to MRUI to get
|available_source_map| and there's no need for |source_cast_mode_callback_|.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:167: if (source_entry !=
end(available_source_map)) {
nit: prefer available_source_map.end().

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:279:
int current_cast_mode = current_cast_mode_entry != end(current_cast_modes)
nit: current_cast_modes.end()

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:284:
off_the_record_, current_cast_mode));
Is the current_cast_mode value on the route response used? Or is it ok to set
current_cast_mode only on routes updates?

[btolsch, 2016-06-06 23:46:53]

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:89:
replace-route-available="[[computeReplaceRouteAvailable_(currentRoute_,
currentLaunchingSinkId_, shownCastModeValue_)]]"
On 2016/06/06 20:17:10, apacible wrote:
> nit: Can you split this into two lines? With a recent update to Polymer,
> newlines/whitespace for compute bindings should now work.

Nice! The line is gone now though.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:836:
computeReplaceRouteAvailable_: function(
On 2016/06/06 20:17:10, apacible wrote:
> Can we move this to <route-details>? The functionality makes more sense there.
> 
> Each of the params (and |sink|) can be bound to the <route-details> element,
> then do computation there. The function also won't be loaded unless we've
> stamped <route-details>.

Done.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:163: auto
available_source_map(source_cast_mode_callback_.Run());
On 2016/06/06 20:48:19, imcheng wrote:
> I was wondering if we can move the computation of |current_cast_modes| to
> MediaRouterUI. That way we don't have to call back to MRUI to get
> |available_source_map| and there's no need for |source_cast_mode_callback_|.

That's where I originally put it but it would be much more difficult to test
there.  It seems like I might have to add a routes observer to InitForTest?

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:167: if (source_entry !=
end(available_source_map)) {
On 2016/06/06 20:48:19, imcheng wrote:
> nit: prefer available_source_map.end().

Done.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:279:
int current_cast_mode = current_cast_mode_entry != end(current_cast_modes)
On 2016/06/06 20:48:19, imcheng wrote:
> nit: current_cast_modes.end()

Removed.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:284:
off_the_record_, current_cast_mode));
On 2016/06/06 20:48:19, imcheng wrote:
> Is the current_cast_mode value on the route response used? Or is it ok to set
> current_cast_mode only on routes updates?

It's not used on the route response.  I'll use -1 directly here.

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:877:
auto current_cast_mode_entry =
On 2016/06/06 20:17:10, apacible wrote:
> Pull out lines 877-881 (and 277-281) into a helper function.

Done. Only this instance remains in use but still pulled it out for clarity.

[btolsch, 2016-06-07 01:32:30]

I was wrong about the create route response not being used for display (I
thought even for-display routes were updated by rebuildRouteMaps_) so I added
that back to the message handler.  I also made <route-details> only need a
boolean for whether a sink is launching instead of directly using the sink ID.

[apacible, 2016-06-07 12:13:42]

https://codereview.chromium.org/2040883002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/route_details/route_details.js
(right):

https://codereview.chromium.org/2040883002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/route_details/route_details.js:61:
sinkCurrentlyLaunching: {
nit: Rename as something like, |isAnySinkCurrentlyLaunching|.

It makes more sense with the comments in route-details but this would make it
more obvious that |sinkCurrentlyLaunching| and |sink| don't refer to the same
sink in media-router-container.

[imcheng, 2016-06-07 17:25:42]

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:163: auto
available_source_map(source_cast_mode_callback_.Run());
On 2016/06/06 23:46:52, btolsch wrote:
> On 2016/06/06 20:48:19, imcheng wrote:
> > I was wondering if we can move the computation of |current_cast_modes| to
> > MediaRouterUI. That way we don't have to call back to MRUI to get
> > |available_source_map| and there's no need for |source_cast_mode_callback_|.
> 
> That's where I originally put it but it would be much more difficult to test
> there.  It seems like I might have to add a routes observer to InitForTest?

Or you could just modify an existing test that invokes
|MediaRouterUI::OnRoutesUpdated()| and then assert on |current_cast_modes_|?

[btolsch, 2016-06-07 19:45:34]

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2040883002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:163: auto
available_source_map(source_cast_mode_callback_.Run());
On 2016/06/07 17:25:42, imcheng wrote:
> On 2016/06/06 23:46:52, btolsch wrote:
> > On 2016/06/06 20:48:19, imcheng wrote:
> > > I was wondering if we can move the computation of |current_cast_modes| to
> > > MediaRouterUI. That way we don't have to call back to MRUI to get
> > > |available_source_map| and there's no need for
|source_cast_mode_callback_|.
> > 
> > That's where I originally put it but it would be much more difficult to test
> > there.  It seems like I might have to add a routes observer to InitForTest?
> 
> Or you could just modify an existing test that invokes
> |MediaRouterUI::OnRoutesUpdated()| and then assert on |current_cast_modes_|?

Done. Modified new tests to invoke MediaRouterUI::OnRoutesUpdated and use
existing source observers.

https://codereview.chromium.org/2040883002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/route_details/route_details.js
(right):

https://codereview.chromium.org/2040883002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/route_details/route_details.js:61:
sinkCurrentlyLaunching: {
On 2016/06/07 12:13:42, apacible wrote:
> nit: Rename as something like, |isAnySinkCurrentlyLaunching|.
> 
> It makes more sense with the comments in route-details but this would make it
> more obvious that |sinkCurrentlyLaunching| and |sink| don't refer to the same
> sink in media-router-container.

Done.

[imcheng, 2016-06-08 20:00:14]

C++ code LGTM. I defer to Jennifer for HTML/JS changes.

[apacible, 2016-06-08 20:00:53]

lgtm

https://codereview.chromium.org/2040883002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/2040883002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:90:
is-any-sink-currently-launching="[[
nit: does this work if you do:

is-any-sink-currently-launching=
    "[[computeIsLaunching_(currentLaunchingSinkId_)]]"

It's a bit easier to read.

https://codereview.chromium.org/2040883002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_test_base.js
(right):

https://codereview.chromium.org/2040883002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_test_base.js:44: *
@param {elementId=} elementId Optional element id to display.
?string

[btolsch, 2016-06-08 20:11:14]

https://codereview.chromium.org/2040883002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/2040883002/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:90:
is-any-sink-currently-launching="[[
On 2016/06/08 20:00:53, apacible wrote:
> nit: does this work if you do:
> 
> is-any-sink-currently-launching=
>     "[[computeIsLaunching_(currentLaunchingSinkId_)]]"
> 
> It's a bit easier to read.

Done.

https://codereview.chromium.org/2040883002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_test_base.js
(right):

https://codereview.chromium.org/2040883002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_test_base.js:44: *
@param {elementId=} elementId Optional element id to display.
On 2016/06/08 20:00:53, apacible wrote:
> ?string

Done.

[btolsch, 2016-06-08 20:24:24]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-06-08 20:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/2040883002/#ps120001
(title: "Rebase for WebUI::CallJavascriptFunction rename")

[commit-bot: I haz the power, 2016-06-08 20:25:03]

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

[commit-bot: I haz the power, 2016-06-08 21:38:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-08 21:41:43]

Description was changed from

==========
[Media Router] Assign each route a current cast mode if possible

This change gives each media route in the WebUI a current cast mode
value if its media source corresponds to one of the currently available
cast modes. This is to prevent showing the 'cast' button that allows
replacing the route when the new route would effectively be a copy of
the original route.

BUG=614144
R=apacible@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Assign each route a current cast mode if possible

This change gives each media route in the WebUI a current cast mode
value if its media source corresponds to one of the currently available
cast modes. This is to prevent showing the 'cast' button that allows
replacing the route when the new route would effectively be a copy of
the original route.

BUG=614144
R=apacible@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a47d5b0506c1183d501767d17ffec02ca419ea76
Cr-Commit-Position: refs/heads/master@{#398687}
==========

[commit-bot: I haz the power, 2016-06-08 21:41:48]

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