Bug 25903 - Create a default UI theme for media controls in Chromium.
Summary: Create a default UI theme for media controls in Chromium.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Albert J. Wong
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-20 19:09 PDT by Albert J. Wong
Modified: 2009-05-21 18:56 PDT (History)
1 user (show)

See Also:


Attachments
Patch to create a default UI for HTML5 media controls in chromium. (17.82 KB, patch)
2009-05-20 19:28 PDT, Albert J. Wong
fishd: review-
Details | Formatted Diff | Diff
Updated patch with style & copyright fixes. (18.11 KB, patch)
2009-05-21 14:51 PDT, Albert J. Wong
eric: review-
Details | Formatted Diff | Diff
Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement() (18.95 KB, patch)
2009-05-21 17:10 PDT, Albert J. Wong
eric: review-
Details | Formatted Diff | Diff
Couple more style fixes. (19.03 KB, patch)
2009-05-21 17:28 PDT, Albert J. Wong
no flags Details | Formatted Diff | Diff
Normalize Color specification. (21.62 KB, patch)
2009-05-21 18:07 PDT, Albert J. Wong
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Albert J. Wong 2009-05-20 19:09:18 PDT
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.
Comment 1 Albert J. Wong 2009-05-20 19:28:41 PDT
Created attachment 30520 [details]
Patch to create a default UI for HTML5 media controls in chromium.
Comment 2 Darin Fisher (:fishd, Google) 2009-05-21 14:27:49 PDT
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.
Comment 3 Albert J. Wong 2009-05-21 14:51:29 PDT
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 4 Eric Seidel (no email) 2009-05-21 16:44:49 PDT
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.
Comment 5 Albert J. Wong 2009-05-21 17:10:39 PDT
Created attachment 30569 [details]
Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement()
Comment 6 Eric Seidel (no email) 2009-05-21 17:16:22 PDT
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.
Comment 7 Albert J. Wong 2009-05-21 17:28:05 PDT
Created attachment 30570 [details]
Couple more style fixes.
Comment 8 Albert J. Wong 2009-05-21 18:07:59 PDT
Created attachment 30571 [details]
Normalize Color specification.
Comment 9 Eric Seidel (no email) 2009-05-21 18:11:19 PDT
Comment on attachment 30571 [details]
Normalize Color specification.

Looks fine.  Thanks for going through so many revisions.
Comment 10 David Levin 2009-05-21 18:56:15 PDT
http://trac.webkit.org/changeset/44029