[webkit-reviews] review denied: [Bug 24244] Chromium Linux: switch to using Skia for widget renderer : [Attachment 28098] patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <darin at chromium.org> has denied Adam Langley
<agl at chromium.org>'s request for review:
Bug 24244: Chromium Linux: switch to using Skia for widget renderer
https://bugs.webkit.org/show_bug.cgi?id=24244

Attachment 28098: patch
https://bugs.webkit.org/attachment.cgi?id=28098&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<darin at chromium.org>
>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>+2009-02-27  Adam Langley  <agl at chromium.org>
...
>+	  WARNING: NO TEST CASES ADDED OR CHANGED

nit: please remove this WARNING line... and add a link to this bug.


>+	  * rendering/RenderThemeChromiumLinux.cpp: Added.
>+	  (WebCore::):
>+	  (WebCore::supportsFocus):
>+	  (WebCore::setSizeIfAuto):
>+	  (WebCore::defaultGUIFont):
>+	  (WebCore::theme):

nit: when adding a new file, please delete all of the functions that
prepare-ChangeLog added.



>+++ b/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp
...
> void ScrollbarThemeChromium::paintThumb(GraphicsContext* gc, Scrollbar*
scrollbar, const IntRect& rect)
> {
...
>+    SkPaint paint;
>+    if (hovered) {
>+	paint.setARGB(255, 0xff, 0xff, 0xff);
>+    } else {
>+	paint.setARGB(255, 0xf4, 0xf2, 0xef);
>+    }

nit: indentation should be 4 spaces, and you should not use brackets
when there is only a single line statement.

please see:  http://webkit.org/coding/coding-style.html


>+++ b/WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp
...
>+    paint->setAntiAlias(true);

Making friends in high places?


>+++ b/WebCore/rendering/RenderThemeChromiumLinux.cpp

It might be better to do a "svn mv" so that this diff would just
show your changes.


>+Color RenderThemeChromiumLinux::platformActiveSelectionBackgroundColor()
const
>+{
>+    return Color(0x1e, 0x90, 0xff);
>+}
>+
>+Color RenderThemeChromiumLinux::platformInactiveSelectionBackgroundColor()
const
>+{
>+    return Color(200, 200, 200);
>+}

sometimes hex, sometimes decimal?


>+void RenderThemeChromiumLinux::systemFont(int propId, Document* document,
FontDescription& fontDescription) const
>+{
>+  fontDescription.firstFamily().setFamily(defaultGUIFont(NULL));
>+  fontDescription.setSpecifiedSize(12);

bad indentation


>+++ b/WebCore/rendering/RenderThemeChromiumLinux.h

>+	  virtual void adjustButtonInnerStyle(RenderStyle* style) const;

nit: please remove the parameter name since it doesn't add any information.