Bug 132036 – Unify platformWidthForGlyph across OS X and iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Unify platformWidthForGlyph across OS X and iOS

[Myles C. Maxfield]

Created attachment 229948 [details]
Patch

[Myles C. Maxfield]

This patch isn't very clean - there are 4 helper functions. I would appreciate any advice you have about how to clean this up.

[Myles C. Maxfield]

Created attachment 229993 [details]
Patch

[Myles C. Maxfield]

Style is a false negative - the enum values are already defined in the header, so I can't rename them.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:61:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:62:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:63:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:64:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:65:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:66:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 6 in 4 files


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

[Myles C. Maxfield]

Created attachment 229997 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:61:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:62:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:63:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:64:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:65:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:66:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 6 in 4 files


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

[Myles C. Maxfield]

Created attachment 230000 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:62:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:63:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:64:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:65:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:66:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/SimpleFontData.h:67:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 6 in 4 files


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

[Enrica Casucci]

Comment on attachment 230000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230000&action=review

> Source/WebCore/platform/graphics/SimpleFontData.h:71
> +#endif

You should use a data type that is not platform specific in this file.

> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:432
> +        if (!advanceForColorBitmapFont(glyph, advance)) {

I think this could all be in one if statement.

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

[Tim Horton]

(In reply to comment #10)
> (From update of attachment 230000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230000&action=review
> 
> > Source/WebCore/platform/graphics/SimpleFontData.h:71
> > +#endif
> 
> You should use a data type that is not platform specific in this file.
> 
> > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:432
> > +        if (!advanceForColorBitmapFont(glyph, advance)) {
> 
> I think this could all be in one if statement.
> 
> > 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

This mostly isn't true anymore; we tend to use forward declarations and has_include, like Myles does here.

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

[Darin Adler]

Comment on attachment 230000 [details]
Patch

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.

[Myles C. Maxfield]

Comment on attachment 230000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230000&action=review

>> Source/WebCore/platform/graphics/ios/SimpleFontDataIOS.mm:198
>>  }
> 
> It’s better to omit the argument names rather than using UNUSED_PARAM in a case like this.

Done.

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

Done.

>> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:389
>> +            break;
> 
> Doesn’t seem like we need this code. That’s already what style is set to.

Done.

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

Done.

>> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:402
>> +    return false;
> 
> I think code like this reads better if the early return is the failure case.
> 
>     if (!font || !platformData().isColorBitmapFont())
>         return false;

Done.

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

Done.

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

Done.

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

Done.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/167773