Bug 24244 – Chromium Linux: switch to using Skia for widget renderer

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Adam Langley]

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.

[Adam Langley]

Created attachment 28098 [details]
patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 28098 [details]
patch

>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>[phone]  Adam Langley  <[e-mail]>
...
>+        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.

[Adam Langley]

Created attachment 28188 [details]
patch

[Darin Fisher (:fishd, Google)]

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)

[Adam Langley]

Created attachment 28194 [details]
patch

[Adam Langley]

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);
 

[Darin Fisher (:fishd, Google)]

Comment on attachment 28194 [details]
patch

LGTM

[Adam Langley]

Created attachment 28206 [details]
patch

Merge with trunk: collided with Ojan.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41416