Bug 43406 – WebKitTestRunner needs to be able to set the font smoothing type

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jon Honeycutt]

WebKitTestRunner needs to be able to set the font smoothing type to match the setting used by DRT.

[Jon Honeycutt]

Created attachment 63497 [details]
Patch

[Jon Honeycutt]

Created attachment 63527 [details]
Patch

Add FontSmoothingLevel.h to non-Windows project files.

[Jon Honeycutt]

<rdar://problem/8275136>

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKPreferences.cpp:98:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit2/UIProcess/API/C/WKPreferences.cpp:125:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 20 files


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

[Adam Roben (:aroben)]

Comment on attachment 63527 [details]
Patch

> +enum {
> +    FontSmoothingLevelStandard = 0,
> +    FontSmoothingLevelLight = 1,
> +    FontSmoothingLevelMedium = 2,
> +    FontSmoothingLevelStrong = 3,
> +    FontSmoothingLevelWindows = 4,
> +};
> +typedef uint32_t FontSmoothingLevel;

Why the typedef? Why not just call the enum "FontSmoothingLevel"? If the typedef is only needed for WebKit2, it seems inappropriate (and undesirable) to have it in WebCore.

Should we compile out FontSmoothingLevelWindows on non-Windows platforms?

> +// Defaults to kWKFontSmoothingLevelMedium.
> +WK_EXPORT void WKPreferencesSetFontSmoothingLevel(WKPreferencesRef, WKFontSmoothingLevel);
> +WK_EXPORT WKFontSmoothingLevel WKPreferencesGetFontSmoothingLevel(WKPreferencesRef);

kWKFontSmoothingLevelStandard doesn't seem very "Standard" if it isn't the default!

> @@ -70,6 +70,7 @@ void TestInvocation::resetPreferencesToConsistentValues()
>  {
>      WKPreferencesRef preferences = WKContextGetPreferences(TestController::shared().context());
>      WKPreferencesSetOfflineWebApplicationCacheEnabled(preferences, true);
> +    WKPreferencesSetFontSmoothingLevel(preferences, kWKFontSmoothingLevelStandard);
>  }

It seems unfortunate for "Standard" on Windows to trigger an arguably non-standard mode (i.e., CG font rendering).

The code looks fine, but I'm concerned we don't quite have the right API yet. For instance, maybe turning on CG font rendering should be a private API on Windows. It should at least be clearer what's going on. Sam and Anders will probably be able to give some guidance.

[Jon Honeycutt]

(In reply to comment #5)
> (From update of attachment 63527 [details])
> > +enum {
> > +    FontSmoothingLevelStandard = 0,
> > +    FontSmoothingLevelLight = 1,
> > +    FontSmoothingLevelMedium = 2,
> > +    FontSmoothingLevelStrong = 3,
> > +    FontSmoothingLevelWindows = 4,
> > +};
> > +typedef uint32_t FontSmoothingLevel;
> 
> Why the typedef? Why not just call the enum "FontSmoothingLevel"? If the typedef is only needed for WebKit2, it seems inappropriate (and undesirable) to have it in WebCore.

I did this to avoid some casts, but I'll change it. I'll also move it out of WebCore.

> 
> Should we compile out FontSmoothingLevelWindows on non-Windows platforms?

Yes, I'll do this.

> 
> > +// Defaults to kWKFontSmoothingLevelMedium.
> > +WK_EXPORT void WKPreferencesSetFontSmoothingLevel(WKPreferencesRef, WKFontSmoothingLevel);
> > +WK_EXPORT WKFontSmoothingLevel WKPreferencesGetFontSmoothingLevel(WKPreferencesRef);
> 
> kWKFontSmoothingLevelStandard doesn't seem very "Standard" if it isn't the default!

Yes, the naming is strange. I copied these names from the old WebPreferences, and I don't know why they were chosen - maybe they match CG terminology. I'll look into changing it.

> 
> > @@ -70,6 +70,7 @@ void TestInvocation::resetPreferencesToConsistentValues()
> >  {
> >      WKPreferencesRef preferences = WKContextGetPreferences(TestController::shared().context());
> >      WKPreferencesSetOfflineWebApplicationCacheEnabled(preferences, true);
> > +    WKPreferencesSetFontSmoothingLevel(preferences, kWKFontSmoothingLevelStandard);
> >  }
> 
> It seems unfortunate for "Standard" on Windows to trigger an arguably non-standard mode (i.e., CG font rendering).
> 
> The code looks fine, but I'm concerned we don't quite have the right API yet. For instance, maybe turning on CG font rendering should be a private API on Windows. It should at least be clearer what's going on. Sam and Anders will probably be able to give some guidance.

Thanks for the review! I'll talk to Sam and/or Anders and post a new patch.

[Jon Honeycutt]

Created attachment 63689 [details]
Patch v2

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKPreferencesPrivate.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/WKPreferencesPrivate.cpp:39:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit2/UIProcess/API/C/WKPreferencesPrivate.cpp:68:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 3 in 18 files


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

[Adam Roben (:aroben)]

Comment on attachment 63689 [details]
Patch v2

> +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

[Jon Honeycutt]

(In reply to comment #9)
> (From update of attachment 63689 [details])
> > +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.)

I don't like the name, but I'm not sure FontSmoothingStyle would be better. I was trying to match the WKSI call that does the real work here, and there's another WKSI function wkSetFontSmoothingStyle().

> 
> > +void WKPreferencesSetFontSmoothingLevel(WKPreferencesRef preferencesRef, WKFontSmoothingLevel wkLevel)
> > +{
> > +    FontSmoothingLevel level = FontSmoothingLevelNoSubpixelAntiAliasing;
> 
> Does the compiler complain if you leave out this initialization?

No - removed.

> 
> > +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.

Fixed.

> 
> > +#ifndef WKPreferencesPrivate_h
> > +#define WKPreferencesPrivate_h
> > +
> > +#include <WebKit2/WKBase.h>
> > +
> > +#ifndef __cplusplus
> > +#include <stdbool.h>
> > +#endif
> 
> Why is this needed?

It's not - removed.

> 
> > +++ b/WebKit2/UIProcess/WebPreferences.cpp
> > @@ -27,6 +27,8 @@
> >  
> >  #include "WebContext.h"
> >  
> > +using namespace WebCore;
> 
> I don't think you need this anymore.

Removed.

> 
> > +        * WebKitTestRunner/TestInvocation.cpp:
> > +        (WTR::TestInvocation::resetPreferencesToConsistentValues):
> > +        Set the font smoothing level to
> > +        kWKFontSmoothingLevelNoSubpixelAntiAliasing.
> 
> If this is what DRT does, you should say so.

Done.

> 
> r=me

Thanks for the review! Landed in r64888.