[webkit-reviews] review granted: [Bug 43406] WebKitTestRunner needs to be able to set the font smoothing type : [Attachment 63689] Patch v2

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 43406: WebKitTestRunner needs to be able to set the font smoothing type
https://bugs.webkit.org/show_bug.cgi?id=43406

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +enum FontSmoothingLevel {
> +    FontSmoothingLevelNoSubpixelAntiAliasing = 0,
> +    FontSmoothingLevelLight = 1,
> +    FontSmoothingLevelMedium = 2,
> +    FontSmoothingLevelStrong = 3,
> +#if PLATFORM(WIN)
> +    FontSmoothingLevelWindows = 4,
> +#endif
> +};

I wonder if "FontSmoothingStyle" would be more appropriate.
"NoSubpixelAntiAliasing" and "Windows" aren't really "levels". (I'm not sure
"Light", "Medium", and "Strong" are, either.)

> +void WKPreferencesSetFontSmoothingLevel(WKPreferencesRef preferencesRef,
WKFontSmoothingLevel wkLevel)
> +{
> +    FontSmoothingLevel level = FontSmoothingLevelNoSubpixelAntiAliasing;

Does the compiler complain if you leave out this initialization?

> +WKFontSmoothingLevel WKPreferencesGetFontSmoothingLevel(WKPreferencesRef
preferencesRef)
> +{
> +    FontSmoothingLevel level = toWK(preferencesRef)->fontSmoothingLevel();
> +    switch (level) {
> +	   case FontSmoothingLevelNoSubpixelAntiAliasing:
> +	       return kWKFontSmoothingLevelNoSubpixelAntiAliasing;
> +	   case FontSmoothingLevelLight:
> +	       return kWKFontSmoothingLevelLight;
> +	   case FontSmoothingLevelMedium:
> +	       return kWKFontSmoothingLevelMedium;
> +	   case FontSmoothingLevelStrong:
> +	       return kWKFontSmoothingLevelStrong;
> +#if PLATFORM(WIN)
> +	   case FontSmoothingLevelWindows:
> +	       return kWKFontSmoothingLevelWindows;
> +#endif
> +    }
> +
> +    ASSERT_NOT_REACHED();
> +    return kWKFontSmoothingLevelNoSubpixelAntiAliasing;
> +}

I think you should return Medium here, to match the setter and the claimed
default.

> +#ifndef WKPreferencesPrivate_h
> +#define WKPreferencesPrivate_h
> +
> +#include <WebKit2/WKBase.h>
> +
> +#ifndef __cplusplus
> +#include <stdbool.h>
> +#endif

Why is this needed?

> +++ b/WebKit2/UIProcess/WebPreferences.cpp
> @@ -27,6 +27,8 @@
>  
>  #include "WebContext.h"
>  
> +using namespace WebCore;

I don't think you need this anymore.

> +	   * WebKitTestRunner/TestInvocation.cpp:
> +	   (WTR::TestInvocation::resetPreferencesToConsistentValues):
> +	   Set the font smoothing level to
> +	   kWKFontSmoothingLevelNoSubpixelAntiAliasing.

If this is what DRT does, you should say so.

r=me