We should implement a CSS extension to adjust font smoothing. Hyatt recommends the following for values for the property: 1. subpixel aa (cleartype vs. lcd on mac) 2. smoothed still 3. completely nonaa 4. auto (do what the OS wants)
Created attachment 39633 [details] Patch Unfortunately, I am having trouble with my Windows set up, and I am not sure if the Windows code that I wrote actually works or not.
The names of the members of the EFontSmoothing enum are a thing of horror.
Comment on attachment 39633 [details] Patch > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 48402) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,66 @@ > +2009-09-15 Beth Dakin <bdakin@apple.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Fix for <rdar://problem/7083741> Implement a CSS extension to > + adjust sub-pixel anti-aliasing for text Please add the bugzilla bug URL here. Please also lists the tests added. > > + EFontSmoothing smoothing = fontSmoothing(); > + if (smoothing == Antialiased) { > + context->setShouldAntialias(true); > + setShouldUseSmoothing(false); > + } else if (smoothing == Subpixel_Antialiased) { > + context->setShouldAntialias(true); > + setShouldUseSmoothing(true); > + } else if (smoothing == NoneSmooth) { > + context->setShouldAntialias(false); > + setShouldUseSmoothing(false); > + } > + Any reason not to use a switch statement here? > + > + EFontSmoothing smoothing = fontSmoothing(); > + if (smoothing == Antialiased) { > + graphicsContext->setShouldAntialias(true); > + setShouldUseSmoothing(false); > + } else if (smoothing == Subpixel_Antialiased) { > + graphicsContext->setShouldAntialias(true); > + setShouldUseSmoothing(true); > + } else if (smoothing == NoneSmooth) { > + graphicsContext->setShouldAntialias(false); > + setShouldUseSmoothing(false); > + } > + Any reason not to use a switch statement here? > > +enum EFontSmoothing { > + AutoSmooth, NoneSmooth, Antialiased, Subpixel_Antialiased > +}; Despite this file being filled with them, the E at the beginning of the enum name is not necessary. I also think the names should instead be: AutoSmoothing, NoSmoothing, AntialiasedSmoothing, SubpixelAntialiasedSmoothing The rest of the patch looks fine, but I would like for Mitz or Hyatt to look at it.
Comment on attachment 39633 [details] Patch > + if (id == CSSValueAuto || id == CSSValueNone > + || id == CSSValueAntialiased || id == CSSValueSubpixelAntialiased) > + valid_primitive = true; I hate the say the "||" lines up with valid_primitive on the next line, but I don't think we have consensus about a better style. > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(EFontSmoothing e) How about calling the argument "smoothing" instead of "e"? > + switch (e) { > + case AutoSmooth: > + m_value.ident = CSSValueAuto; > + break; > + case NoneSmooth: > + m_value.ident = CSSValueNone; > + break; > + case Antialiased: > + m_value.ident = CSSValueAntialiased; > + break; > + case Subpixel_Antialiased: > + m_value.ident = CSSValueSubpixelAntialiased; > + break; > + } It would be nice to fire an assertion if the enum has an illegal value. One way to do that would be to use "return" instead of "break" and stick an ASSERT_NOT_REACHED outside the switch statement. We could also set m_value.ident to CSSValueAuto in that case just to avoid uninitialized memory if the unthinkable happens. I think our coding style now formally states that case should be aligned with the switch instead of indented. It would be good to either conform to that or to revise the style guidelines. I know you're matching the rest of the file here. > +template<> inline CSSPrimitiveValue::operator EFontSmoothing() const > +{ > + switch (m_value.ident) { > + case CSSValueAuto: > + return AutoSmooth; > + case CSSValueNone: > + return NoneSmooth; > + case CSSValueAntialiased: > + return Antialiased; > + case CSSValueSubpixelAntialiased: > + return Subpixel_Antialiased; > + default: > + ASSERT_NOT_REACHED(); > + return AutoSmooth; > + } > +} Generally it's better style to put such ASSERT_NOT_REACHED outside the switch statement. If the type is an enum this gives us a warning if we leave out any enum values. In this case that doesn't apply, but I like the idea of following that style. > + case CSSPropertyWebkitFontSmoothing: > + { Normally our coding style would require putting this brace on the same line as the case. > + int id = primitiveValue->getIdent(); > + EFontSmoothing smoothing; > + switch (id) { > + case CSSValueAuto: > + smoothing = AutoSmooth; > + break; > + case CSSValueNone: > + smoothing = NoneSmooth; > + break; > + case CSSValueAntialiased: > + smoothing = Antialiased; > + break; > + case CSSValueSubpixelAntialiased: > + smoothing = Subpixel_Antialiased; > + break; > + } > + fontDescription.setFontSmoothing(smoothing); Given the code in CSSPrimitiveValueMappings the entire block above can be written like this: fontDescription.setFontSmoothing(primitiveValue->getIdent()); And not only that, it will work better than the above it also include an assert we omitted here. So lets do it that way! > Index: WebCore/platform/graphics/Font.h > =================================================================== > --- WebCore/platform/graphics/Font.h (revision 48399) > +++ WebCore/platform/graphics/Font.h (working copy) > @@ -125,6 +125,7 @@ public: > QFont font() const; > #endif > > + EFontSmoothing fontSmoothing() const { return m_fontDescription.fontSmoothing(); } Is it really important to have this convenience function? I think the various forwarding functions in this class don't do a lot of good. Why not write fontDescription().fontSmoothing() at each call site instead? There seem to be only two call sites. > + void setFontSmoothing (EFontSmoothing s) { m_fontSmoothing = s; } There's an extra space here before the parenthesis. And why not use the word "smoothing" instead of the letter "s"? > + EFontSmoothing smoothing = fontSmoothing(); > + if (smoothing == Antialiased) { > + context->setShouldAntialias(true); > + setShouldUseSmoothing(false); > + } else if (smoothing == Subpixel_Antialiased) { > + context->setShouldAntialias(true); > + setShouldUseSmoothing(true); > + } else if (smoothing == NoneSmooth) { > + context->setShouldAntialias(false); > + setShouldUseSmoothing(false); > + } I think the above should use a switch statement; for one thing it would get rid of the need for a local variable. I also don't understand how the Auto case works, and a switch statement would give you an opportunity to explain this in the empty case. Further, I think it's a little strange to actually change the state with setShouldUseSmoothing just so we can fetch that state a couple lines below. Could this be written to just use local variables instead? > +enum EFontSmoothing { > + AutoSmooth, NoneSmooth, Antialiased, Subpixel_Antialiased > +}; It's a little strange that we use E in all these type names. All of my comments are minor coding style issues that don't need to be fixed, so I'll say r=me. But if you want me to review a new version that's fine too.
Fixed with r48441. This is the one change I was unable to make: >> + int id = primitiveValue->getIdent(); >> + EFontSmoothing smoothing; >> + switch (id) { >> + case CSSValueAuto: >> + smoothing = AutoSmooth; >> + break; >> + case CSSValueNone: >> + smoothing = NoneSmooth; >> + break; >> + case CSSValueAntialiased: >> + smoothing = Antialiased; >> + break; >> + case CSSValueSubpixelAntialiased: >> + smoothing = Subpixel_Antialiased; >> + break; >> + } >> + fontDescription.setFontSmoothing(smoothing); > >Given the code in CSSPrimitiveValueMappings the entire block above can be >written like this: > > fontDescription.setFontSmoothing(primitiveValue->getIdent()); > >And not only that, it will work better than the above it also include an assert >we omitted here. So lets do it that way! When I tried to make this change, font smoothing was never set appropriately. It ended up being set to the integer value of the primitive's identifier.