[webkit-reviews] review granted: [Bug 29291] Implement a CSS extension to adjust sub-pixel anti-aliasing for text : [Attachment 39633] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 29291: Implement a CSS extension to adjust sub-pixel anti-aliasing for text
https://bugs.webkit.org/show_bug.cgi?id=29291

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   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.