Bug 66935 – [Chromium] Change OOP Font loading code to use CGFont*() APIs

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jeremy Moskovich]

This change is necessary due a bug in ATSFontDeactivate() on 10.5. See crbug.com/93191 for details.

[Jeremy Moskovich]

Created attachment 105173 [details]
Patch

[Mark Mentovai]

Comment on attachment 105173 [details]
Patch

> diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog

> +        This change is necessary due a bug in ATSFontDeactivate() on 10.5.

Not true. ATSFontDeactivate did not appear to have this bug (based on disassembly) in 10.5. The bug is present in 10.6, but we never observed it there (or it never happened often enough to cause us to take notice). It is present on 10.7 and occurs very frequently there. You should revise the ChangeLog message to not mislead.

>diff --git a/Source/WebKit/chromium/ChangeLog b/Source/WebKit/chromium/ChangeLog

Same in this file.

Assuming it works, LGTM on the code.

[Jeremy Moskovich]

Whoops 10.5 -> 10.7 .

[Jeremy Moskovich]

Created attachment 105606 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 105606 [details]
Patch

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

I think this is fine.  It could be better with more RetainPtr usage.

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:135
> +    CGFontRef cgFont;

I think ideally we'd use a RetainPtr here.

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:150
> +        CGFontRelease(cgFont);

Then this goes away.

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:162
> +  MemoryActivatedFont* font = new MemoryActivatedFont(fontID, nsFont, cgFont);
>    return adoptRef(font);

You don't need the local here.

[Jeremy Moskovich]

Created attachment 106313 [details]
Patch

[Jeremy Moskovich]

Eric: Thanks for the review, would you mind taking another look. Does my use of RetainPtr look logical to you? Specifically the argument passing and the fact I'm not using the AdoptCF tag?

[Eric Seidel (no email)]

Comment on attachment 106313 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.h:75
> +    static PassRefPtr<MemoryActivatedFont> create(uint32_t fontID, NSFont*, RetainPtr<CGFontRef>);

Do we not have a PassRetainPtr?  I'm really not a RetainPtr expert, I just know they're strictly superior to manual memory management.

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:160
> +MemoryActivatedFont::MemoryActivatedFont(uint32_t fontID, NSFont* nsFont, RetainPtr<CGFontRef> cgFont)

I think you can jus thave ethis be a CGFontRef, I'm not sure we gain anything by forcing the caller to use a RetainPtr.

[Jeremy Moskovich]

Created attachment 106722 [details]
Patch

[Jeremy Moskovich]

Thanks Eric, there is no PassRetainPtr to the best of my knowledge (http://www.webkit.org/coding/RefPtr.html lists the omission as an area that needs to be documented).

I'm going to land this patch in the coming days if there are no objections.

[Eric Seidel (no email)]

Comment on attachment 106722 [details]
Patch

It appears this change is almost entirely red lines now.

[WebKit Review Bot]

Comment on attachment 106722 [details]
Patch

Clearing flags on attachment: 106722

Committed r94936: <http://trac.webkit.org/changeset/94936>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.