[webkit-reviews] review granted: [Bug 48539] Support the text-emphasis CSS property : [Attachment 76712] Part 3: Platform support

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org's request for review:
Bug 48539: Support the text-emphasis CSS property
https://bugs.webkit.org/show_bug.cgi?id=48539

Attachment 76712: Part 3: Platform support
https://bugs.webkit.org/attachment.cgi?id=76712&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=76712&action=review

> WebCore/platform/graphics/FontFastPath.cpp:200
> +// FIXME: This method is not complex-text aware.

Anthropomorphizing the function and saying it’s not aware of complex text
doesn’t make clear to me what the problem is here. Is it incorrect in some way?
For what cases? I think you can probably write a slightly different comment
that will be less ambiguous.

Also, I’d prefer you use the C++ term function rather than the term method.

> WebCore/platform/graphics/SimpleFontData.cpp:200
> +	   m_derivedFontData = new DerivedFontData(isCustomFont());

Can I talk you into future-proofing this code by calling adoptPtr in all the
call sites like this one? In a related question, should we use the create
function idiom for this struct?

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:85
> +    return new SimpleFontData(
> +	  
FontPlatformData(cairo_scaled_font_get_font_face(m_platformData.scaledFont()),

I would have made that first argument be on the first line because of my
distaste for breaking right after an open paren. I think the line length ends
up being not too horrible.

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:96
> +	   m_derivedFontData->smallCaps = scaledFontData(fontDescription, .7);

We should really name that 0.7 constant at some point. I know it wasn’t named
before.

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:106
> +	   m_derivedFontData->emphasisMark = scaledFontData(fontDescription,
.5);

And this 0.5 constant.