[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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38395|review?                     |review-
               Flag|                            |




--- Comment #8 from Eric Seidel <eric at webkit.org>  2009-08-22 09:41:31 PDT ---
(From update of attachment 38395)
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.

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