Bug 24244 - Chromium Linux: switch to using Skia for widget renderer
Summary: Chromium Linux: switch to using Skia for widget renderer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Adam Langley
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-27 12:18 PST by Adam Langley
Modified: 2009-03-03 22:09 PST (History)
0 users

See Also:


Attachments
patch (57.75 KB, patch)
2009-02-27 12:18 PST, Adam Langley
fishd: review-
Details | Formatted Diff | Diff
patch (38.56 KB, patch)
2009-03-02 11:17 PST, Adam Langley
fishd: review+
Details | Formatted Diff | Diff
patch (39.48 KB, patch)
2009-03-02 13:18 PST, Adam Langley
fishd: review+
Details | Formatted Diff | Diff
patch (39.18 KB, patch)
2009-03-02 17:26 PST, Adam Langley
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-02-27 12:18:19 PST
Chromium Linux: Switch to using Skia to render widgets.

In order to sandbox the Chromium renderer on Linux we need to remove the
X connection. GTK cannot render without an X connection so, for now, we
render widgets ourselves.

Previously use didn't anti-alias fonts in order to match Windows font
rendering exactly. This was helpful when bootstrapping our layout
tests. Now, however, we are ready to enable it.
Comment 1 Adam Langley 2009-02-27 12:18:37 PST
Created attachment 28098 [details]
patch
Comment 2 Darin Fisher (:fishd, Google) 2009-02-27 14:16:23 PST
Comment on attachment 28098 [details]
patch

>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>+2009-02-27  Adam Langley  <agl@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.
Comment 3 Adam Langley 2009-03-02 11:17:16 PST
Created attachment 28188 [details]
patch
Comment 4 Darin Fisher (:fishd, Google) 2009-03-02 11:31:13 PST
Comment on attachment 28188 [details]
patch

>+++ b/WebCore/platform/chromium/ScrollbarThemeChromium.cpp
...
>+    const int girth = 0;
>+    // On Linux, we don't use buttons
>+    return IntSize(0, 0);

seems like it shouldn't be necessary to define girth given the early return,
or does GCC complain without it being there?


>+++ b/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp

>+    paint.setARGB(255, 0xe3, 0xdd, 0xd8);
...
>+    paint.setARGB(255, 0xc5, 0xba, 0xb0);

the mixing of decimal and hex really seems odd to me...  is that somehow
conventional?  (this hurts my brain)


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

>+        paint.setARGB(255, 0xe9, 0xe9, 0xe9);

ditto (and elsewhere in this file)
Comment 5 Adam Langley 2009-03-02 13:18:00 PST
Created attachment 28194 [details]
patch
Comment 6 Adam Langley 2009-03-02 13:19:39 PST
Have addressed all comments. It seems that replying to the bugzilla emails doesn't actually do anything, so sorry you missed the reply to the first one - but I believe I addressed everything there too. Patch interdiff follows since bugzilla doesn't seem to supply them:

