Media Router: Update routes in sink list when routes are updated.

Media Router: Update routes in sink list when routes are updated.

Currently, the routes in the sink list are only updated when the dialog is opened. We don't update them when we have create new or close existing routes. This change updates the sink list to reflect any active routes immediately when they WebUI knows about them.

BUG=507802
Committed: https://crrev.com/3a50384f614850e1bf5641a90d165524cbb786f7
Cr-Commit-Position: refs/heads/master@{#338388}

[apacible, 2015-07-07 22:59:32]

Patchset #1 (id:1) has been deleted

[apacible, 2015-07-07 22:59:36]

Patchset #1 (id:20001) has been deleted

[apacible, 2015-07-08 00:05:55]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-07-08 00:17:54]

Patchset #1 (id:60001) has been deleted

[apacible, 2015-07-08 00:18:07]

Patchset #1 (id:80001) has been deleted

[apacible, 2015-07-08 00:18:14]

Patchset #1 (id:100001) has been deleted

[apacible, 2015-07-08 00:21:53]

apacible@chromium.org changed reviewers:
+ kmarshall@chromium.org

[apacible, 2015-07-08 00:25:36]

Patchset #1 (id:120001) has been deleted

[apacible, 2015-07-08 00:26:17]

apacible@chromium.org changed reviewers:
+ michaelpg@chromium.org

[apacible, 2015-07-08 00:29:28]

PTAL, thanks!

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:57:
<span>[[
michaelpg@: L57-59 new lines is to get past the 80char lines and binding without
any whitespace between the [[text]] and <span> tags. Let me know if there's a
more preferable way for this.

[Kevin M, 2015-07-08 22:47:22]

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:326:
// Rebuild |sinkToRouteMap_| with a temporary map to avoid multiple calls
Unnecessary UI rebinding, right?

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:361:
var index = this.routeList.indexOf(data.detail.route);
Is this handling the case where the data couldn't be found? indexOf() returns -1
in that case, which has totally different behavior with array.splice.

[apacible, 2015-07-08 23:23:53]

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:326:
// Rebuild |sinkToRouteMap_| with a temporary map to avoid multiple calls
On 2015/07/08 22:47:22, Kevin M wrote:
> Unnecessary UI rebinding, right?

Correct.

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:361:
var index = this.routeList.indexOf(data.detail.route);
On 2015/07/08 22:47:22, Kevin M wrote:
> Is this handling the case where the data couldn't be found? indexOf() returns
-1
> in that case, which has totally different behavior with array.splice.

Fixed.

[michaelpg, 2015-07-10 01:17:30]

lgtm w/ one nuance

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:57:
<span>[[
On 2015/07/08 00:29:28, apacible wrote:
> michaelpg@: L57-59 new lines is to get past the 80char lines and binding
without
> any whitespace between the [[text]] and <span> tags. Let me know if there's a
> more preferable way for this.

This is what we do.

https://codereview.chromium.org/1218213007/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:327:
// to |computeRouteInSinkListHidden_| and |computeRouteInSinkListValue_|.
Since those computed annotations are observing sinkToRouteMap_ (rather than
sinkToRouteMap_.*), they'll only fire when sinkToRouteMap_ is set to a new
object, instead of when you update a property on that object.

What's really happening (I think) is that if you didn't use tempSinkToRouteMap,
the computed annotation would fire right when you set sinkToRouteMap_ = {}, so
you wouldn't get any routes.

What you have is ok -- the comment is just a little misleading in that case.

[apacible, 2015-07-10 02:36:41]

Patchset #3 (id:180001) has been deleted

[apacible, 2015-07-10 02:49:28]

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/1218213007/diff/140001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:57:
<span>[[
On 2015/07/10 01:17:29, michaelpg wrote:
> On 2015/07/08 00:29:28, apacible wrote:
> > michaelpg@: L57-59 new lines is to get past the 80char lines and binding
> without
> > any whitespace between the [[text]] and <span> tags. Let me know if there's
a
> > more preferable way for this.
> 
> This is what we do.

Acknowledged.

https://codereview.chromium.org/1218213007/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/160001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:327:
// to |computeRouteInSinkListHidden_| and |computeRouteInSinkListValue_|.
On 2015/07/10 01:17:30, michaelpg wrote:
> Since those computed annotations are observing sinkToRouteMap_ (rather than
> sinkToRouteMap_.*), they'll only fire when sinkToRouteMap_ is set to a new
> object, instead of when you update a property on that object.
> 
> What's really happening (I think) is that if you didn't use
tempSinkToRouteMap,
> the computed annotation would fire right when you set sinkToRouteMap_ = {}, so
> you wouldn't get any routes.
> 
> What you have is ok -- the comment is just a little misleading in that case.

Thanks, and updated the comment.

[Kevin M, 2015-07-10 20:33:12]

https://codereview.chromium.org/1218213007/diff/220001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:239:
return !sinkToRouteMap_[sinkId];
Looks like this is referring to an undefined entity.

Should this be sinkToRouteMap? Or this.sinkToRouteMap_ ?

https://codereview.chromium.org/1218213007/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:248:
var route = sinkToRouteMap_[sinkId];
ditto

[apacible, 2015-07-10 21:15:24]

https://codereview.chromium.org/1218213007/diff/220001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:239:
return !sinkToRouteMap_[sinkId];
On 2015/07/10 20:33:12, Kevin M wrote:
> Looks like this is referring to an undefined entity.
> 
> Should this be sinkToRouteMap? Or this.sinkToRouteMap_ ?

Oops, I switched it from this.sinkRouteMap_ to sinkToRouteMap to be consistent
with the other compute functions and didn't remove the trailing_.

https://codereview.chromium.org/1218213007/diff/220001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:248:
var route = sinkToRouteMap_[sinkId];
On 2015/07/10 20:33:12, Kevin M wrote:
> ditto

Fixed.

[Kevin M, 2015-07-10 21:19:34]

lgtm

https://codereview.chromium.org/1218213007/diff/180002/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/180002/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:355:
var tempSinkToRouteMap_ = {};
No need for a trailing _

[apacible, 2015-07-10 21:44:33]

https://codereview.chromium.org/1218213007/diff/180002/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1218213007/diff/180002/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:355:
var tempSinkToRouteMap_ = {};
On 2015/07/10 21:19:34, Kevin M wrote:
> No need for a trailing _

Done.

[apacible, 2015-07-10 21:55:23]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-07-10 21:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1218213007/#ps270001
(title: "Changes per kmarshall@'s comments.")

[commit-bot: I haz the power, 2015-07-10 21:55:38]

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

[commit-bot: I haz the power, 2015-07-10 22:50:33]

Committed patchset #7 (id:270001)

[commit-bot: I haz the power, 2015-07-10 22:51:25]

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