[webkit-reviews] review denied: [Bug 25903] Create a default UI theme for media controls in Chromium. : [Attachment 30520] Patch to create a default UI for HTML5 media controls in chromium.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.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 30520: Patch to create a default UI for HTML5 media controls in
chromium.
https://bugs.webkit.org/attachment.cgi?id=30520&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ 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.