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

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