Fix controlling volume slider with keyboard

Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try to control
the volume slider by using keyboard. It increases/decreases the volume by
0.1 (or 10%) after each press. In the CL2281173002, it accidentally
changed this increment to an integer, so the volume either jumps to silent
or max.

BUG=631775
Committed: https://crrev.com/362963e633c27e32ecbd7a9f2bcd9752281dfbab
Cr-Commit-Position: refs/heads/master@{#415700}

[yiyix, 2016-08-30 19:27:23]

Description was changed from

==========
Fix controlling volume slider with keyboard

BUG=631775
==========

to

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used to Control the volume 
slider by using keyboard. It increase/decrease the volume by 0.1 (or 10%)
after each press. In the CL2281173002, it accidentally changed the number 
to an integer, so the volume either jumps to silent or max. 

BUG=631775
==========

[yiyix, 2016-08-30 19:27:23]

yiyix@chromium.org changed reviewers:
+ sadrul@chromium.org

[yiyix, 2016-08-30 19:28:58]

Description was changed from

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used to Control the volume 
slider by using keyboard. It increase/decrease the volume by 0.1 (or 10%)
after each press. In the CL2281173002, it accidentally changed the number 
to an integer, so the volume either jumps to silent or max. 

BUG=631775
==========

to

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try control 
the volume slider by using keyboard. It increase/decrease the volume by 
0.1 (or 10%) after each press. In the CL2281173002, it accidentally 
changed the number to an integer, so the volume either jumps to silent 
or max. 

BUG=631775
==========

[yiyix, 2016-08-30 19:30:19]

Description was changed from

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try control 
the volume slider by using keyboard. It increase/decrease the volume by 
0.1 (or 10%) after each press. In the CL2281173002, it accidentally 
changed the number to an integer, so the volume either jumps to silent 
or max. 

BUG=631775
==========

to

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try to control 
the volume slider by using keyboard. It increases/decreases the volume by 
0.1 (or 10%) after each press. In the CL2281173002, it accidentally 
changed this increment to an integer, so the volume either jumps to silent 
or max. 

BUG=631775
==========

[yiyix, 2016-08-30 19:30:39]

Could you please take a look? Thank you

[sadrul, 2016-08-30 19:32:31]

Can you add a test in slider_unittest.cc so this doesn't break again?

[yiyix, 2016-08-30 20:17:37]

On 2016/08/30 19:32:31, sadrul wrote:
> Can you add a test in slider_unittest.cc so this doesn't break again?

I am currently split slider into material design slider impl and non material
design slider impl. I am thinking to add the test in that cl.

[sadrul, 2016-08-30 20:19:50]

On 2016/08/30 20:17:37, yiyix wrote:
> On 2016/08/30 19:32:31, sadrul wrote:
> > Can you add a test in slider_unittest.cc so this doesn't break again?
> 
> I am currently split slider into material design slider impl and non material
> design slider impl. I am thinking to add the test in that cl.

Is the behaviour (i.e. response to key-event) going to be different in MD mode?
If not, you can add the test in this cl.

[yiyix, 2016-08-30 21:46:56]

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

[commit-bot: I haz the power, 2016-08-30 21:47:52]

Dry run: 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-30 22:27:19]

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

[commit-bot: I haz the power, 2016-08-30 22:27:20]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yiyix, 2016-08-31 15:27:59]

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

[commit-bot: I haz the power, 2016-08-31 15:28:14]

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

[sadrul, 2016-08-31 15:42:22]

lgtm

https://codereview.chromium.org/2297833002/diff/40001/ui/views/controls/slide...
File ui/views/controls/slider_unittest.cc (right):

https://codereview.chromium.org/2297833002/diff/40001/ui/views/controls/slide...
ui/views/controls/slider_unittest.cc:314: EXPECT_FLOAT_EQ(0.5,
slider()->value());
Instead of hardcoding the values, just check that the values have changed. You
can do that either using TestSliderListener, or perhaps more easily:

  float value = 0.5;
  slider()->SetValue(value);
  slider()->RequestFocus();
  press right
  EXPECT_NE(value, slider()->value());
  value = slider()->value();
  press left
  EXPECT_NE(value, slider()->value());

This way, your test does not depend on the actual value of
Slider::keyboard_increment_.

[commit-bot: I haz the power, 2016-08-31 16:02:32]

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

[commit-bot: I haz the power, 2016-08-31 16:02:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yiyix, 2016-08-31 16:12:52]

https://codereview.chromium.org/2297833002/diff/40001/ui/views/controls/slide...
File ui/views/controls/slider_unittest.cc (right):

https://codereview.chromium.org/2297833002/diff/40001/ui/views/controls/slide...
ui/views/controls/slider_unittest.cc:314: EXPECT_FLOAT_EQ(0.5,
slider()->value());
On 2016/08/31 15:42:22, sadrul wrote:
> Instead of hardcoding the values, just check that the values have changed. You
> can do that either using TestSliderListener, or perhaps more easily:
> 
>   float value = 0.5;
>   slider()->SetValue(value);
>   slider()->RequestFocus();
>   press right
>   EXPECT_NE(value, slider()->value());
>   value = slider()->value();
>   press left
>   EXPECT_NE(value, slider()->value());
> 
> This way, your test does not depend on the actual value of
> Slider::keyboard_increment_.

that's true... I can use ge and le to make sure it is moving to the right
direction.

If we use NE, it still won't catch the bug I had last time. As the volume jumps
to 0 after each press, the new value is still different than the old value.

[yiyix, 2016-08-31 17:25:50]

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

[commit-bot: I haz the power, 2016-08-31 17:26:12]

Dry run: 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-31 18:39:52]

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

[commit-bot: I haz the power, 2016-08-31 18:39:53]

Dry run: This issue passed the CQ dry run.

[yiyix, 2016-08-31 18:41:02]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2016-08-31 18:41:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2297833002/#ps60001
(title: "improve tests")

[commit-bot: I haz the power, 2016-08-31 18:41:59]

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-31 18:49:35]

Description was changed from

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try to control 
the volume slider by using keyboard. It increases/decreases the volume by 
0.1 (or 10%) after each press. In the CL2281173002, it accidentally 
changed this increment to an integer, so the volume either jumps to silent 
or max. 

BUG=631775
==========

to

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try to control 
the volume slider by using keyboard. It increases/decreases the volume by 
0.1 (or 10%) after each press. In the CL2281173002, it accidentally 
changed this increment to an integer, so the volume either jumps to silent 
or max. 

BUG=631775
==========

[commit-bot: I haz the power, 2016-08-31 18:49:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-31 18:52:42]

Description was changed from

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try to control 
the volume slider by using keyboard. It increases/decreases the volume by 
0.1 (or 10%) after each press. In the CL2281173002, it accidentally 
changed this increment to an integer, so the volume either jumps to silent 
or max. 

BUG=631775
==========

to

==========
Fix controlling volume slider with keyboard

In the current code base, a float number is used when users try to control
the volume slider by using keyboard. It increases/decreases the volume by
0.1 (or 10%) after each press. In the CL2281173002, it accidentally
changed this increment to an integer, so the volume either jumps to silent
or max.

BUG=631775

Committed: https://crrev.com/362963e633c27e32ecbd7a9f2bcd9752281dfbab
Cr-Commit-Position: refs/heads/master@{#415700}
==========

[commit-bot: I haz the power, 2016-08-31 18:52:44]

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