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.
Created attachment 40807 [details] Sets volume to 1
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.
Only chromium port has implemented the volume slider and is disabled on all other ports, so we don't need to update test results.
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?
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.
> 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.
What about I port the test from chromium to here and have it disabled by default. (And chromium enables it)
Comment on attachment 40807 [details] Sets volume to 1 > + m_volumeSlider->setAttribute(valueAttr, "1"); What if the initial volume is not 1?
What about changing it to read from .volume property?
Sounds like a plan ;-)
Created attachment 41621 [details] volume slider patch (rev. 2)
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 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
Created attachment 41694 [details] Volume slider patch (rev. 3)
Skipped the test and checks the old value when updating the volume slider control.
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 :-(
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 on attachment 41744 [details] Volume slider patch (rev. 4) r=me Thanks!
Comment on attachment 41744 [details] Volume slider patch (rev. 4) Clearing flags on attachment: 41744 Committed r49995: <http://trac.webkit.org/changeset/49995>
All reviewed patches have been landed. Closing bug.