Bug 30177 – Volume slider always starts at half volume

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Hin-Chung Lam]

Make a page with:

<video src="somevideo.ogg" controls></video> in Chromium.

When you mouse over the volume icon, volume slider always starts at 0.5 volume.

[Hin-Chung Lam]

Created attachment 40807 [details]
Sets volume to 1

[Eric Seidel (no email)]

Comment on attachment 40807 [details]
Sets volume to 1

If this were covered by existing tests, we would need update test results, no?  Doesn't this affect other ports?  Or only Chromium port?  It's not clear why this change would only affect the chromium port.

[Hin-Chung Lam]

Only chromium port has implemented the volume slider and is disabled on all other ports, so we don't need to update test results.

[Eric Seidel (no email)]

So when this is checked in there will need to be a corresponding change to Chromium's repository to update Chromium test results?  Have you prepared that code review?  Could you link to it from here?

[Hin-Chung Lam]

The test has to go into chromium's repository, review is here: http://codereview.chromium.org/269100

Reasons are:
1. Volume slider is only implemented in chromium
2. Testing this require knowing the specific implementation and position of the volume slider

This is not a manual test because:
Manual test doesn't verify the render tree nor it has pixel results.

[Eric Carlson]

> 2. Testing this require knowing the specific implementation and position of the 
> volume slider

You could write a pixel test, though they have their own set of issues.

[Hin-Chung Lam]

What about I port the test from chromium to here and have it disabled by default. (And chromium enables it)

[Eric Carlson]

Comment on attachment 40807 [details]
Sets volume to 1

> +    m_volumeSlider->setAttribute(valueAttr, "1");

What if the initial volume is not 1?

[Hin-Chung Lam]

What about changing it to read from .volume property?

[Eric Carlson]

Sounds like a plan ;-)

[Hin-Chung Lam]

Created attachment 41621 [details]
volume slider patch (rev. 2)

[Hin-Chung Lam]

Moving a pixel test from chromium repository. Unfortunately the test doesn't test anything if the volume slider is not implemented.

Also found a related problem, that changing the volume attribute after the controls is enabled doesn't move the thumb of the video control slider. The fix is included in this patch since it's closely related.

[Eric Carlson]

Comment on attachment 41621 [details]
volume slider patch (rev. 2)

> LayoutTests/media/video-volume-slider.html

Committing this patch as-is will break the build because the controller UI on SnowLeopard is different from that on all other ports. I really appreciate that you went to the extra effort to create this test with results for other ports, but I don't think it helps to have the test run just yet because it will be a PIYA to get the correct results and none of the other ports implements the slider yet. I don't want to lose the test because we will add a volume slider, so I would like to see it committed but added to the skip lists.


> +void MediaControlVolumeSliderElement::update()
> +{
> +    setValue(String::number(m_mediaElement->volume()));
> +    MediaControlInputElement::update();
> +}

setValue always causes a renderer update, so it would be more efficient to only call it when the value is out of sync with the element's volume.

r- for now

[Hin-Chung Lam]

Created attachment 41694 [details]
Volume slider patch (rev. 3)

[Hin-Chung Lam]

Skipped the test and checks the old value when updating the volume slider control.

[Eric Carlson]

Comment on attachment 41694 [details]
Volume slider patch (rev. 3)


> +    String newValue = String::number(m_mediaElement->volume());
> +    if (value() != newValue) {
> +        setValue(newValue);

String comparison is slower than comparing floats and it seems wasteful to allocate a String when it won't be used, so I would like to see something like the following instead:

   float volume = m_mediaElement->volume();
   if (value().toFloat() != volume) {
        setValue(String::number(volume));

Sorry to be so picky :-(

[Hin-Chung Lam]

Created attachment 41744 [details]
Volume slider patch (rev. 4)

That's a good point! :)

Here's the new patch, that we compares a float first.

[Eric Carlson]

Comment on attachment 41744 [details]
Volume slider patch (rev. 4)

r=me

Thanks!

[WebKit Commit Bot]

Comment on attachment 41744 [details]
Volume slider patch (rev. 4)

Clearing flags on attachment: 41744

Committed r49995: <http://trac.webkit.org/changeset/49995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.