[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





--- Comment #21 from Hin-Chung Lam <hclam at google.com>  2009-08-24 21:39:45 PDT ---
(In reply to comment #18)
> (From update of attachment 38512 [details])
> > 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.)
> 
> 

I think a bunch of them not useful because of the empty spaces I removed, I'll
remove those cleanup and will add some comments to the change log.

> 
> 
> > 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.
> 

Will revert this.

> 
> > +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)?"
> 

Will add this back.

> 
> > 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)

Will also correct this.

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