Summary: | [Chromium] Change OOP Font loading code to use CGFont*() APIs | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Moskovich <playmobil> | ||||||||||
Component: | WebCore Misc. | Assignee: | Jeremy Moskovich <playmobil> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, eric, hyatt, mark, mitz, rniwa, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jeremy Moskovich
2011-08-25 06:36:13 PDT
Created attachment 105173 [details]
Patch
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. Whoops 10.5 -> 10.7 . Created attachment 105606 [details]
Patch
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. Created attachment 106313 [details]
Patch
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 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. Created attachment 106722 [details]
Patch
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 on attachment 106722 [details]
Patch
It appears this change is almost entirely red lines now.
Comment on attachment 106722 [details] Patch Clearing flags on attachment: 106722 Committed r94936: <http://trac.webkit.org/changeset/94936> All reviewed patches have been landed. Closing bug. |