[webkit-reviews] review denied: [Bug 25903] Create a default UI theme for media controls in Chromium. : [Attachment 30559] Updated patch with style & copyright fixes.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Albert J. Wong <ajwong at chromium.org>'s
request for review:
Bug 25903: Create a default UI theme for media controls in Chromium.
https://bugs.webkit.org/show_bug.cgi?id=25903

Attachment 30559: Updated patch with style & copyright fixes.
https://bugs.webkit.org/attachment.cgi?id=30559&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I did not look at your CSS, I assume it's perfect in every way.

Why's are generally more helpful comments than "whats":
 60 // The background for the media player controls should be a 60% opaque
black rectangle.

Color::black?
 419	 context->setFillColor(Color(0x00,0x00,0x00));

Write a static HTMLMediaElement* toMediaElement(Node*) function instead of this
(slightly dangerous) copy/paste code:
40     HTMLMediaElement* mediaElement = static_cast<HTMLMediaElement*>(node ?
node->shadowAncestorNode() : 0);
 441 
 442	 if (!mediaElement || (!mediaElement->hasTagName(HTMLNames::videoTag)
&& !mediaElement->hasTagName(HTMLNames::audioTag)))
 443	     return false;

Are you intentionally avoiding static RefPtr's?  I guess we do that to avoid
using static constructors/destructors, eh?
 445	 static Image* mediaPlay =
Image::loadPlatformResource("mediaPlay").releaseRef();

Bah!  The code is identical between Win and Linux.  Can't we put this in a
shared RenderThemeChromium?  Or RenderThemeChomiumMedia or something? 
copy/paste code, as Jim Roskin would say, is the root of all evil. :)  (I'm
paraphrasing here.)

r- for the unsafe casts.  I'd r+ it with the copy paste... grudgingly.