[webkit-reviews] review denied: [Bug 28241] Media controls panel does not have a volume control slider : [Attachment 38512] controls implementation without theme

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Hin-Chung Lam <hclam at google.com>'s
request for review:
Bug 28241: Media controls panel does not have a volume control slider
https://bugs.webkit.org/show_bug.cgi?id=28241

Attachment 38512: controls implementation without theme
https://bugs.webkit.org/attachment.cgi?id=38512&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

Your changelog is out of date with your changes.  I found at least one function
mentioned in it that is no longer changed.
Also the changelog doesn't help me understand why functions were changed like
they were.  It is best if you describe this in the change log especially with a
a more complicated/bigger change like this. (See change log entries by others
like Darin Adler for example.)




> diff --git a/WebCore/rendering/MediaControlElements.cpp
b/WebCore/rendering/MediaControlElements.cpp
>  PassRefPtr<RenderStyle> MediaControlInputElement::styleForElement()
>  {
> -    return
m_mediaElement->renderer()->getCachedPseudoStyle(m_pseudoStyleId);
> +    PassRefPtr<RenderStyle> style =
m_mediaElement->renderer()->getCachedPseudoStyle(m_pseudoStyleId);
> +    return style;

Why is this being done?
PassRefPtr isn't suppose to be used as a local variable like this.  It would be
easiest to change the code back to what it was.


> +void MediaControlVolumeSliderElement::defaultEventHandler(Event* event)
> +{
...
> +    float volume = narrowPrecisionToFloat(value().toDouble());
> +    if (volume != m_mediaElement->volume()) {
> +	   ExceptionCode ec;
> +	   m_mediaElement->setVolume(volume, ec);

As Eric Seidel suggested: "Do we need to ASSERT(!ec)?"


> diff --git a/WebCore/rendering/RenderMediaControls.cpp
b/WebCore/rendering/RenderMediaControls.cpp
> index 06d901a..d9de8aa 100644
> --- a/WebCore/rendering/RenderMediaControls.cpp
> +++ b/WebCore/rendering/RenderMediaControls.cpp
> @@ -128,6 +128,18 @@ bool
RenderMediaControls::paintMediaControlsPart(MediaControlElementType part, R
>	   case MediaSliderThumb:
>	       paintThemePart(SafariTheme::MediaSliderThumbPart,
paintInfo.context->platformContext(), r, NSRegularControlSize,
determineState(o));
>	       break;
> +	   case MediaVolumeSliderContainer:
> +	       // FIXME: Implemnt volume slider.

typo: Implemnt (several places)