WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
24244
Chromium Linux: switch to using Skia for widget renderer
https://bugs.webkit.org/show_bug.cgi?id=24244
Summary
Chromium Linux: switch to using Skia for widget renderer
Adam Langley
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Adam Langley
Comment 1
2009-02-27 12:18:37 PST
Created
attachment 28098
[details]
patch
Darin Fisher (:fishd, Google)
Comment 2
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.
Adam Langley
Comment 3
2009-03-02 11:17:16 PST
Created
attachment 28188
[details]
patch
Darin Fisher (:fishd, Google)
Comment 4
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)
Adam Langley
Comment 5
2009-03-02 13:18:00 PST
Created
attachment 28194
[details]
patch
Adam Langley
Comment 6
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);
Darin Fisher (:fishd, Google)
Comment 7
2009-03-02 16:06:15 PST
Comment on
attachment 28194
[details]
patch LGTM
Adam Langley
Comment 8
2009-03-02 17:26:18 PST
Created
attachment 28206
[details]
patch Merge with trunk: collided with Ojan.
Darin Fisher (:fishd, Google)
Comment 9
2009-03-03 22:09:46 PST
Landed as
http://trac.webkit.org/changeset/41416
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug