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.
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
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.
Created attachment 87864 [details] Fixed style
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.
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
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.
Created attachment 88285 [details] Added ChangeLog Entries
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.
Created attachment 88287 [details] removed tab (oops)
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.
(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.
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.
Created attachment 88477 [details] Response to review comments by Stephen White
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.
Created attachment 88482 [details] Style fix
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).
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.
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.
Created attachment 89052 [details] Patch
Comment on attachment 89052 [details] Patch Looks good to me.
Comment on attachment 89052 [details] Patch Clearing flags on attachment: 89052 Committed r83541: <http://trac.webkit.org/changeset/83541>
All reviewed patches have been landed. Closing bug.