[Media Router WebUI] Append new sinks to the end of the list.

[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace |sinksToShow_| in the container. This sometimes causes re-sorting and sinks suddenly appear in a different place. This can result in accidental casting to the wrong sink.

This change appends new sinks to the end of the sink list. A user who has incrementally discovered sinks will see the dialog grow vertically as new sinks are appended at the bottom of the list.

Also updated variable naming to make it more easily readable.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[apacible, 2016-08-09 00:48:23]

Description was changed from

==========
asdf

BUG=
==========

to

==========
asdf

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[apacible, 2016-08-09 00:50:36]

The CQ bit was checked by apacible@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 00:51:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[apacible, 2016-08-09 00:55:09]

Description was changed from

==========
asdf

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[apacible, 2016-08-09 00:55:09]

Description was changed from

==========
asdf

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-09 00:58:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 00:58:13]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[apacible, 2016-08-09 01:09:10]

The CQ bit was checked by apacible@chromium.org to run a CQ dry run

[apacible, 2016-08-09 01:09:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-09 01:09:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[apacible, 2016-08-09 01:10:53]

Description was changed from

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

Also updated variable naming to make it more easily readable.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-09 01:17:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 01:17:30]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[apacible, 2016-08-09 01:37:05]

The CQ bit was checked by apacible@chromium.org to run a CQ dry run

[apacible, 2016-08-09 01:37:19]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-09 01:37:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[apacible, 2016-08-09 01:40:55]

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

[apacible, 2016-08-09 01:43:07]

PTAL, thanks! I plan merge this back to M53 before OOO, so would like to get
this change in by Tuesday EOD.

[commit-bot: I haz the power, 2016-08-09 01:44:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 01:44:13]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[imcheng, 2016-08-09 02:29:09]

