Bug 30177

Summary: Volume slider always starts at half volume
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: MediaAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, eric.carlson, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Description Flags
Sets volume to 1
eric.carlson: review-
volume slider patch (rev. 2)
eric.carlson: review-
Volume slider patch (rev. 3)
eric.carlson: review-
Volume slider patch (rev. 4) none

Description Hin-Chung Lam 2009-10-07 11:25:10 PDT
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.
Comment 1 Hin-Chung Lam 2009-10-07 11:42:15 PDT
Created attachment 40807 [details]
Sets volume to 1
Comment 2 Eric Seidel (no email) 2009-10-07 12:09:28 PDT
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.
Comment 3 Hin-Chung Lam 2009-10-07 12:40:30 PDT
Only chromium port has implemented the volume slider and is disabled on all other ports, so we don't need to update test results.
Comment 4 Eric Seidel (no email) 2009-10-15 13:34:53 PDT
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?
Comment 5 Hin-Chung Lam 2009-10-15 15:08:22 PDT
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.
Comment 6 Eric Carlson 2009-10-19 12:19:42 PDT
> 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.
Comment 7 Hin-Chung Lam 2009-10-19 12:23:20 PDT
What about I port the test from chromium to here and have it disabled by default. (And chromium enables it)
Comment 8 Eric Carlson 2009-10-19 12:25:21 PDT
Comment on attachment 40807 [details]
Sets volume to 1

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

What if the initial volume is not 1?
Comment 9 Hin-Chung Lam 2009-10-19 12:30:53 PDT
What about changing it to read from .volume property?
Comment 10 Eric Carlson 2009-10-19 12:31:22 PDT
Sounds like a plan ;-)
Comment 11 Hin-Chung Lam 2009-10-21 16:46:50 PDT
Created attachment 41621 [details]
volume slider patch (rev. 2)
Comment 12 Hin-Chung Lam 2009-10-21 16:51:23 PDT
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.
Comment 13 Eric Carlson 2009-10-22 06:24:21 PDT
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
Comment 14 Hin-Chung Lam 2009-10-22 16:20:05 PDT
Created attachment 41694 [details]
Volume slider patch (rev. 3)
Comment 15 Hin-Chung Lam 2009-10-22 16:43:58 PDT
Skipped the test and checks the old value when updating the volume slider control.
Comment 16 Eric Carlson 2009-10-23 10:25:57 PDT
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) {

Sorry to be so picky :-(
Comment 17 Hin-Chung Lam 2009-10-23 13:17:40 PDT
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.
Comment 18 Eric Carlson 2009-10-23 13:24:50 PDT
Comment on attachment 41744 [details]
Volume slider patch (rev. 4)


Comment 19 WebKit Commit Bot 2009-10-23 13:35:31 PDT
Comment on attachment 41744 [details]
Volume slider patch (rev. 4)

Clearing flags on attachment: 41744

Committed r49995: <http://trac.webkit.org/changeset/49995>
Comment 20 WebKit Commit Bot 2009-10-23 13:35:41 PDT
All reviewed patches have been landed.  Closing bug.