[webkit-reviews] review denied: [Bug 37508] [Haiku] Rework FontPlatformData implementation : [Attachment 54037] [Haiku] Reimplementation of FontPlatformData.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 37508: [Haiku] Rework FontPlatformData implementation
https://bugs.webkit.org/show_bug.cgi?id=37508

Attachment 54037: [Haiku] Reimplementation of FontPlatformData.
https://bugs.webkit.org/attachment.cgi?id=54037&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
WebCore/ChangeLog:1
 +  2010-04-13	Stephan Aßmus	<superstippi at gmx.de>
Is this character in your name correct or do we have some sort of encoding
problem?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:44
 +	    if (familyName.contains("Sans", false) != B_ERROR)
This seems really hacky.  What if there's a non-san-serif font that happens to
contain the string "sans"?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:81
 +  // #pragma mark -
What does this comment mean?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:91
 +	void ref();
Why doesn't this inherit from RefCounted?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:107
 +	: m_refCount(1)
Usually we have a static FontPlatformData::create method to help people not
leak because of initializing the refcount to 1.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:152
 +  // #pragma mark -
This again.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:202
 +	    m_data->ref();
Please don't use manual reference counting like this.  We have lots of tools to
avoid these error-prone patterns.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:217
 +	     m_data->deref();
Bad indent.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:231
 +	    || isHashTableDeletedValue() || other.isHashTableDeletedValue()) {
Why is this clause split in a strange place?

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:243
 +	return &(m_data->m_font);
No parens needed.

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:290
 +	return m_data == reinterpret_cast<FontPlatformDataPrivate*>(-1);
Yuck

WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp:309
 +	return String(fontFamily) + "/" + String(fontStyle) +
String::format("/%.1f/%d&%d", size, isBold, isOblique);
Why do you use concatenation for part of this string but String::format for the
rest?  Please pick one or the other.