Unify platformWidthForGlyph across OS X and iOS
Created attachment 229948 [details] Patch
This patch isn't very clean - there are 4 helper functions. I would appreciate any advice you have about how to clean this up.
Created attachment 229993 [details] Patch
Style is a false negative - the enum values are already defined in the header, so I can't rename them.
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.
Created attachment 229997 [details] Patch
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.
Created attachment 230000 [details] Patch
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.
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.
(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.
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.
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.
http://trac.webkit.org/changeset/167773