Bug 28241 – Media controls panel does not have a volume control slider

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Hin-Chung Lam]

Open a page with <video controls></video> in WebKit, there should be a volume control slider. It may not need to be turned on by default since WebKit mac port uses the quick time layout, but it should be at least configurable via css.

[Hin-Chung Lam]

Created attachment 38358 [details]
implemented the control, css style, logic for showing but no appearance

[Hin-Chung Lam]

Created attachment 38360 [details]
added controls, style and implementation of controls but no theme

[Eric Carlson]

> +void RenderMedia::updateVolumeSliderContainer(bool visible) {

Brace should be on a new line


> +    } else if (!visible) {
> +        m_volumeSliderContainer->setVisible(false);
> +        m_volumeSlider->update();
> +    }

Why call setVisible(false) if it is already not visible?


>  void RenderMedia::forwardEvent(Event* event)
>  {
>      if (event->isMouseEvent() && m_controlsShadowRoot) {
>          MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
>          IntPoint point(mouseEvent->absoluteLocation());
> -        if (m_muteButton && m_muteButton->hitTest(point))
> +        bool showVolumeSlider = false;

This breaks the current mute button. Why don't you use a compile switch
for the new code so ports can opt into when they are ready.

[Hin-Chung Lam]

Created attachment 38395 [details]
added controls, style and implementation of controls but no theme

[Hin-Chung Lam]

I have updated the patch addressing Eric's comments. About the comments that mute button will break, the line "if (m_muteButton && m_muteButton->hitTest(point))" is moved to line 525. I tested this on a mac and the mute button works fine.

The volume slider is shown above or below the mute button (depending on the position of the mute button in the frame). It doesn't overlay on top. And if the port doesn't implement the appearance of the volume slider, that this patch doesn't include. The slider won't be shown at all.

[Hin-Chung Lam]

To add a bit more about the behavior of the volume slider:

- When mouse over the mute button
- Volume slider is shown either above the mute button or below the mute button (They will not overlap)
- When mouse out of the volume slider or mute button, the volume slider will disappear

The behavior of the volume slider will be similar to youtube.

Also in this patch I don't include any theme implementation and by default -webkit-media-controls-volume-slider-container and -webkit-media-controls-volume-slider don't have renderers unless they are defined in css with a "display" value so they won't affect layout tests nor shown.

[Eric Carlson]

This looks great to me, thanks Alpha!

I think this is ready to land, but I won't r+ it as I am not a reviewer.

[Eric Seidel (no email)]

Comment on attachment 38395 [details]
added controls, style and implementation of controls but no theme

Strange to have WebCore:: on one and not the other:
03     if (m_isVisible)
 204         style->setVisibility(VISIBLE);
 205     else
 206         style->setVisibility(WebCore::HIDDEN);

I might have even written that style->setVisibility(m_visible ? VISIBLE : HIDDEN)

Same in the next function below.

Don't we only need to clone styles when they're not already shared?
1     RefPtr<RenderStyle> style = RenderStyle::clone(renderer()->style());
 222     if (visible)
 223         style->setVisibility(VISIBLE);
 224     else
 225         style->setVisibility(WebCore::HIDDEN);
Or maybe clone() is smart enough to do that.

I don't understand this comment:
216     // This function is used during the RenderMedia::layout()
 217     // call, where we cannot change the renderer at this time.
 218     if (!renderer() || !renderer()->style())
 219         return;

We should figure out if we always need to be cloning, or if there is a similar "make sure this style is not shared" method:
 236     RefPtr<RenderStyle> style = RenderStyle::clone(renderer()->style());

Why not just "1"?
 598     setAttribute(maxAttr, String::number(1));

Sometimes we have exception ignoring versions of dom methods which are only used internally:
614         ExceptionCode ec;
 615         m_mediaElement->setVolume(volume, ec);
I'm surprise we don't have one here.
Do we need to ASSERT(!ec)?

Isn't this doing a bunch of extra checks?
21     return MediaControlInputElement::rendererIsNeeded(style) && parent() && parent()->renderer() && parent()->renderer()->style()->visibility() == VISIBLE;
RenderObject::rendererIsNeeded() takes care of all of that, no?  Why do you need to override?

Its a little sad that we have so many classes in these files.

I'm not sure I fully understand:
 494 void RenderMedia::updateVolumeSliderContainer(bool visible)
Seems like a lot of code. ;)

In general this looks fine, but there are a couple unanswered questions above.

[Eric Seidel (no email)]

I'm most concerned with the rendererIsNeeded implementation, and the possibly unecessary clone() calls.

[Eric Carlson]

> I don't understand this comment:
>  216     // This function is used during the RenderMedia::layout()
>  217     // call, where we cannot change the renderer at this time.
>  218     if (!renderer() || !renderer()->style())
>  219         return;

Looks like the comment was copy/pasted from MediaControlTimeDisplayElement::setVisible and isn't relevant here.


> Isn't this doing a bunch of extra checks?
> 21     return MediaControlInputElement::rendererIsNeeded(style) && parent() &&
> parent()->renderer() && parent()->renderer()->style()->visibility() == VISIBLE;
> RenderObject::rendererIsNeeded() takes care of all of that, no?  Why do you
> need to override?

I don't see RenderObject::rendererIsNeeded? MediaControlInputElement::rendererIsNeeded does check "parent() && parent()->renderer()" so those aren't needed, but it doesn't check the visibility of the parent (the slider container). The question about  whether or not this is needed at all is a fair one though. The other media control elements that override rendererIsNeeded do so because we want to force a re-layout when they appear/disappear, which is not the case with the volume slider since it is position absolute.


> Sometimes we have exception ignoring versions of dom methods which are only
> used internally:
>  614         ExceptionCode ec;
>  615         m_mediaElement->setVolume(volume, ec);
> I'm surprise we don't have one here.

This is the first internal use of setVolume, it seems a bit heavyweight to add a convenience method just yet.

[Hin-Chung Lam]

(In reply to comment #8)
> (From update of attachment 38395 [details])
> Strange to have WebCore:: on one and not the other:
> 03     if (m_isVisible)
>  204         style->setVisibility(VISIBLE);
>  205     else
>  206         style->setVisibility(WebCore::HIDDEN);
> 
> I might have even written that style->setVisibility(m_visible ? VISIBLE :
> HIDDEN)
> 
> Same in the next function below.

Will change this.

> 
> Don't we only need to clone styles when they're not already shared?
> 1     RefPtr<RenderStyle> style = RenderStyle::clone(renderer()->style());
>  222     if (visible)
>  223         style->setVisibility(VISIBLE);
>  224     else
>  225         style->setVisibility(WebCore::HIDDEN);
> Or maybe clone() is smart enough to do that.

I'll take out the extra clone()s.

> 
> I don't understand this comment:
> 216     // This function is used during the RenderMedia::layout()
>  217     // call, where we cannot change the renderer at this time.
>  218     if (!renderer() || !renderer()->style())
>  219         return;
> 
> We should figure out if we always need to be cloning, or if there is a similar
> "make sure this style is not shared" method:
>  236     RefPtr<RenderStyle> style = RenderStyle::clone(renderer()->style());
> 
> Why not just "1"?
>  598     setAttribute(maxAttr, String::number(1));
> 
> Sometimes we have exception ignoring versions of dom methods which are only
> used internally:
> 614         ExceptionCode ec;
>  615         m_mediaElement->setVolume(volume, ec);
> I'm surprise we don't have one here.
> Do we need to ASSERT(!ec)?
> 
> Isn't this doing a bunch of extra checks?
> 21     return MediaControlInputElement::rendererIsNeeded(style) && parent() &&
> parent()->renderer() && parent()->renderer()->style()->visibility() == VISIBLE;
> RenderObject::rendererIsNeeded() takes care of all of that, no?  Why do you
> need to override?

The reason was that after the volume container goes away, the slider is still shown. Instead of changing the visiblity of also the slider I took out it's renderer. But this is not needed any more, I'll makes its parent goes away instead so both of them would have renderer.

> 
> Its a little sad that we have so many classes in these files.
> 
> I'm not sure I fully understand:
>  494 void RenderMedia::updateVolumeSliderContainer(bool visible)
> Seems like a lot of code. ;)
> 
> In general this looks fine, but there are a couple unanswered questions above.

[Hin-Chung Lam]

Thanks for the review!

(In reply to comment #10)
> > I don't understand this comment:
> >  216     // This function is used during the RenderMedia::layout()
> >  217     // call, where we cannot change the renderer at this time.
> >  218     if (!renderer() || !renderer()->style())
> >  219         return;
> 
> Looks like the comment was copy/pasted from
> MediaControlTimeDisplayElement::setVisible and isn't relevant here.
> 
> 
> > Isn't this doing a bunch of extra checks?
> > 21     return MediaControlInputElement::rendererIsNeeded(style) && parent() &&
> > parent()->renderer() && parent()->renderer()->style()->visibility() == VISIBLE;
> > RenderObject::rendererIsNeeded() takes care of all of that, no?  Why do you
> > need to override?
> 
> I don't see RenderObject::rendererIsNeeded?
> MediaControlInputElement::rendererIsNeeded does check "parent() &&
> parent()->renderer()" so those aren't needed, but it doesn't check the
> visibility of the parent (the slider container). The question about  whether or
> not this is needed at all is a fair one though. The other media control
> elements that override rendererIsNeeded do so because we want to force a
> re-layout when they appear/disappear, which is not the case with the volume
> slider since it is position absolute.
> 

The volume slider container is absolute positioned, so does the volume slider, so simply setting the visibility of the container won't make the slider goes away. It was to make the renderer of the volume slider goes away when the container is not visible. I'll now change this so that the volume container's renderer would go away, and thus volume slider's renderer would be detached too.

> 
> > Sometimes we have exception ignoring versions of dom methods which are only
> > used internally:
> >  614         ExceptionCode ec;
> >  615         m_mediaElement->setVolume(volume, ec);
> > I'm surprise we don't have one here.
> 
> This is the first internal use of setVolume, it seems a bit heavyweight to add
> a convenience method just yet.

[Hin-Chung Lam]

Created attachment 38512 [details]
controls implementation without theme

[Hin-Chung Lam]

Created attachment 38513 [details]
volume slider theme implementation for chromium port

[Hin-Chung Lam]

Updated implementations in MediaControlElements.cpp to get rid of RenderStyle::Clone() and removed MediaControlVolumeSliderContainerElement::rendererIsNeeded().

[Andrew Scherkus]

> diff --git a/WebCore/rendering/RenderThemeChromiumSkia.cpp b/WebCore/rendering/RenderThemeChromiumSkia.cpp
> index c8875dc..d66a4af 100644
> --- a/WebCore/rendering/RenderThemeChromiumSkia.cpp
> +++ b/WebCore/rendering/RenderThemeChromiumSkia.cpp
> @@ -571,6 +571,28 @@ bool RenderThemeChromiumSkia::paintMediaSliderTrack(RenderObject* object, const
>  #endif
>  }
>  
> +bool RenderThemeChromiumSkia::paintMediaVolumeSliderTrack(RenderObject* object, const RenderObject::PaintInfo& paintInfo, const IntRect& rect)
> +{
> +#if ENABLE(VIDEO)
> +    HTMLMediaElement* mediaElement = mediaElementParent(object->node());
> +    if (!mediaElement)
> +        return false;

You should probably check hasAudio() here unless you can guarantee this is never called if/when hasAudio() is false

[Hin-Chung Lam]

(In reply to comment #16)
> > diff --git a/WebCore/rendering/RenderThemeChromiumSkia.cpp b/WebCore/rendering/RenderThemeChromiumSkia.cpp
> > index c8875dc..d66a4af 100644
> > --- a/WebCore/rendering/RenderThemeChromiumSkia.cpp
> > +++ b/WebCore/rendering/RenderThemeChromiumSkia.cpp
> > @@ -571,6 +571,28 @@ bool RenderThemeChromiumSkia::paintMediaSliderTrack(RenderObject* object, const
> >  #endif
> >  }
> >  
> > +bool RenderThemeChromiumSkia::paintMediaVolumeSliderTrack(RenderObject* object, const RenderObject::PaintInfo& paintInfo, const IntRect& rect)
> > +{
> > +#if ENABLE(VIDEO)
> > +    HTMLMediaElement* mediaElement = mediaElementParent(object->node());
> > +    if (!mediaElement)
> > +        return false;
> 
> You should probably check hasAudio() here unless you can guarantee this is
> never called if/when hasAudio() is false

This check should go into MediaControlElements.cpp, I'll include the change in this patch.

[David Levin]

Comment on attachment 38512 [details]
controls implementation without theme

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

[David Levin]

Comment on attachment 38513 [details]
volume slider theme implementation for chromium port

Per Andrew's comment.

[Hin-Chung Lam]

(In reply to comment #19)
> (From update of attachment 38513 [details])
> Per Andrew's comment.

The issue Andrew pointed out should be addressed in RenderMedia.cpp rather than in the theme implementation of chromium port. I'll include the fix in RenderMedia.cpp.

[Hin-Chung Lam]

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

[Hin-Chung Lam]

Created attachment 38529 [details]
volume controls implementation without theme

[Hin-Chung Lam]

Created attachment 38530 [details]
volume controls implementation without theme

[Hin-Chung Lam]

Updated the patch for basic implementation of volume slider control.

Re-activate the patch for chromium port's theme implementation since Andrew's comment is address in above patch.

[Hin-Chung Lam]

Created attachment 38531 [details]
volume controls implementation without theme

[David Levin]

Comment on attachment 38513 [details]
volume slider theme implementation for chromium port

As discussed the duplicate
  static Image* mediaVolumeSliderThumb = Image::loadPlatformResource("mediaVolumeSliderThumb").releaseRef();
is unfortunate.  Please correct this.

[David Levin]

Comment on attachment 38531 [details]
volume controls implementation without theme

Looks fine to me and it addresses the comments given by the two Eric's.

[Eric Seidel (no email)]

Sorry the commit-queue had a hiccup this evening.  It's back up now and your patch will land as soon as the builders turn green again.

[Eric Seidel (no email)]

Comment on attachment 38531 [details]
volume controls implementation without theme

Clearing flags on attachment: 38531

Committed r47744: <http://trac.webkit.org/changeset/47744>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.