Bug 145234 – [Cocoa] Use CTFontDrawGlyphs() instead of CGContextShowGlyphsWithAdvances()/CGContextShowGlyphsAtPositions()

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] Use CTFontDrawGlyphs() instead of CGContextShowGlyphsWithAdvances()/CGContextShowGlyphsAtPositions()

[Myles C. Maxfield]

Created attachment 253480 [details]
Patch

[Jon Lee]

Comment on attachment 253480 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:-386
> -    CGAffineTransform matrix = useLetterpressEffect || platformData.isColorBitmapFont() ? CGAffineTransformIdentity : CGAffineTransformMakeScale(fontSize, fontSize);

Does the scale end up being applied in CTFontDrawGlyphs?

[Myles C. Maxfield]

Created attachment 253494 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 253480 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:-386
>> -    CGAffineTransform matrix = useLetterpressEffect || platformData.isColorBitmapFont() ? CGAffineTransformIdentity : CGAffineTransformMakeScale(fontSize, fontSize);
> 
> Does the scale end up being applied in CTFontDrawGlyphs?

No. Our current code draws the text at a size of 1pt into a scaled context. However, this approach is wrong when using CTFontDrawGlyphs() because CT will set the font size for us. Therefore, if we don't get rid of this context scaling, all text appears as if its point size was squared. (Note that [drawFont matrix] is just a scale by fontSize).

[Enrica Casucci]

Comment on attachment 253494 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:-219
> -        CGContextSetTextMatrix(context, savedMatrix);

I'm not sure I understand why it is ok to remove the line above.

> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:386
>  #endif

Could you explain this piece of code? From what I understand you removed a font scaling transform, but you also removed line 392. I won't be able to review this unless I fully understand it.

[Myles C. Maxfield]

Comment on attachment 253494 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:-219
>> -        CGContextSetTextMatrix(context, savedMatrix);
> 
> I'm not sure I understand why it is ok to remove the line above.

Upon investigation, it appears that it isn't okay. The fact that no tests failed is troubling. I will investigate more.

>> Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:386
>>  #endif
> 
> Could you explain this piece of code? From what I understand you removed a font scaling transform, but you also removed line 392. I won't be able to review this unless I fully understand it.

See my previous comment to jonlee. I think that should explain it.

[Myles C. Maxfield]

(In reply to comment #4)
> (Note that [drawFont matrix] is just a scale by fontSize).

This is incorrect. It is often just a scale, but it might not be.

[Myles C. Maxfield]

Created attachment 253561 [details]
Patch

[Enrica Casucci]

Comment on attachment 253561 [details]
Patch

Looks good now.

[Myles C. Maxfield]

Committed r184796: <http://trac.webkit.org/changeset/184796>

[Antti Koivisto]

This might have been 2% PLT regression. Can we roll out to confirm?

[Myles C. Maxfield]

But I measured it :(

Go ahead, but make sure you also roll out http://trac.webkit.org/changeset/184832 first.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145449

[Myles C. Maxfield]

Rolled out in https://bugs.webkit.org/show_bug.cgi?id=145449

[Myles C. Maxfield]

This is done.

[Radar WebKit Bug Importer]

<rdar://problem/82116941>