[webkit-reviews] review denied: [Bug 30177] Volume slider always starts at half volume : [Attachment 41621] volume slider patch (rev. 2)

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 30177: Volume slider always starts at half volume
https://bugs.webkit.org/show_bug.cgi?id=30177

Attachment 41621: volume slider patch (rev. 2)
https://bugs.webkit.org/attachment.cgi?id=41621&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> 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