Shows a launch spinner next to a sink once it is selected to create a route. The launch spinner sur…

Shows a launch spinner next to a sink once it is selected to create a route. The launch spinner survives MR UI closing and re-opening.

Once it is in launch state, a few features are disabled:
  * clicking other sink has no effect.
  * can not change cast mode.

After a launch attempt succeeds, go straight to route detail view.

Note that MediaRouterMojoImpl supports multiple simultaneous launch attempts. However, MR UI only supports one.

BUG=519667
Committed: https://crrev.com/2d652bcd6beebeaf6f867a3f0dd34103dd27ec16
Cr-Commit-Position: refs/heads/master@{#343106}

[haibinlu, 2015-08-07 23:38:01]

haibinlu@chromium.org changed reviewers:
+ apacible@chromium.org, imcheng@chromium.org, kmarshall@chromium.org

[haibinlu, 2015-08-07 23:38:01]

will add/update unit test after initial review.

[Kevin M, 2015-08-08 00:35:57]

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

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:100: const
std::set<MediaSink::Id>& sink_ids_in_launch() const {
ISTM that this doesn't belong at this layer - there is very little information
about sinks and routes that is persisted in this object, and I think we should
try to maintain that separation.

Can you move this to media_router_ui?

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:455:
if (this.isLaunching_)
I noticed that the UI still responds to clicks, showing animations and such
while in the launching state. Is this desirable?

Could you make use of the "disabled" Polymer attribute instead?

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:524:
rebuildLaunchState_: function() {
Do we need to store the result of this in a bool, or could we just do the some()
calculation in the getter every time?

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.h:187:
std::vector<MediaSinkWithCastModes> sinks_;
Can you put the "is launching" state here?

Or as a bool in the "MediaSinkWithCastModes" struct?

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:183:
scoped_ptr<base::StringValue> sink_id_value(new base::StringValue(sink_id));
I don't think scoped_ptr + heap allocation are needed here (or elsewhere in this
file?)

You can just declare a base::StringValue on the stack.

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:241:
media_router_ui->sinks(), media_router_ui->GetSinkIdsInLaunch()));
I think it might be cleaner to have media_router_ui->sinks() contain the "is
launching" bool than to require that this layer do some additional assembly work
with another set.

[haibinlu, 2015-08-08 01:25:39]

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

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:100: const
std::set<MediaSink::Id>& sink_ids_in_launch() const {
On 2015/08/08 00:35:56, Kevin M wrote:
> ISTM that this doesn't belong at this layer - there is very little information
> about sinks and routes that is persisted in this object, and I think we should
> try to maintain that separation.
> 
> Can you move this to media_router_ui?

It cannot. It has to survive UI close and reopen. Plus, auto-mirroring-to-flings
transition (yet to be implemented) will go without UI open; mirroring launch
state needs to be kept outside UI too.

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:455:
if (this.isLaunching_)
On 2015/08/08 00:35:56, Kevin M wrote:
> I noticed that the UI still responds to clicks, showing animations and such
> while in the launching state. Is this desirable?
> 
> Could you make use of the "disabled" Polymer attribute instead?

I tried that. The UI looks ugly when disabled. Plus "disabled" prevents button
clicking event, but still generate clicking event for <div>. We can update this
later with UX input.

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:524:
rebuildLaunchState_: function() {
On 2015/08/08 00:35:56, Kevin M wrote:
> Do we need to store the result of this in a bool, or could we just do the
some()
> calculation in the getter every time?

Since we are going to use the result at quite a few places, the repeated
calculation seems inefficient.

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.h:187:
std::vector<MediaSinkWithCastModes> sinks_;
On 2015/08/08 00:35:56, Kevin M wrote:
> Can you put the "is launching" state here?
> 
> Or as a bool in the "MediaSinkWithCastModes" struct?

sinks_ was overwritten when extension updates the list. Unless we let extension
keep tracking the launch states and set the isLaunching boolean in each sink
object, we still need to maintain a launch set here.

Letting extension maintain launch status is fine too.

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:183:
scoped_ptr<base::StringValue> sink_id_value(new base::StringValue(sink_id));
On 2015/08/08 00:35:57, Kevin M wrote:
> I don't think scoped_ptr + heap allocation are needed here (or elsewhere in
this
> file?)
> 
> You can just declare a base::StringValue on the stack.

Done.

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:241:
media_router_ui->sinks(), media_router_ui->GetSinkIdsInLaunch()));
On 2015/08/08 00:35:57, Kevin M wrote:
> I think it might be cleaner to have media_router_ui->sinks() contain the "is
> launching" bool than to require that this layer do some additional assembly
work
> with another set.

