[webkit-reviews] review granted: [Bug 3315] CSS3: Implement @font-face rule (master bug) : [Attachment 16434] Patch that implements font-face on Mac and Windows

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 3315: CSS3: Implement @font-face rule (master bug)
http://bugs.webkit.org/show_bug.cgi?id=3315

Attachment 16434: Patch that implements font-face on Mac and Windows
http://bugs.webkit.org/attachment.cgi?id=16434&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
Is there a Ptr type (like OwnPtr) which could make more clear the fact that:
void addSource(CSSFontFaceSource*);
takes ownership of the source?

Yet another area of code where bug 15331 would be useful.

This comment makes the if feel backwards:
// If we are still loading, then we let the system pick a font.

I think you could just change the return type here to be AtomicString and it
would still perform the same "uniquing" trick that you seem to be after:

static String hashForFont(const String& familyName, bool bold, bool italic)
 69 {
 70	String familyHash(familyName);
 71	if (bold)
 72	    familyHash += "-webkit-bold";
 73	if (italic)
 74	    familyHash += "-webkit-italic";
 75	return AtomicString(familyHash);
 76 }

I believe these .get() calls are unnecessary:
 84	if (!fontFamily.get() || !src.get() || !fontFamily->isValueList() ||
!src->isValueList())
AFAIK RefPtr has a ! operator, as well as should be able to convert to a bool
automagically for things like ptr && otherPtr.

I would almost write a "static bool isNonEmptyValueList(CSSValue*)" function to
perform the checks, since it seems to take several lines each.	But such is
certainly not necessary.

Typo:
 251	     else if (familyName == "-webkit-fantasy")
 252		 genericFamily = settings->cursiveFontFamily();

It's hard to imagine we don't have a cleaner way to express this with the
string classes:
2480		 parsedValue = new
CSSFontFaceSrcValue(String(KURL(styleElement->baseURL().deprecatedString(),
value.deprecatedString()).url()), 

m_systemFallbackChild must have been leaking before and we didn't notice.  The
deletion in the destructor is new in this patch.

Isn't there a CFRetainPtr or something?  I could have sworn...
     if (f && objc_collecting_enabled())
 34	    CFRetain(f); // fix for <rdar://problems/4223619>

That's all.  Other than Mitz's comments, it looks great.  I very much look
forward to seeing this on feature-branch. :)