Media Controls: invalidate volume slider when value changes.

Media Controls: invalidate volume slider when value changes.

Otherwise, the painting will not take into consideration the new
value.

BUG=677903
Review-Url: https://codereview.chromium.org/2616343004
Cr-Commit-Position: refs/heads/master@{#442864}
Committed: https://chromium.googlesource.com/chromium/src/+/e8a0940aae4094d975ccc14f7398154b80e8b42a

[mlamouri (slow - plz ping), 2017-01-09 13:32:54]

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

[commit-bot: I haz the power, 2017-01-09 13:33:07]

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

[mlamouri (slow - plz ping), 2017-01-09 13:33:56]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org

[mlamouri (slow - plz ping), 2017-01-09 13:33:57]

avayvod@, PTAL :)

This might require some tests rebasing but the change is simple enough. It's
following the same pattern that the timeline slider is using.

[commit-bot: I haz the power, 2017-01-09 14:54:17]

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

[commit-bot: I haz the power, 2017-01-09 14:54:17]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-09 16:06:59]

lgtm

https://codereview.chromium.org/2616343004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2616343004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:859: if
(value().toDouble() == volume)
nit: is it okay to compare doubles like this? I remember that doing something
like abs(a - b) < epsilon is better in general - do we have a method for that in
WTF or base and does it matter? (e.g. if the volume values are always within 0-1
range with 0.1 intervals, it shouldn't)

[whywhat, 2017-01-09 16:10:48]

Also, the change seems to be broader than the bug it fixes - I'd assume we could
just invalidate the volume bar when the muted state changes, not for every
change of the volume slider (presumably it invalidates itself already when this
happens, no?)

[mlamouri (slow - plz ping), 2017-01-10 14:13:19]

Regarding the other comment, invalidating the object for paint if it was already
invalidated would be a no-op so I don't think it should be an issue. Though, I
don't think it's done properly. For example, if you look into it, you will see
that moving the slider will not update the blue/dark area until you actually
commit a value change which means the widget doesn't seem to do paint
invalidation.

Note that the paint invalidation is required because we do have painting logic
that defines the colour. I wonder if we should avoid this and use CSS instead.
Though, I wouldn't know how to do it :)

https://codereview.chromium.org/2616343004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2616343004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:859: if
(value().toDouble() == volume)
On 2017/01/09 at 16:06:59, whywhat wrote:
> nit: is it okay to compare doubles like this? I remember that doing something
like abs(a - b) < epsilon is better in general - do we have a method for that in
WTF or base and does it matter? (e.g. if the volume values are always within 0-1
range with 0.1 intervals, it shouldn't)

In general, we can assume that any change is meaningful. There might be an
epsilon we could use but we could easily end up with issues such as many epsilon
changes leading to a large change we have been ignoring.

[mlamouri (slow - plz ping), 2017-01-10 14:13:24]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-01-10 14:13:32]

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, 2017-01-10 15:52:10]

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

[commit-bot: I haz the power, 2017-01-10 15:52:11]

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...)

[mlamouri (slow - plz ping), 2017-01-11 10:14:43]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-01-11 10:15:04]

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, 2017-01-11 10:44:32]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484129683434810, "parent_rev":
"7caec88b05daa5caec5c923ff60d84e96a98f8ff", "commit_rev":
"e8a0940aae4094d975ccc14f7398154b80e8b42a"}

[commit-bot: I haz the power, 2017-01-11 10:45:03]

Description was changed from

==========
Media Controls: invalidate volume slider when value changes.

Otherwise, the painting will not take into consideration the new
value.

BUG=677903
==========

to

==========
Media Controls: invalidate volume slider when value changes.

Otherwise, the painting will not take into consideration the new
value.

BUG=677903

Review-Url: https://codereview.chromium.org/2616343004
Cr-Commit-Position: refs/heads/master@{#442864}
Committed:
https://chromium.googlesource.com/chromium/src/+/e8a0940aae4094d975ccc14f7398...
==========

[commit-bot: I haz the power, 2017-01-11 10:45:04]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/e8a0940aae4094d975ccc14f7398...