diff --git a/WebCore/platform/chromium/ScrollbarThemeChromium.cpp b/WebCore/platform/chromium/ScrollbarThemeChromium.cpp
index 3096785..426a078 100644
--- a/WebCore/platform/chromium/ScrollbarThemeChromium.cpp
+++ b/WebCore/platform/chromium/ScrollbarThemeChromium.cpp
@@ -167,6 +167,11 @@ bool ScrollbarThemeChromium::shouldCenterOnThumb(Scrollbar*, const PlatformMouse
 
 IntSize ScrollbarThemeChromium::buttonSize(Scrollbar* scrollbar)
 {
+#if defined(__linux__)
+    // On Linux, we don't use buttons
+    return IntSize(0, 0);
+#endif
+
     // Our desired rect is essentially thickness by thickness.
 
     // Our actual rect will shrink to half the available space when we have < 2
@@ -175,20 +180,13 @@ IntSize ScrollbarThemeChromium::buttonSize(Scrollbar* scrollbar)
 
     int thickness = scrollbarThickness(scrollbar->controlSize());
 
-#if !defined(__linux__)
     // In layout test mode, we force the button "girth" (i.e., the length of
     // the button along the axis of the scrollbar) to be a fixed size.
     // FIXME: This is retarded!  scrollbarThickness is already fixed in layout
     // test mode so that should be enough to result in repeatable results, but
     // preserving this hack avoids having to rebaseline pixel tests.
     const int kLayoutTestModeGirth = 17;
-
     int girth = ChromiumBridge::layoutTestMode() ? kLayoutTestModeGirth : thickness;
-#else
-    const int girth = 0;
-    // On Linux, we don't use buttons
-    return IntSize(0, 0);
-#endif
 
     if (scrollbar->orientation() == HorizontalScrollbar) {
         int width = scrollbar->width() < 2 * girth ? scrollbar->width() / 2 : girth;
diff --git a/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp b/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp
index 6694946..11e2472 100644
--- a/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp
+++ b/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp
@@ -58,12 +58,12 @@ void ScrollbarThemeChromium::paintTrackPiece(GraphicsContext* gc, Scrollbar* scr
     SkCanvas* const canvas = gc->platformContext()->canvas();
 
     SkPaint paint;
-    paint.setARGB(255, 0xe3, 0xdd, 0xd8);
+    paint.setARGB(0xff, 0xe3, 0xdd, 0xd8);
     SkRect skrect;
     skrect.set(rect.x(), rect.y(), right, bottom);
     canvas->drawRect(skrect, paint);
 
-    paint.setARGB(255, 0xc5, 0xba, 0xb0);
+    paint.setARGB(0xff, 0xc5, 0xba, 0xb0);
     canvas->drawLine(rect.x(), rect.y(), rect.x(), bottom, paint);
     canvas->drawLine(right, rect.y(), right, bottom, paint);
     canvas->drawLine(rect.x(), rect.y(), right, rect.y(), paint);
@@ -88,9 +88,9 @@ void ScrollbarThemeChromium::paintThumb(GraphicsContext* gc, Scrollbar* scrollba
 
     SkPaint paint;
     if (hovered)
-        paint.setARGB(255, 0xff, 0xff, 0xff);
+        paint.setARGB(0xff, 0xff, 0xff, 0xff);
     else
-        paint.setARGB(255, 0xf4, 0xf2, 0xef);
+        paint.setARGB(0xff, 0xf4, 0xf2, 0xef);
 
     SkRect skrect;
     if (vertical) {
@@ -101,9 +101,9 @@ void ScrollbarThemeChromium::paintThumb(GraphicsContext* gc, Scrollbar* scrollba
     canvas->drawRect(skrect, paint);
 
     if (hovered) {
-      paint.setARGB(255, 0xf4, 0xf2, 0xef);
+      paint.setARGB(0xff, 0xf4, 0xf2, 0xef);
     } else {
-      paint.setARGB(255, 0xea, 0xe5, 0xe0);
+      paint.setARGB(0xff, 0xea, 0xe5, 0xe0);
     }
     if (vertical) {
       skrect.set(midx + 1, rect.y(), right, bottom);
@@ -112,14 +112,14 @@ void ScrollbarThemeChromium::paintThumb(GraphicsContext* gc, Scrollbar* scrollba
     }
     canvas->drawRect(skrect, paint);
 
-    paint.setARGB(255, 0x9d, 0x96, 0x8e);
+    paint.setARGB(0xff, 0x9d, 0x96, 0x8e);
     canvas->drawLine(rect.x(), rect.y(), rect.x(), bottom, paint);
     canvas->drawLine(right, rect.y(), right, bottom, paint);
     canvas->drawLine(rect.x(), rect.y(), right, rect.y(), paint);
     canvas->drawLine(rect.x(), bottom, right, bottom, paint);
 
     if (rect.height() > 10 && rect.width() > 10) {
-      paint.setARGB(255, 0x9d, 0x96, 0x8e);
+      paint.setARGB(0xff, 0x9d, 0x96, 0x8e);
       canvas->drawLine(midx - 1, midy, midx + 3, midy, paint);
       canvas->drawLine(midx - 1, midy - 3, midx + 3, midy - 3, paint);
       canvas->drawLine(midx - 1, midy + 3, midx + 3, midy + 3, paint);
diff --git a/WebCore/rendering/RenderThemeChromiumLinux.cpp b/WebCore/rendering/RenderThemeChromiumLinux.cpp
index 086552d..2a078ce 100644
--- a/WebCore/rendering/RenderThemeChromiumLinux.cpp
+++ b/WebCore/rendering/RenderThemeChromiumLinux.cpp
@@ -204,7 +204,7 @@ static void paintButtonLike(RenderTheme* theme, RenderObject* o, const RenderObj
 
     // If the button is too small, fallback to drawing a single, solid color
     if (rect.width() < 5 || rect.height() < 5) {
-        paint.setARGB(255, 0xe9, 0xe9, 0xe9);
+        paint.setARGB(0xff, 0xe9, 0xe9, 0xe9);
         skrect.set(rect.x(), rect.y(), right, bottom);
         canvas->drawRect(skrect, paint);
         return;
@@ -217,15 +217,15 @@ static void paintButtonLike(RenderTheme* theme, RenderObject* o, const RenderObj
     canvas->drawLine(rect.x() + 1, bottom - 1, right - 1, bottom - 1, paint);
     canvas->drawLine(rect.x(), rect.y() + 1, rect.x(), bottom - 1, paint);
 
-    paint.setARGB(255, 0, 0, 0);
+    paint.setARGB(0xff, 0, 0, 0);
     SkPoint p[2];
     const int lightEnd = theme->isPressed(o) ? 1 : 0;
     const int darkEnd = !lightEnd;
     p[lightEnd].set(SkIntToScalar(rect.x()), SkIntToScalar(rect.y()));
     p[darkEnd].set(SkIntToScalar(rect.x()), SkIntToScalar(bottom - 1));
     SkColor colors[2];
-    colors[0] = SkColorSetARGB(255, 0xf8, 0xf8, 0xf8);
-    colors[1] = SkColorSetARGB(255, 0xdd, 0xdd, 0xdd);
+    colors[0] = SkColorSetARGB(0xff, 0xf8, 0xf8, 0xf8);
+    colors[1] = SkColorSetARGB(0xff, 0xdd, 0xdd, 0xdd);
 
     SkShader* s = SkGradientShader::CreateLinear(
         p, colors, NULL, 2, SkShader::kClamp_TileMode, NULL);
@@ -237,7 +237,7 @@ static void paintButtonLike(RenderTheme* theme, RenderObject* o, const RenderObj
     canvas->drawRect(skrect, paint);
 
     paint.setShader(NULL);
-    paint.setARGB(255, 0xce, 0xce, 0xce);
+    paint.setARGB(0xff, 0xce, 0xce, 0xce);
     canvas->drawPoint(rect.x() + 1, rect.y() + 1, paint);
     canvas->drawPoint(right - 2, rect.y() + 1, paint);
     canvas->drawPoint(rect.x() + 1, bottom - 2, paint);
@@ -290,7 +290,7 @@ bool RenderThemeChromiumLinux::paintMenuList(RenderObject* o, const RenderObject
     paintButtonLike(this, o, i, rect);
 
     SkPaint paint;
-    paint.setARGB(255, 0, 0, 0);
+    paint.setARGB(0xff, 0, 0, 0);
     paint.setAntiAlias(true);
     paint.setStyle(SkPaint::kFill_Style);
 
Comment 7 Darin Fisher (:fishd, Google) 2009-03-02 16:06:15 PST
Comment on attachment 28194 [details]
patch

LGTM
Comment 8 Adam Langley 2009-03-02 17:26:18 PST
Created attachment 28206 [details]
patch

Merge with trunk: collided with Ojan.
Comment 9 Darin Fisher (:fishd, Google) 2009-03-03 22:09:46 PST
Landed as http://trac.webkit.org/changeset/41416