[webkit-reviews] review granted: [Bug 132036] Unify platformWidthForGlyph across OS X and iOS : [Attachment 230000] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 132036: Unify platformWidthForGlyph across OS X and iOS
https://bugs.webkit.org/show_bug.cgi?id=132036

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

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


> Source/WebCore/platform/graphics/ios/SimpleFontDataIOS.mm:198
> +bool SimpleFontData::advanceForColorBitmapFont(Glyph glyph, CGSize& advance)
const
> +{
> +    UNUSED_PARAM(glyph);
> +    UNUSED_PARAM(advance);
> +    return false;
>  }

It’s better to omit the argument names rather than using UNUSED_PARAM in a case
like this.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:375
> +CGFontRenderingStyle SimpleFontData::renderingStyle() const

Probably should be inline.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:389
> +	   default:
> +	       style = kCGFontRenderingStyleAntialiasing |
kCGFontRenderingStyleSubpixelPositioning |
kCGFontRenderingStyleSubpixelQuantization;
> +	       break;

Doesn’t seem like we need this code. That’s already what style is set to.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:395
> +bool SimpleFontData::advanceForColorBitmapFont(Glyph glyph, CGSize& advance)
const

Probably should be inline.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:402
> +    NSFont *font = platformData().font();
> +    if (font && platformData().isColorBitmapFont()) {
> +	   advance = NSSizeToCGSize([font advancementForGlyph:glyph]);
> +	   return true;
> +    }
> +    return false;

I think code like this reads better if the early return is the failure case.

    if (!font || !platformData().isColorBitmapFont())
	return false;

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:408
> +#if !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED < 1090

I think PLATFORM(MAC) would be better than !PLATFORM(IOS) here.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:417
> +static bool isEmoji(const FontPlatformData& platformData)

Definitely should be inline.

>> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:432
>> +	    if (!advanceForColorBitmapFont(glyph, advance)) {
> 
> I think this could all be in one if statement.

I agree. And I also think we might want a more complete helper function that
combines the last three clauses into a single statement to keep the if
statement shorter and also to give us room to write short clear comments about
why these are the cases where we use CGFontGetGlyphAdvancesForStyle.

>> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:435
>> +		if (!CGFontGetGlyphAdvancesForStyle(platformData().cgFont(),
&m, renderingStyle(), &glyph, 1, &advance)) {
> 
> Are you sure this is API? Normally on OS X we used wksi when we are using
SPI, otherwise it does not compile on the bots. For iOS we never had this
problem and we still don't have OpenSource bots building iOS.

I think this will work. You’ll note that Myles declares this function above as
part of the __has_include dance. This can be a superior approach to the
WebKitSystemInterface one in cases like this.