Bug 66935 - [Chromium] Change OOP Font loading code to use CGFont*() APIs
Summary: [Chromium] Change OOP Font loading code to use CGFont*() APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Moskovich
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-25 06:36 PDT by Jeremy Moskovich
Modified: 2011-09-11 20:13 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.52 KB, patch)
2011-08-25 06:40 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2011-08-30 03:02 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (11.60 KB, patch)
2011-09-05 01:15 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (11.55 KB, patch)
2011-09-08 04:49 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2011-08-25 06:36:13 PDT
This change is necessary due a bug in ATSFontDeactivate() on 10.5. See crbug.com/93191 for details.
Comment 1 Jeremy Moskovich 2011-08-25 06:40:10 PDT
Created attachment 105173 [details]
Patch
Comment 2 Mark Mentovai 2011-08-25 11:29:57 PDT
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.
Comment 3 Jeremy Moskovich 2011-08-30 02:56:09 PDT
Whoops 10.5 -> 10.7 .
Comment 4 Jeremy Moskovich 2011-08-30 03:02:05 PDT
Created attachment 105606 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-08-30 12:18:01 PDT
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.
Comment 6 Jeremy Moskovich 2011-09-05 01:15:28 PDT
Created attachment 106313 [details]
Patch
Comment 7 Jeremy Moskovich 2011-09-05 01:18:30 PDT
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?
Comment 8 Eric Seidel (no email) 2011-09-06 13:51:59 PDT
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.
Comment 9 Jeremy Moskovich 2011-09-08 04:49:16 PDT
Created attachment 106722 [details]
Patch
Comment 10 Jeremy Moskovich 2011-09-08 04:51:38 PDT
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.
Comment 11 Eric Seidel (no email) 2011-09-08 08:10:33 PDT
Comment on attachment 106722 [details]
Patch

It appears this change is almost entirely red lines now.
Comment 12 WebKit Review Bot 2011-09-11 20:13:49 PDT
Comment on attachment 106722 [details]
Patch

Clearing flags on attachment: 106722

Committed r94936: <http://trac.webkit.org/changeset/94936>
Comment 13 WebKit Review Bot 2011-09-11 20:13:54 PDT
All reviewed patches have been landed.  Closing bug.