Adds a new style sheet, plus code changes to WebCore/rendering/RenderThemeChromium{Win|Linux}.{h,cc} to create a default ui for HTML5 media elements.
Created attachment 30520 [details] Patch to create a default UI for HTML5 media controls in chromium.
Comment on attachment 30520 [details] Patch to create a default UI for HTML5 media controls in chromium. > +++ b/WebCore/ChangeLog ... > + Reviewed by NOBODY (OOPS!). > + > + Implement a default UI for chromium. Add a style sheet for Chromium media controls > + with good defaults and implemented basic draw functions for play/pause & mute buttons. > + > + * css/mediaControlsChromium.css: Added. > + * rendering/RenderThemeChromiumLinux.cpp: > + (WebCore::RenderThemeChromiumWin::extraMediaControlsStyleSheet): Export our custom > + media controls style sheet. > + (WebCore::RenderThemeChromiumLinux::paintMediaButtonInternal): Paint buttons > + respecting chromium media controls color scheme. nit: please make sure that all of the lines (even those which wrap) are indented. compare to other ChangeLog entries. > +++ b/WebCore/css/mediaControlsChromium.css ... > + * Copyright (C) 2009 Google Inc. All rights reserved. It looks like you basically forked this from the existing mediaControls.css or mediaControlsQT.css. Both of those are Copyright Apple. I suspect that you probably want to preserve their copyright, and then add in our copyright line. You can see that being done in other files, like PlatformKeyboardEventChromium.cpp. > +++ b/WebCore/rendering/RenderThemeChromiumLinux.cpp ... > // form control fonts. > static const float DefaultFontSize = 16.0; though you didn't change this line, it looks like it should probably have been written defaultFontSize instead. might be nice to fix that while you are here or send a note to whomever has blame (agl?) so that person can fix it. > +bool RenderThemeChromiumWin::paintMediaButtonInternal(GraphicsContext* context, const IntRect& rect, Image* image) ... > +bool RenderThemeChromiumWin::paintMediaPlayButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& rect) ... > +bool RenderThemeChromiumWin::paintMediaMuteButton(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& rect) The code duplication here is quite unfortunate. We should find a way to avoid this. Doesn't have to block this patch since I know we have a lot of other duplication too.
Created attachment 30559 [details] Updated patch with style & copyright fixes. Fixed the style issues, copyright, and the incorrectly capitalized defaultFontSize. I completely agree with the code duplication. This change is what prompted me to start the thread about refactoring the render themes. I'll be in this file for a bit, and intend to try and find a way to factor out the common bits sanely. But that's something for next week.
Comment on attachment 30559 [details] Updated patch with style & copyright fixes. 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.
Created attachment 30569 [details] Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement()
Comment on attachment 30569 [details] Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement() We use 0, not NULL in c++ code. See the WK style guide: http://webkit.org/coding/coding-style.html also { goes on a new line. node ? node->shadowAncestorNode() : 0 would read better to me as: if (!node) return 0; Node* mediaNode = node->shadowAncestorNode(); Since this is a function only written once, you can afford to take up a little more space. :) Also, I wonder if we shoudl call this toMediaElemetn since we're not casting the passed in node. mediaElementParent(node) maybe? Please file a bug about removing the copy/paste code. We really need to clean this up. r- for the style nits.
Created attachment 30570 [details] Couple more style fixes.
Created attachment 30571 [details] Normalize Color specification.
Comment on attachment 30571 [details] Normalize Color specification. Looks fine. Thanks for going through so many revisions.
http://trac.webkit.org/changeset/44029