[Media Router WebUI] Disable search by default.

[Media Router WebUI] Disable search by default.

This change disables the search feature by default, only enabling it for
exceptionally long sink lists and when pseudo sinks are available.
Whether search is enabled is determined when the list of available sinks
is updated. Once it has been enabled, it will not be disabled for the
rest of the life of the dialog.

R=amp@chromium.org, apacible@chromium.org
BUG=612969
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4d46c6973164274ba1e67242eae30bbc9050fce0
Cr-Commit-Position: refs/heads/master@{#395128}

[btolsch, 2016-05-18 21:44:29]

Description was changed from

==========
[Media Router WebUI] Disable search by default.

This change disables the search feature by default, only enabling it for
exceptionally long sink lists and when pseudo sinks are available.
Whether search is enabled is determined when the list of available sinks
is updated. Once it has been enabled, it will not be disabled for the
rest of the life of the dialog.

R=amp@chromium.org, apacible@chromium.org
BUG=612969
==========

to

==========
[Media Router WebUI] Disable search by default.

This change disables the search feature by default, only enabling it for
exceptionally long sink lists and when pseudo sinks are available.
Whether search is enabled is determined when the list of available sinks
is updated. Once it has been enabled, it will not be disabled for the
rest of the life of the dialog.

R=amp@chromium.org, apacible@chromium.org
BUG=612969
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[btolsch, 2016-05-18 21:45:09]

PTAL, thanks!

[amp, 2016-05-18 22:25:25]

https://codereview.chromium.org/1993003002/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/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2281:
sinkList.style.paddingBottom = '0';
Does this ever need to be reset, or if search is visible is it always 0?

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:74:
media_router.MINIMUM_SINKS_FOR_SEARCH = 20;
I think even at 10 it would useful.

At 20 I would doubt this would show up for any non-cloud user. (but maybe that's
what we are going for?)

[btolsch, 2016-05-18 22:39:33]

https://codereview.chromium.org/1993003002/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/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2281:
sinkList.style.paddingBottom = '0';
On 2016/05/18 22:25:25, amp wrote:
> Does this ever need to be reset, or if search is visible is it always 0?

As long as search is visible, it is always 0.  If we allowed search to be
re-disabled, it would need to be reset.  As things are now, that would be dead
code.

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:74:
media_router.MINIMUM_SINKS_FOR_SEARCH = 20;
On 2016/05/18 22:25:25, amp wrote:
> I think even at 10 it would useful.
> 
> At 20 I would doubt this would show up for any non-cloud user. (but maybe
that's
> what we are going for?)

My understanding from previous comments was even when the list is long enough to
scroll, it's faster to scroll than type the sink name.  So from my perspective
we are effectively deemphasizing filtering and focusing more on search.

I don't feel strongly about what we make this number though.

[amp, 2016-05-19 01:03:41]

On 2016/05/18 22:39:33, btolsch wrote:
>
https://codereview.chromium.org/1993003002/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/1993003002/diff/1/chrome/browser/resources/me...
>
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2281:
> sinkList.style.paddingBottom = '0';
> On 2016/05/18 22:25:25, amp wrote:
> > Does this ever need to be reset, or if search is visible is it always 0?
> 
> As long as search is visible, it is always 0.  If we allowed search to be
> re-disabled, it would need to be reset.  As things are now, that would be dead
> code.
> 
>
https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
> File chrome/browser/resources/media_router/media_router_data.js (right):
> 
>
https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
> chrome/browser/resources/media_router/media_router_data.js:74:
> media_router.MINIMUM_SINKS_FOR_SEARCH = 20;
> On 2016/05/18 22:25:25, amp wrote:
> > I think even at 10 it would useful.
> > 
> > At 20 I would doubt this would show up for any non-cloud user. (but maybe
> that's
> > what we are going for?)
> 
> My understanding from previous comments was even when the list is long enough
to
> scroll, it's faster to scroll than type the sink name.  So from my perspective
> we are effectively deemphasizing filtering and focusing more on search.
> 
> I don't feel strongly about what we make this number though.

Ok, I'll defer to more UX minded folks.

[amp, 2016-05-19 01:03:49]

lgtm

[apacible, 2016-05-20 16:46:39]

lgtm with minor comments

https://codereview.chromium.org/1993003002/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/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1993:
this.sinksToShow_.length > media_router.MINIMUM_SINKS_FOR_SEARCH;
>=

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2072:
var searchContainer = this.$$('#sink-search');
When referring to elements, please keep the naming consistent so it's easier to
follow. e.g. "search" in line 1739 vs. "searchContainer" here vs. "search" in
the next line, which is a different element.

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2281:
sinkList.style.paddingBottom = '0';
On 2016/05/18 22:39:33, btolsch wrote:
> On 2016/05/18 22:25:25, amp wrote:
> > Does this ever need to be reset, or if search is visible is it always 0?
> 
> As long as search is visible, it is always 0.  If we allowed search to be
> re-disabled, it would need to be reset.  As things are now, that would be dead
> code.

Please add this as a comment. ^

[btolsch, 2016-05-20 17:54:01]

https://codereview.chromium.org/1993003002/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/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1993:
this.sinksToShow_.length > media_router.MINIMUM_SINKS_FOR_SEARCH;
On 2016/05/20 16:46:39, apacible wrote:
> >=

Done.

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2072:
var searchContainer = this.$$('#sink-search');
On 2016/05/20 16:46:39, apacible wrote:
> When referring to elements, please keep the naming consistent so it's easier
to
> follow. e.g. "search" in line 1739 vs. "searchContainer" here vs. "search" in
> the next line, which is a different element.

Done.

https://codereview.chromium.org/1993003002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2281:
sinkList.style.paddingBottom = '0';
On 2016/05/20 16:46:39, apacible wrote:
> On 2016/05/18 22:39:33, btolsch wrote:
> > On 2016/05/18 22:25:25, amp wrote:
> > > Does this ever need to be reset, or if search is visible is it always 0?
> > 
> > As long as search is visible, it is always 0.  If we allowed search to be
> > re-disabled, it would need to be reset.  As things are now, that would be
dead
> > code.
> 
> Please add this as a comment. ^

Done.

[btolsch, 2016-05-20 17:54:14]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-05-20 17:54:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/1993003002/#ps20001
(title: "Responding to comments")

[commit-bot: I haz the power, 2016-05-20 17:54:38]

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

[commit-bot: I haz the power, 2016-05-20 19:07:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-20 19:08:38]

Description was changed from

==========
[Media Router WebUI] Disable search by default.

This change disables the search feature by default, only enabling it for
exceptionally long sink lists and when pseudo sinks are available.
Whether search is enabled is determined when the list of available sinks
is updated. Once it has been enabled, it will not be disabled for the
rest of the life of the dialog.

R=amp@chromium.org, apacible@chromium.org
BUG=612969
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router WebUI] Disable search by default.

This change disables the search feature by default, only enabling it for
exceptionally long sink lists and when pseudo sinks are available.
Whether search is enabled is determined when the list of available sinks
is updated. Once it has been enabled, it will not be disabled for the
rest of the life of the dialog.

R=amp@chromium.org, apacible@chromium.org
BUG=612969
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4d46c6973164274ba1e67242eae30bbc9050fce0
Cr-Commit-Position: refs/heads/master@{#395128}
==========

[commit-bot: I haz the power, 2016-05-20 19:08:38]

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