MD Settings: cr-slider: Make display consistent and clean up.

MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

Committed: https://crrev.com/26b1599ba3f24f4124d8d78227fb815dc1c2e696
Cr-Commit-Position: refs/heads/master@{#440785}

[stevenjb, 2016-12-20 00:21:23]

Description was changed from

==========
MD Settings: cr-slider: Make display consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

BUG=598897
==========

to

==========
MD Settings: cr-slider: Make display consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

[stevenjb, 2016-12-20 00:45:53]

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

[stevenjb, 2016-12-22 02:00:13]

stevenjb@chromium.org changed reviewers:
+ dschuyler@chromium.org

[stevenjb, 2016-12-22 02:00:48]

Description was changed from

==========
MD Settings: cr-slider: Make display consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

to

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

[dschuyler, 2016-12-22 02:02:54]

On 2016/12/22 02:00:13, stevenjb wrote:

I was looking to the issue for screen shots as suggested, but I think the bug=
may be incorrect.

[dschuyler, 2016-12-22 02:36:36]

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:56: /** @private
{!Array<number>} Mode index values for slider. */
This looks like a departure from the common style, which would be:

/**
 * Mode index values for slider. 
 * @private {!Array<number>}
 */

grepping in settings/ found 1323 hits for @private. Only one of those, in
passwords_and_forms_page.js uses the style on line 56 :)

If you do want to change the way we are commenting @private, I'm not against
that. I just thought this might be a whoops or "didn't notice that" kind of
thing.

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:64: if
(this.$.slider.dragging && this.tickValues.length > 0 &&
I'm not seeing why the this.tickValues.length > 0 are being added here and
elsewhere. Would having a length <= 0 just be a developer error, or could this
happen normally at run-time?

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:76: // get called with an
undefined value.
Can the observer on line 39 be altered to account for this (so that this check
becomes unnecessary)?

[stevenjb, 2016-12-22 02:46:03]

Description was changed from

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

to

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

[stevenjb, 2016-12-22 02:55:25]

Sorry about the bogus issue #, I fixed that.

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:56: /** @private
{!Array<number>} Mode index values for slider. */
On 2016/12/22 02:36:35, dschuyler wrote:
> This looks like a departure from the common style, which would be:
> 
> /**
>  * Mode index values for slider. 
>  * @private {!Array<number>}
>  */
> 
> grepping in settings/ found 1323 hits for @private. Only one of those, in
> passwords_and_forms_page.js uses the style on line 56 :)
> 
> If you do want to change the way we are commenting @private, I'm not against
> that. I just thought this might be a whoops or "didn't notice that" kind of
> thing.

It's shorthand, used more commonly when the type isn't needed, but as you
noticed not entirely without precedent. I generally prefer short when it's
clear.

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:64: if
(this.$.slider.dragging && this.tickValues.length > 0 &&
On 2016/12/22 02:36:35, dschuyler wrote:
> I'm not seeing why the this.tickValues.length > 0 are being added here and
> elsewhere. Would having a length <= 0 just be a developer error, or could this
> happen normally at run-time?

It can happen briefly in display.js:157 when the list of modes has not
initialized. It seemed better to handle it here then requiring some slight of
hand in any client that has a similar problem (even though that is likely).

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:76: // get called with an
undefined value.
On 2016/12/22 02:36:35, dschuyler wrote:
> Can the observer on line 39 be altered to account for this (so that this check
> becomes unnecessary)?

I'm not entirely sure why this gets called with value == undefined to be honest.
I didn't think that was possible with the way the observer is defined, but it
happened which is why I added this. Sometimes Polymer baffles me.

[michaelpg, 2016-12-22 07:31:32]

This is complicated and I'm on vacation, so feel free to use dschuyler's
approval and not wait for mine.

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:56: /** @private
{!Array<number>} Mode index values for slider. */
On 2016/12/22 02:55:25, stevenjb wrote:
> On 2016/12/22 02:36:35, dschuyler wrote:
> > This looks like a departure from the common style, which would be:
> > 
> > /**
> >  * Mode index values for slider. 
> >  * @private {!Array<number>}
> >  */
> > 
> > grepping in settings/ found 1323 hits for @private. Only one of those, in
> > passwords_and_forms_page.js uses the style on line 56 :)

