[webkit-reviews] review denied: [Bug 28241] Media controls panel does not have a volume control slider : [Attachment 38395] added controls, style and implementation of controls but no theme

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Hin-Chung Lam <hclam at google.com>'s
request for review:
Bug 28241: Media controls panel does not have a volume control slider
https://bugs.webkit.org/show_bug.cgi?id=28241

Attachment 38395: added controls, style and implementation of controls but no
theme
https://bugs.webkit.org/attachment.cgi?id=38395&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.