Bug 25903 – Create a default UI theme for media controls in Chromium.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Albert J. Wong]

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.

[Albert J. Wong]

Created attachment 30520 [details]
Patch to create a default UI for HTML5 media controls in chromium.

[Darin Fisher (:fishd, Google)]

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.

[Albert J. Wong]

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.

[Eric Seidel (no email)]

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.

[Albert J. Wong]

Created attachment 30569 [details]
Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement()

[Eric Seidel (no email)]

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.

[Albert J. Wong]

Created attachment 30570 [details]
Couple more style fixes.

[Albert J. Wong]

Created attachment 30571 [details]
Normalize Color specification.

[Eric Seidel (no email)]

Comment on attachment 30571 [details]
Normalize Color specification.

Looks fine.  Thanks for going through so many revisions.

[David Levin]

http://trac.webkit.org/changeset/44029