As discussed, since we are no longer maintaining order by sink name, we should
just remove the sorting logic in the C++ code.

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

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2024:
// dialog, which can surprise users / lead to inadvertent casting to the
nit: inadvertently

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2029:
for (var j = 0; j < updatedSinkList.length; j++) {
You can probably write L2028-L2041 as:

var index = this.sinksToShow_.findIndex(updatedSink => { return
this.sinksToShow_[i].id == updatedSink.id; });
if (index < 0) {
  this.sinksToShow_.splice(i, 1);
} else {
  updatedSinkList.splice(j, 1);
}

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2044:
updatedSinkList = this.sinksToShow_.concat(updatedSinkList);
this.sinksToShow_.add(...updatedSinkList); and no need reassign
this.sinksToShow_ in this case.

[imcheng, 2016-08-09 02:29:55]

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

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2044:
updatedSinkList = this.sinksToShow_.concat(updatedSinkList);
On 2016/08/09 02:29:09, imcheng wrote:
> this.sinksToShow_.add(...updatedSinkList); and no need reassign
> this.sinksToShow_ in this case.

s/add/push

[apacible, 2016-08-09 03:58:47]

The CQ bit was checked by apacible@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 03:59:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[apacible, 2016-08-09 04:02:39]

> As discussed, since we are no longer maintaining order by sink name, we should
> just remove the sorting logic in the C++ code.

After thinking about it a bit more, I think it makes sense to keep the sorting
logic in the C++ side. After initial discovery, opening the dialog should give
us most, if not all, of the available sinks. It would be nice to maintain an
order. WDYT?

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

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2024:
// dialog, which can surprise users / lead to inadvertent casting to the
On 2016/08/09 02:29:09, imcheng wrote:
> nit: inadvertently

Done.

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2029:
for (var j = 0; j < updatedSinkList.length; j++) {
On 2016/08/09 02:29:09, imcheng wrote:
> You can probably write L2028-L2041 as:
> 
> var index = this.sinksToShow_.findIndex(updatedSink => { return
> this.sinksToShow_[i].id == updatedSink.id; });
> if (index < 0) {
>   this.sinksToShow_.splice(i, 1);
> } else {
>   updatedSinkList.splice(j, 1);
> }

Done (with minor tweaks).

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2044:
updatedSinkList = this.sinksToShow_.concat(updatedSinkList);
On 2016/08/09 02:29:55, imcheng wrote:
> On 2016/08/09 02:29:09, imcheng wrote:
> > this.sinksToShow_.add(...updatedSinkList); and no need reassign
> > this.sinksToShow_ in this case.
> 
> s/add/push

Kept as concat() as push() takes items individually as parameters. Reassigning
this.sinksToShow_() explicitly also triggers the observer function.

[imcheng, 2016-08-09 04:31:10]

On 2016/08/09 04:02:39, apacible wrote:
> > As discussed, since we are no longer maintaining order by sink name, we
should
> > just remove the sorting logic in the C++ code.
> 
> After thinking about it a bit more, I think it makes sense to keep the sorting
> logic in the C++ side. After initial discovery, opening the dialog should give
> us most, if not all, of the available sinks. It would be nice to maintain an
> order. WDYT?


That may or may not be true depending on the user's machine and network
conditions. IMO it makes more sense for it to be either always sorted, or no
sorting at all. I feel the user may be confused by the almost-sorted behavior.
Another way to address this would be to ignore clicks for a short period right
after the list is updated (say 100ms). Just a thought. But I will defer to UX's
judgment on this.

 
> 
>
https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
> File
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
> (right):
> 
>
https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2024:
> // dialog, which can surprise users / lead to inadvertent casting to the
> On 2016/08/09 02:29:09, imcheng wrote:
> > nit: inadvertently
> 
> Done.
> 
>
https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2029:
> for (var j = 0; j < updatedSinkList.length; j++) {
> On 2016/08/09 02:29:09, imcheng wrote:
> > You can probably write L2028-L2041 as:
> > 
> > var index = this.sinksToShow_.findIndex(updatedSink => { return
> > this.sinksToShow_[i].id == updatedSink.id; });
> > if (index < 0) {
> >   this.sinksToShow_.splice(i, 1);
> > } else {
> >   updatedSinkList.splice(j, 1);
> > }
> 
> Done (with minor tweaks).
> 
>
https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2044:
> updatedSinkList = this.sinksToShow_.concat(updatedSinkList);
> On 2016/08/09 02:29:55, imcheng wrote:
> > On 2016/08/09 02:29:09, imcheng wrote:
> > > this.sinksToShow_.add(...updatedSinkList); and no need reassign
> > > this.sinksToShow_ in this case.
> > 
> > s/add/push
> 
> Kept as concat() as push() takes items individually as parameters. Reassigning
> this.sinksToShow_() explicitly also triggers the observer function.

[imcheng, 2016-08-09 04:31:27]

lgtm after comments addressed

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

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2029:
for (var j = 0; j < updatedSinkList.length; j++) {
On 2016/08/09 04:02:39, apacible wrote:
> On 2016/08/09 02:29:09, imcheng wrote:
> > You can probably write L2028-L2041 as:
> > 
> > var index = this.sinksToShow_.findIndex(updatedSink => { return
> > this.sinksToShow_[i].id == updatedSink.id; });
> > if (index < 0) {
> >   this.sinksToShow_.splice(i, 1);
> > } else {
> >   updatedSinkList.splice(j, 1);
> > }
> 
> Done (with minor tweaks).

Cool. What is the reason for not using the arrow function? ES6 should be
supported by now?

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2044:
updatedSinkList = this.sinksToShow_.concat(updatedSinkList);
On 2016/08/09 04:02:39, apacible wrote:
> On 2016/08/09 02:29:55, imcheng wrote:
> > On 2016/08/09 02:29:09, imcheng wrote:
> > > this.sinksToShow_.add(...updatedSinkList); and no need reassign
> > > this.sinksToShow_ in this case.
> > 
> > s/add/push
> 
> Kept as concat() as push() takes items individually as parameters. Reassigning
> this.sinksToShow_() explicitly also triggers the observer function.

Okay since we need to trigger observer function. By the way, the spread operator
"..." will convert an iterable object into individual items:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/S...

[apacible, 2016-08-09 05:23:00]

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

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2029:
for (var j = 0; j < updatedSinkList.length; j++) {
On 2016/08/09 04:31:26, imcheng wrote:
> On 2016/08/09 04:02:39, apacible wrote:
> > On 2016/08/09 02:29:09, imcheng wrote:
> > > You can probably write L2028-L2041 as:
> > > 
> > > var index = this.sinksToShow_.findIndex(updatedSink => { return
> > > this.sinksToShow_[i].id == updatedSink.id; });
> > > if (index < 0) {
> > >   this.sinksToShow_.splice(i, 1);
> > > } else {
> > >   updatedSinkList.splice(j, 1);
> > > }
> > 
> > Done (with minor tweaks).
> 
> Cool. What is the reason for not using the arrow function? ES6 should be
> supported by now?

Presubmit complained about it when I tried to upload the patch. I also didn't
find other usages of the arrow function under c/b/r.

https://codereview.chromium.org/2221933003/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2044:
updatedSinkList = this.sinksToShow_.concat(updatedSinkList);
On 2016/08/09 04:31:26, imcheng wrote:
> On 2016/08/09 04:02:39, apacible wrote:
> > On 2016/08/09 02:29:55, imcheng wrote:
> > > On 2016/08/09 02:29:09, imcheng wrote:
> > > > this.sinksToShow_.add(...updatedSinkList); and no need reassign
> > > > this.sinksToShow_ in this case.
> > > 
> > > s/add/push
> > 
> > Kept as concat() as push() takes items individually as parameters.
Reassigning
> > this.sinksToShow_() explicitly also triggers the observer function.
> 
> Okay since we need to trigger observer function. By the way, the spread
operator
> "..." will convert an iterable object into individual items:
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/S...

Acknowledged, thanks!

[apacible, 2016-08-09 05:29:39]

The CQ bit was unchecked by apacible@chromium.org

[apacible, 2016-08-09 05:29:40]

The CQ bit was checked by apacible@chromium.org

[commit-bot: I haz the power, 2016-08-09 05:29:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 05:33:31]

Description was changed from

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

Also updated variable naming to make it more easily readable.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

Also updated variable naming to make it more easily readable.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-09 05:33:32]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-08-09 05:36:14]

Description was changed from

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

Also updated variable naming to make it more easily readable.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router WebUI] Append new sinks to the end of the list.

Currently, when the sink list is updated in the WebUI, we just replace
|sinksToShow_| in the container. This sometimes causes re-sorting and sinks
suddenly appear in a different place. This can result in accidental casting to
the wrong sink.

This change appends new sinks to the end of the sink list. A user who has
incrementally discovered sinks will see the dialog grow vertically as new sinks
are appended at the bottom of the list.

Also updated variable naming to make it more easily readable.

BUG=634365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-08-09 05:36:16]

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