[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 #12 from Hin-Chung Lam <hclam at google.com>  2009-08-24 16:19:50 PDT ---
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.

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