Bug 152293 – [Cocoa] Use faster function to calculate glyph advances

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

[Myles C. Maxfield]

[Cocoa] Use faster function to calculate glyph advances

[Myles C. Maxfield]

Created attachment 267343 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 267343 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:653
> +    CTFontGetUnsummedAdvancesForGlyphsAndStyle(m_platformData.ctFont(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, renderingStyle(platformData()), &glyph, &advance, 1)

This should probably be kCGFontRenderingStyleAntialiasing | kCGFontAntialiasingStyleUnfiltered

[Myles C. Maxfield]

Created attachment 267390 [details]
Patch

[Myles C. Maxfield]

Created attachment 267411 [details]
Patch

[Myles C. Maxfield]

Created attachment 267419 [details]
Patch

[Myles C. Maxfield]

It looks like the existing CoreText function is fast enough now.

[Sam Weinig]

Comment on attachment 267419 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Covered by existing tests.

This changelog could use information about what API you are switching to, and how it was performance tested (what tests? what was the speed up?)

[Darin Adler]

Comment on attachment 267419 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Covered by existing tests.
> 
> This changelog could use information about what API you are switching to, and how it was performance tested (what tests? what was the speed up?)

Could not agree more.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:656
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000)

Should have a blank line after this line.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:663
> +    CGSize advance = CGSizeZero;
> +    bool horizontal = platformData().orientation() == Horizontal;
> +    CTFontGetUnsummedAdvancesForGlyphsAndStyle(m_platformData.ctFont(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, renderingStyle(platformData()), &glyph, &advance, 1);
> +
> +    return advance.width + m_syntheticBoldOffset;

Slightly peculiar paragraphing. I think this would read better without a blank line.

Seems we should have a helper function that turns one orientation into the other without the intermediate step of converting to a boolean.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:712
> +#endif

Should have a blank line before this line.

[Ahmad Saleem]

It seems that similar was implemented by following 179957@main in bug 161119. I think we can mark this as duplicate of other.

[Tim Nguyen (:ntim)]

*** This bug has been marked as a duplicate of bug 161119 ***