We must be searching differently, because I found a settings hit (only) in
keyboard.js:

\/\*\*\s@private\s{.*}\s[^*]

> > 
> > If you do want to change the way we are commenting @private, I'm not against
> > that. I just thought this might be a whoops or "didn't notice that" kind of
> > thing.
> 
> It's shorthand, used more commonly when the type isn't needed, but as you
> noticed not entirely without precedent. I generally prefer short when it's
> clear.

+1. I'm surprised we have so little of this; I thought I used it commonly myself
but I guess not? Also a small amount of precedent for the substantially similar
@type annotation:

/** @type {foo} Foo fighters. */

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:161: this.modeValues_ =
Array.from(Array(numModes).keys());
I think I have a TODO somewhere to add this to cr-slider directly so we can just
say "use values as ticks" instead of doing all this crap. Not that we have to
fix it now.

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:320: if
(this.currentSelectedModeIndex_ === undefined ||
since we remove the listener in detached(), do we need to also clear
this.currentSelectedModeIndex_ there for this logic to still work after another
attach()?

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right):

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:27: /* Make the slider
bar always grey */
nit: ending period

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:76: // get called with an
undefined value.
On 2016/12/22 02:55:25, stevenjb wrote:
> On 2016/12/22 02:36:35, dschuyler wrote:
> > Can the observer on line 39 be altered to account for this (so that this
check
> > becomes unnecessary)?
> 
> I'm not entirely sure why this gets called with value == undefined to be
honest.
> I didn't think that was possible with the way the observer is defined, but it
> happened which is why I added this. Sometimes Polymer baffles me.

i thought multi-property observers are called when a dependent parameter changes
from not-undefined to undefined. Just not when one property is undefined, and
another property changes.

Line 49 could be setting this.value to |undefined| if immediateValue can be
invalid (maybe paper-sider sets it to -1 in some cases?)

If that's what's happening then the comment is wrong. If you logged/stepped thru
and found that this really is getting called before |value| is *ever* set, then
damn.

[stevenjb, 2016-12-22 19:21:50]

On 2016/12/22 07:31:32, michaelpg (OOO) wrote:
> This is complicated and I'm on vacation, so feel free to use dschuyler's
> approval and not wait for mine.
> 

You shouldn't be doing code reviews while on vacation :P None of these are
urgent. I did add Dave to go ahead and push this one forward though, but thanks
for the feedback.

[stevenjb, 2016-12-22 20:42:18]

PTAL

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:161: this.modeValues_ =
Array.from(Array(numModes).keys());
On 2016/12/22 07:31:32, michaelpg (OOO) wrote:
> I think I have a TODO somewhere to add this to cr-slider directly so we can
just
> say "use values as ticks" instead of doing all this crap. Not that we have to
> fix it now.

You do, and I thought about doing it that way, but in order for it to be robust
it has to be kind of complicated (e.g. we have one slider that starts at 0 and
one that starts at 1, so it needs min and max). Given how few sliders we have
(and this one may go away), it didn't seem worth the bother.

https://codereview.chromium.org/2587913007/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:320: if
(this.currentSelectedModeIndex_ === undefined ||
On 2016/12/22 07:31:32, michaelpg (OOO) wrote:
> since we remove the listener in detached(), do we need to also clear
> this.currentSelectedModeIndex_ there for this logic to still work after
another
> attach()?

That's probably a good idea. Done.

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right):

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:27: /* Make the slider
bar always grey */
On 2016/12/22 07:31:32, michaelpg (OOO) wrote:
> nit: ending period

Done.

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:76: // get called with an
undefined value.
On 2016/12/22 07:31:32, michaelpg (OOO) wrote:
> On 2016/12/22 02:55:25, stevenjb wrote:
> > On 2016/12/22 02:36:35, dschuyler wrote:
> > > Can the observer on line 39 be altered to account for this (so that this
> check
> > > becomes unnecessary)?
> > 
> > I'm not entirely sure why this gets called with value == undefined to be
> honest.
> > I didn't think that was possible with the way the observer is defined, but
it
> > happened which is why I added this. Sometimes Polymer baffles me.
> 
> i thought multi-property observers are called when a dependent parameter
changes
> from not-undefined to undefined. Just not when one property is undefined, and
> another property changes.
> 
> Line 49 could be setting this.value to |undefined| if immediateValue can be
> invalid (maybe paper-sider sets it to -1 in some cases?)
> 
> If that's what's happening then the comment is wrong. If you logged/stepped
thru
> and found that this really is getting called before |value| is *ever* set,
then
> damn.

