[webkit-reviews] review denied: [Bug 68734] Enable LCD text in Skia on Mac : [Attachment 108544] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Cary Clark
<caryclark at google.com>'s request for review:
Bug 68734: Enable LCD text in Skia on Mac
https://bugs.webkit.org/show_bug.cgi?id=68734

Attachment 108544: Patch
https://bugs.webkit.org/attachment.cgi?id=108544&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=108544&action=review


> Source/WebCore/ChangeLog:11
> +	   Duplicate the logic in FontMac.mm to pass settings

That word makes me sad.  Is there any way to share the logic instead of
duplicating?

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:109
> +    case Antialiased: {
> +	   shouldSmoothFonts = false;
> +	   break;
> +    }

{ }s in a case statement is very strange - I think this would be easier to read
if they were omitted

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:123
> +    default: 
> +	   ASSERT_NOT_REACHED();

just leave out the 'default' case and the compiler will tell us if all cases
are listed or not

> Source/WebCore/platform/graphics/skia/FontSkia.cpp:126
> +    if (!shouldUseSmoothing() || PlatformSupport::layoutTestMode())

do you really need to check layoutTestMode() here? I thought we twiddled the
shouldSmoothFonts somewhere