[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 #10 from Eric Carlson <eric.carlson at apple.com>  2009-08-22 10:59:13 PDT ---
> 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.

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