Yeah, you're right, this really shouldn't be happening, and it doesn't appear to
be happening any more. Maybe I was confused when it did, or seemed to. Removed.

[dschuyler, 2016-12-22 22:09:45]

LGTM with comment on tick count limit.

https://codereview.chromium.org/2587913007/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:61: this.$.slider.snaps =
numTicks < 10;
Please make a comment about 10 being an aesthetic design choice. (vs. being a
bug workaround or similar). Maybe something like one of these:

// The 10 ticks limit is an aesthetic choice.

// Avoid making the slider too busy looking by limiting the tick count.

// Limiting to 10 tick marks looks good.

[dschuyler, 2016-12-22 22:17:34]

https://codereview.chromium.org/2587913007/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:61: this.$.slider.snaps =
numTicks < 10;
On 2016/12/22 22:09:45, dschuyler wrote:
> Please make a comment about 10 being an aesthetic design choice. (vs. being a
> bug workaround or similar). Maybe something like one of these:
> 
> // The 10 ticks limit is an aesthetic choice.
> 
> // Avoid making the slider too busy looking by limiting the tick count.
> 
> // Limiting to 10 tick marks looks good.

Or maybe make a var like (which wouldn't need a comment imo because the name
tells me what's up):

var aestheticTickCountLimit = 10;

[stevenjb, 2016-12-23 00:18:33]

Patchset #6 (id:100001) has been deleted

[stevenjb, 2016-12-23 00:18:38]

https://codereview.chromium.org/2587913007/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2587913007/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:61: this.$.slider.snaps =
numTicks < 10;
On 2016/12/22 22:17:34, dschuyler wrote:
> On 2016/12/22 22:09:45, dschuyler wrote:
> > Please make a comment about 10 being an aesthetic design choice. (vs. being
a
> > bug workaround or similar). Maybe something like one of these:
> > 
> > // The 10 ticks limit is an aesthetic choice.
> > 
> > // Avoid making the slider too busy looking by limiting the tick count.
> > 
> > // Limiting to 10 tick marks looks good.
> 
> Or maybe make a var like (which wouldn't need a comment imo because the name
> tells me what's up):
> 
> var aestheticTickCountLimit = 10;

So many choices :) Going with a slightly konstant + comment.

[stevenjb, 2016-12-23 00:18:42]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-23 00:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2587913007/#ps120001
(title: "More Feedback")

[commit-bot: I haz the power, 2016-12-23 00:19:00]

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-12-23 01:05:30]

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

[commit-bot: I haz the power, 2016-12-23 01:05:31]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[stevenjb, 2016-12-27 19:22:57]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-27 19:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2587913007/#ps140001
(title: "Update tests")

[commit-bot: I haz the power, 2016-12-27 19:23:10]

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-12-27 20:53:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482866577899880,
"parent_rev": "03ca7ec3a0fd270c915a85a46f7aec65e5b2bc48", "commit_rev":
"bf9f72fd2b0f9ec85519c93fdf8fb3b51acfb372"}

[commit-bot: I haz the power, 2016-12-27 20:53:33]

Description was changed from

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

to

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

Review-Url: https://codereview.chromium.org/2587913007
==========

[commit-bot: I haz the power, 2016-12-27 20:53:34]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2017-01-02 15:47:03]

Description was changed from

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

Review-Url: https://codereview.chromium.org/2587913007
==========

to

==========
MD Settings: cr-slider: Make slider appearance consistent and clean up.

This CL:
* Switches display.html to use cr-slider (was only use of paper-slider)
* Changes styling of cr-slider as follows:
** Slider bar is grey instead of active (left) portion being blue.
** Knob is always blue (even at leftmost position)
** Ticks are shown automatically if there are < 10
** Labels are shown as part of cr-slider and shown below the slider

See issue for screenshots.

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

Committed: https://crrev.com/26b1599ba3f24f4124d8d78227fb815dc1c2e696
Cr-Commit-Position: refs/heads/master@{#440785}
==========

[commit-bot: I haz the power, 2017-01-02 15:47:05]

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