Bug 57584 – [Chromium] Text anti-aliasing fails when rendering text with shadow

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Justin Novosad]

This is in reference to chromium bug: http://code.google.com/p/chromium/issues/detail?id=23440

The skia rendering path for chromium on windows yields poor quality font rendering.  The skia path is used rather than GDI when rendering text with shadows.

[Justin Novosad]

Created attachment 87861 [details]
When drawing text with a blurred shadow, split the rendering in two passes: pass 1 = shadows in skia, pass2 = foreground with GDI

[WebKit Review Bot]

Attachment 87861 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/test_expecta..." exit_code: 1

Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:233:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:400:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:477:  One line control clauses should not use braces.  [whitespace/braces] [4]
LayoutTests/platform/chromium/test_expectations.txt:3186:  Missing a ':' BUGWK57578 WIN LINUX svg/css/composite-shadow-text.svg = IMAGE  [test/expectations] [5]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Novosad]

Created attachment 87864 [details]
Fixed style

[Kenneth Russell]

Comment on attachment 87864 [details]
Fixed style

View in context: https://bugs.webkit.org/attachment.cgi?id=87864&action=review

The logic changes and test expectations updates look good. One minor style issue, just FYI, not necessary to fix. cq+'ing as is.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:389
> +    if (!alpha && graphicsContext->platformContext()->getStrokeStyle() == NoStroke
> +        && !graphicsContext->hasShadow()) {

WebKit style doesn't have a line length, so this would be easier to read if it were all one line.

[WebKit Commit Bot]

Comment on attachment 87864 [details]
Fixed style

Rejecting attachment 87864 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 2

Last 500 characters of output:
d9c211ba3462564fdea90fb1117d204b322fb82f (refs/remotes/trunk)
	M	LayoutTests/platform/gtk/editing/deleting/5408255-expected.checksum
	M	LayoutTests/platform/gtk/editing/deleting/5408255-expected.png
	M	LayoutTests/platform/gtk/editing/deleting/5408255-expected.txt
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
r82708 = 94c8d9b24cab7463b47f35c937399f0cdb9597a0 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8319255

[Kenneth Russell]

Oops, I should have noticed that you didn't have ChangeLog entries in your patch.

Please prepare your patches with the webkit-patch command; "webkit-patch upload" in particular.

[Justin Novosad]

Created attachment 88285 [details]
Added ChangeLog Entries

[WebKit Review Bot]

Attachment 88285 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Novosad]

Created attachment 88287 [details]
removed tab (oops)

[Stephen White]

Comment on attachment 88287 [details]
removed tab (oops)

View in context: https://bugs.webkit.org/attachment.cgi?id=88287&action=review

> LayoutTests/ChangeLog:3
> +        Reviewed by Kenneth Russel.

Should be two 'l's in Russell.  :)

> Source/WebCore/ChangeLog:3
> +        Reviewed by Kenneth Russel.

Ibid.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:405
> +        drawGlyphs(graphicsContext, font, glyphBuffer, from, numGlyphs, point);

Using recursion here seems unnecessary, and potentially fragile.  If someone screws up the tests later on it'll recurse forever.  Could you refactor the single-pass text rendering into a new function, and call that twice instead?  Unfortunately, you'll probably have to make it a static, or a member of some other class (TransparencyAwareGlyphPainter, maybe?) since Font.h is common code.

> Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:232
> +    bool hasShadow = context->platformContext()->getDrawLooper() && context->getShadow(shadowOffset, shadowBlur, shadowColor, shadowColorSpace);

I'm sure I'm just being dumb, but why do you check for the looper here?  Shouldn't checking the shadow params be enough?

> Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:233
> +    return hasShadow ? (!shadowBlur && (shadowColor.alpha() == 255) && (context->fillColor().alpha() == 255)) : true;

Nit:  this could probably be !hasShadow || (...), rather than using a ternary.

> Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp:238
> +    // Check for patterns.

Nit:  this comment is probably wrong, since the call below seems to check for much more than patterns.

[Kenneth Russell]

(In reply to comment #10)
> (From update of attachment 88287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88287&action=review
> 
> > LayoutTests/ChangeLog:3
> > +        Reviewed by Kenneth Russel.
> 
> Should be two 'l's in Russell.  :)
> 
> > Source/WebCore/ChangeLog:3
> > +        Reviewed by Kenneth Russel.
> 
> Ibid.

FYI, you should just leave the "Reviewed by NOBODY (OOPS!)" lines in the patch. The commit queue will rewrite them upon landing.

Looks like there is feedback to address from Stephen. I'll leave the r? bit as is; please update the bug indicating whether a revision will be made.

[Eric Seidel (no email)]

Comment on attachment 87864 [details]
Fixed style

Cleared Kenneth Russell's review+ from obsolete attachment 87864 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Justin Novosad]

Created attachment 88477 [details]
Response to review comments by Stephen White

[WebKit Review Bot]

Attachment 88477 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:481:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Novosad]

Created attachment 88482 [details]
Style fix

[Stephen White]

Comment on attachment 88482 [details]
Style fix

Thanks for the cleanup; that's a lot clearer.  Looks good to me (just need an r+ from Ken).

[Eric Seidel (no email)]

Comment on attachment 88482 [details]
Style fix

View in context: https://bugs.webkit.org/attachment.cgi?id=88482&action=review

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:457
> +    if (!alpha && graphicsContext->platformContext()->getStrokeStyle() == NoStroke
> +        && !graphicsContext->hasShadow()) {

WebKit has no 80c rule.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:459
> +    }

No {} on single line clauses.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:465
> +        FloatSize shadowOffset;
> +        float shadowBlur;
> +        Color shadowColor;

We don't do c-style early declarations in webkit.  You should define things as they're used.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:469
> +        Color tmpFillColor(0, 0, 0, 0);

I'm not sure there is any use in this being a local.  Also, isn't there Color::transparent or some other named color which is the same?  Maybe even just Color().

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:475
> +        Color tmpShadowColor(0, 0, 0, 0);

Again here.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:480
> +    } else
> +      drawGlyphsWin(graphicsContext, font, glyphBuffer, from, numGlyphs, point);

Seems like this could just be an early return and then you don't need the logn if block.

[Kenneth Russell]

Comment on attachment 88482 [details]
Style fix

Eric raises some valid WebKit style and maintainability issues. Could you please address them? Apologies for the repeated turnaround.

[Justin Novosad]

Created attachment 89052 [details]
Patch

[Kenneth Russell]

Comment on attachment 89052 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 89052 [details]
Patch

Clearing flags on attachment: 89052

Committed r83541: <http://trac.webkit.org/changeset/83541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.