If we add inLaunching in the sink object, it is better to let extension set it,
and this will be pass-through.  Let us discuss next Monday.

[apacible, 2015-08-10 14:13:27]

Please attach a crbug for this, thanks!

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1273423004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:455:
if (this.isLaunching_)
On 2015/08/08 01:25:38, haibinlu wrote:
> On 2015/08/08 00:35:56, Kevin M wrote:
> > I noticed that the UI still responds to clicks, showing animations and such
> > while in the launching state. Is this desirable?
> > 
> > Could you make use of the "disabled" Polymer attribute instead?
> 
> I tried that. The UI looks ugly when disabled. Plus "disabled" prevents button
> clicking event, but still generate clicking event for <div>. We can update
this
> later with UX input.

Let's chat about this offline. Jonny's in MTV this week, but we can send him
screenshots for feedback.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css
(right):

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:74:
.sink-icon {
The cast mode list icons used the "paper-item iron-icon" styling as well; please
make sure this styling is still applied.

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

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:74:
<template is="dom-if" if="[[item.isLaunching]]">
Switch to just using modifying the hidden attribute rather than using a
conditional template (template is more costly).

From the primer:
"Note, since it is generally much faster to hide/show elements rather than
create/destroy them, conditional templates are only useful to save initial
creation cost when the elements being stamped are relatively heavyweight and the
conditional may rarely (or never) be true in given useages. Otherwise, liberal
use of conditional templates can actually add significant runtime performance
overhead.

...

Hiding/showing elements is as simple as attribute-binding to the hidden
attribute (e.g. <div hidden$="{{!shouldShow}}">), and does not require
conditional templating at all."

More info:
https://github.com/Polymer/polymer/blob/master/PRIMER.md#conditional-template

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

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:106:
* True if the Media Router is creating a route.
Can you be more specific with this comment? Perhaps something along the lines
of: "True if we have requested route creation / False when the WebUI gets a
response and/or is not expecting a response anymore".

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:107:
*
nit: remove this line

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:110:
isLaunching_: {
nit: put in alphabetical order

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:219:
* @param {?media_router.Route} route The newly create route to the sink
nit: created

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:225:
if (!route) {
Are we pushing an issue from MR if route creation failed?

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:341:
return sinkToRouteMap[sinkId] && sinkToRouteMap[sinkId].isLocal ?
Per UX review / discussion, we'll show the blue icon if:
...in WebUI (this case): any active route exists (local and non-local)
...in Toolbar: a local route exists

The sinkToRouteMap[sinkId].isLocal condition here is not needed.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:382:
if (this.localRouteCount_ == 1 && this.justOpened_ && route) {
nit: no curly braces

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:419:
this.set(['sinkList', index, 'isLaunching'], isLaunching);
It looks like |isLaunching| param always corresponds with |this.isLaunching_|.
We can remove |isLaunching| and replace with |this.isLaunching_| directly in the
function.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:458:
this.$.sinkList.itemForElement(event.target));
Can this fit on the previous line?

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:501:
onSinkListUpdated_: function() {
nit: alphabetize

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:520:
* Called when |sinkList| is updated. Rebuilds |isLaunching_|.
nit: "Update" rather than "Rebuilds" since |isLaunching_| is just a boolean,
here and for function name.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:524:
rebuildLaunchState_: function() {
nit: alphabetize

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:542:
* @param {?media_router.Route} route The route to show.
nit: newline between description and @param

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:566:
if (this.isLaunching_)
Can we gray out the arrow toggle icon? Otherwise, there won't be any visual
feedback for why the cast mode list isn't being toggled if we just do nothing.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:22: *   C++
MediaCastMode.
Can you also align this with the first instance of "type" on the line above, for
consistency? Thanks!

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:126: *    to this
sink.
nit: align with "isLaunching" on the line above.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:291: if (!route) {
To simplify, maybe something like this for L291-296?

if (!route)
  LOG(ERROR)...

handler_->OnCreateRouteResponseReceived(sink_id, route);

[haibinlu, 2015-08-11 01:31:47]

PTAL

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css
(right):

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:74:
.sink-icon {
On 2015/08/10 14:13:26, apacible wrote:
> The cast mode list icons used the "paper-item iron-icon" styling as well;
please
> make sure this styling is still applied.

Done.

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

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:74:
<template is="dom-if" if="[[item.isLaunching]]">
On 2015/08/10 14:13:26, apacible wrote:
> Switch to just using modifying the hidden attribute rather than using a
> conditional template (template is more costly).
> 
> From the primer:
> "Note, since it is generally much faster to hide/show elements rather than
> create/destroy them, conditional templates are only useful to save initial
> creation cost when the elements being stamped are relatively heavyweight and
the
> conditional may rarely (or never) be true in given useages. Otherwise, liberal
> use of conditional templates can actually add significant runtime performance
> overhead.
> 
> ...
> 
> Hiding/showing elements is as simple as attribute-binding to the hidden
> attribute (e.g. <div hidden$="{{!shouldShow}}">), and does not require
> conditional templating at all."
> 
> More info:
> https://github.com/Polymer/polymer/blob/master/PRIMER.md#conditional-template

per offline discussion, launch is much less often than opening UI to show the
list, and only one launch allowed.

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

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:106:
* True if the Media Router is creating a route.
On 2015/08/10 14:13:26, apacible wrote:
> Can you be more specific with this comment? Perhaps something along the lines
> of: "True if we have requested route creation / False when the WebUI gets a
> response and/or is not expecting a response anymore".

MR (now extension) handles when to stop marking sink.isLaunching true. so, it
does not make sense to list when this value is false.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:107:
*
On 2015/08/10 14:13:26, apacible wrote:
> nit: remove this line

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:110:
isLaunching_: {
On 2015/08/10 14:13:26, apacible wrote:
> nit: put in alphabetical order

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:219:
* @param {?media_router.Route} route The newly create route to the sink
On 2015/08/10 14:13:26, apacible wrote:
> nit: created

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:225:
if (!route) {
On 2015/08/10 14:13:26, apacible wrote:
> Are we pushing an issue from MR if route creation failed?

That is one way to do that assuming MR has more info about failure. If we decide
to show a generic error, then UI can handle it.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:341:
return sinkToRouteMap[sinkId] && sinkToRouteMap[sinkId].isLocal ?
On 2015/08/10 14:13:26, apacible wrote:
> Per UX review / discussion, we'll show the blue icon if:
> ...in WebUI (this case): any active route exists (local and non-local)
> ...in Toolbar: a local route exists
> 
> The sinkToRouteMap[sinkId].isLocal condition here is not needed.

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:382:
if (this.localRouteCount_ == 1 && this.justOpened_ && route) {
On 2015/08/10 14:13:26, apacible wrote:
> nit: no curly braces

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:419:
this.set(['sinkList', index, 'isLaunching'], isLaunching);
On 2015/08/10 14:13:26, apacible wrote:
> It looks like |isLaunching| param always corresponds with |this.isLaunching_|.
> We can remove |isLaunching| and replace with |this.isLaunching_| directly in
the
> function.
right now, yes. but we ever allow multiple launches, the param is needed.
Plus, this method sets individual sink field, it is cleaner to have a param for
it.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:458:
this.$.sinkList.itemForElement(event.target));
On 2015/08/10 14:13:26, apacible wrote:
> Can this fit on the previous line?

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:501:
onSinkListUpdated_: function() {
On 2015/08/10 14:13:26, apacible wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:520:
* Called when |sinkList| is updated. Rebuilds |isLaunching_|.
On 2015/08/10 14:13:26, apacible wrote:
> nit: "Update" rather than "Rebuilds" since |isLaunching_| is just a boolean,
> here and for function name.

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:524:
rebuildLaunchState_: function() {
On 2015/08/10 14:13:26, apacible wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:542:
* @param {?media_router.Route} route The route to show.
On 2015/08/10 14:13:26, apacible wrote:
> nit: newline between description and @param

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:566:
if (this.isLaunching_)
On 2015/08/10 14:13:26, apacible wrote:
> Can we gray out the arrow toggle icon? Otherwise, there won't be any visual
> feedback for why the cast mode list isn't being toggled if we just do nothing.

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:22: *   C++
MediaCastMode.
On 2015/08/10 14:13:27, apacible wrote:
> Can you also align this with the first instance of "type" on the line above,
for
> consistency? Thanks!

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:126: *    to this
sink.
On 2015/08/10 14:13:27, apacible wrote:
> nit: align with "isLaunching" on the line above.

Done.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:291: if (!route) {
On 2015/08/10 14:13:27, apacible wrote:
> To simplify, maybe something like this for L291-296?
> 
> if (!route)
>   LOG(ERROR)...
> 
> handler_->OnCreateRouteResponseReceived(sink_id, route);

Done.

[apacible, 2015-08-11 18:29:58]

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

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:74:
<template is="dom-if" if="[[item.isLaunching]]">
On 2015/08/11 01:31:46, haibinlu wrote:
> On 2015/08/10 14:13:26, apacible wrote:
> > Switch to just using modifying the hidden attribute rather than using a
> > conditional template (template is more costly).
> > 
> > From the primer:
> > "Note, since it is generally much faster to hide/show elements rather than
> > create/destroy them, conditional templates are only useful to save initial
> > creation cost when the elements being stamped are relatively heavyweight and
> the
> > conditional may rarely (or never) be true in given useages. Otherwise,
liberal
> > use of conditional templates can actually add significant runtime
performance
> > overhead.
> > 
> > ...
> > 
> > Hiding/showing elements is as simple as attribute-binding to the hidden
> > attribute (e.g. <div hidden$="{{!shouldShow}}">), and does not require
> > conditional templating at all."
> > 
> > More info:
> >
https://github.com/Polymer/polymer/blob/master/PRIMER.md#conditional-template
> 
> per offline discussion, launch is much less often than opening UI to show the
> list, and only one launch allowed.

Acknowledged.

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

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:106:
* True if the Media Router is creating a route.
On 2015/08/11 01:31:47, haibinlu wrote:
> On 2015/08/10 14:13:26, apacible wrote:
> > Can you be more specific with this comment? Perhaps something along the
lines
> > of: "True if we have requested route creation / False when the WebUI gets a
> > response and/or is not expecting a response anymore".
> 
> MR (now extension) handles when to stop marking sink.isLaunching true. so, it
> does not make sense to list when this value is false.

Acknowledged.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:225:
if (!route) {
On 2015/08/11 01:31:46, haibinlu wrote:
> On 2015/08/10 14:13:26, apacible wrote:
> > Are we pushing an issue from MR if route creation failed?
> 
> That is one way to do that assuming MR has more info about failure. If we
decide
> to show a generic error, then UI can handle it.

Acknowledged.

https://codereview.chromium.org/1273423004/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:419:
this.set(['sinkList', index, 'isLaunching'], isLaunching);
On 2015/08/11 01:31:47, haibinlu wrote:
> On 2015/08/10 14:13:26, apacible wrote:
> > It looks like |isLaunching| param always corresponds with
|this.isLaunching_|.
> > We can remove |isLaunching| and replace with |this.isLaunching_| directly in
> the
> > function.
> right now, yes. but we ever allow multiple launches, the param is needed.
> Plus, this method sets individual sink field, it is cleaner to have a param
for
> it.

Acknowledged.

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:207: 
remove newline?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css
(right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:74:
.sink-icon, .cast-mode{
For consistency, maybe "cast-mode-icon"?

nit: order these as ".cast-mode-icon, .sink-icon" and then alphabetize. :)

nit 2: space before the curly brace

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:27:
id="arrow-drop-icon" on-click="toggleCastModeHidden_">
nit: move id to previous line since it looks like it'll fit

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:340:
return sinkToRouteMap[sinkId]?
nit: space before the ?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:341:
'sink-icon active-sink' : 'sink-icon';
Does this fit on the previous line?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:567:
}, this);
Do we need |this|?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_ui_interface.js:19: *     if
succeeded; null otherwise.
nit wording suggestion: "if route creation succeeded; null otherwise"

[Kevin M, 2015-08-11 21:28:48]

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_sink.h:36: bool is_launching_;
Comment?

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:26:
disabled$="[[isLaunching_]]"
Could you create a compute function for this value that takes sinkList as a
param?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:397:
this.isLaunching_ = true;
This boolean seems redundant. Can't we wait for the extension to send over a new
sinkList with the is_launching bit set correctly?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:407:
* Shows or hides the launch spinner next to a sink.
Temporarily overrides the "isLaunching" bit for a sink.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:414:
updateLaunchSpinner_: function(sinkId, isLaunching) {
I think "set" makes more sense than "update" here, since the semantics of the
call mutate a sink's record, rather than persisting or transmitting an existing
sink.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:292: LOG(ERROR) <<
"MediaRouteResponse returned error: " << error;
LOG(ERROR) is probably too spammy, esp. because this condition can occur under
normal flaky networks.

VLOG(0)?

[haibinlu, 2015-08-12 20:32:53]

PTAL

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:207: 
On 2015/08/11 18:29:58, apacible wrote:
> remove newline?

Done.

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_sink.h:36: bool is_launching_;
On 2015/08/11 21:28:47, Kevin M wrote:
> Comment?

Done.

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

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:26:
disabled$="[[isLaunching_]]"
On 2015/08/11 21:28:47, Kevin M wrote:
> Could you create a compute function for this value that takes sinkList as a
> param?

This variable is used at quite a few places. is it worth to do many linear
searching to just avoid having this variable?

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:27:
id="arrow-drop-icon" on-click="toggleCastModeHidden_">
On 2015/08/11 18:29:58, apacible wrote:
> nit: move id to previous line since it looks like it'll fit

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:340:
return sinkToRouteMap[sinkId]?
On 2015/08/11 18:29:58, apacible wrote:
> nit: space before the ?

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:341:
'sink-icon active-sink' : 'sink-icon';
On 2015/08/11 18:29:58, apacible wrote:
> Does this fit on the previous line?

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:397:
this.isLaunching_ = true;
On 2015/08/11 21:28:47, Kevin M wrote:
> This boolean seems redundant. Can't we wait for the extension to send over a
new
> sinkList with the is_launching bit set correctly?

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:407:
* Shows or hides the launch spinner next to a sink.
On 2015/08/11 21:28:47, Kevin M wrote:
> Temporarily overrides the "isLaunching" bit for a sink.

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:414:
updateLaunchSpinner_: function(sinkId, isLaunching) {
On 2015/08/11 21:28:47, Kevin M wrote:
> I think "set" makes more sense than "update" here, since the semantics of the
> call mutate a sink's record, rather than persisting or transmitting an
existing
> sink.

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:567:
}, this);
On 2015/08/11 18:29:58, apacible wrote:
> Do we need |this|?

removed

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_ui_interface.js (right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_ui_interface.js:19: *     if
succeeded; null otherwise.
On 2015/08/11 18:29:58, apacible wrote:
> nit wording suggestion: "if route creation succeeded; null otherwise"

Done.

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1273423004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:292: LOG(ERROR) <<
"MediaRouteResponse returned error: " << error;
On 2015/08/11 21:28:47, Kevin M wrote:
> LOG(ERROR) is probably too spammy, esp. because this condition can occur under
> normal flaky networks.
> 
> VLOG(0)?

Done.

[Kevin M, 2015-08-12 20:39:22]

lgtm

[apacible, 2015-08-12 20:51:49]

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

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:86:
.cast-mode-icon, .sink-icon {
nit: alphabetize (place between .active-sink and #cast-mode-list)

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

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:221:
onCreateRouteResponseReceived: function(sinkId, route) {
nit: alphabetize. Also, can you move propogateExtensionId_() so that it's also
listed alphabetically?

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:596:
this.currentView_ = this.CONTAINER_VIEW_.ROUTE_DETAILS;
Add a check here to update currentView_ only if route isn't null?

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:620:

nit: remove newline

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:295: VLOG(0) <<
"MediaRouteResponse returned error: " << error;
When do we use VLOG versus DVLOG?

[haibinlu, 2015-08-12 21:15:30]

PTAL

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

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:86:
.cast-mode-icon, .sink-icon {
On 2015/08/12 20:51:49, apacible wrote:
> nit: alphabetize (place between .active-sink and #cast-mode-list)

Done.

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

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:221:
onCreateRouteResponseReceived: function(sinkId, route) {
On 2015/08/12 20:51:49, apacible wrote:
> nit: alphabetize. Also, can you move propogateExtensionId_() so that it's also
> listed alphabetically?

moved many methods around.

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:596:
this.currentView_ = this.CONTAINER_VIEW_.ROUTE_DETAILS;
On 2015/08/12 20:51:49, apacible wrote:
> Add a check here to update currentView_ only if route isn't null?

It is a precondition. Update the param type to require non-null route.

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:620:

On 2015/08/12 20:51:49, apacible wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1273423004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:295: VLOG(0) <<
"MediaRouteResponse returned error: " << error;
On 2015/08/12 20:51:49, apacible wrote:
> When do we use VLOG versus DVLOG?

DVLOG for debug build only. It is enough for us. Updated.

[apacible, 2015-08-12 21:17:23]

lgtm

[haibinlu, 2015-08-12 21:18:52]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-08-12 21:18:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1273423004/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-12 21:19:36]

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

[commit-bot: I haz the power, 2015-08-12 22:09:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-12 22:09:56]

Patchset 6 (id:??) landed as
https://crrev.com/2d652bcd6beebeaf6f867a3f0dd34103dd27ec16
Cr-Commit-Position: refs/heads/master@{#343106}