[Webkit-unassigned] [Bug 28241] Media controls panel does not have a volume control slider

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28241


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38512|review?                     |review-
               Flag|                            |




--- Comment #18 from David Levin <levin at chromium.org>  2009-08-24 18:32:34 PDT ---
(From update of attachment 38512)
> 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)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.