[webkit-reviews] review denied: [Bug 28131] [Haiku] Adding font-specific files to WebCore. : [Attachment 34422] Patch to add four other font-specific files to WebCore/platform/graphics/haiku/.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 28131: [Haiku] Adding font-specific files to WebCore.
https://bugs.webkit.org/show_bug.cgi?id=28131

Attachment 34422: Patch to add four other font-specific files to
WebCore/platform/graphics/haiku/.
https://bugs.webkit.org/attachment.cgi?id=34422&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
naming style:
 71	static AtomicString s_DefaultStr(family);

This should indicate that it's a hack somehow:
1 // Fix temp routine to check the conversion works, please move it ASAP
 42 int charUnicodeToUtf8(unsigned short glyph, char* out)
it's not clear to me from reading that method name that this method isn't a
good thing. :)	Although since I started at it enought o see what it does,
you're basically just flattening all passed in chars to utf8 so that you can
pass them off to B drawing routines.  This is a hack, and needs to be
designated as such.

Shouldn't char* out be char[4]& out?

Don't we already have typedefs making these casts uneeded?
2     BView* view = static_cast<BView*>(graphicsContext->platformContext());
 73	BFont* m_font = static_cast<BFont*>(font->platformData().font());

Why is the const_cast needed?
 79	GlyphBufferGlyph* glyphs =
const_cast<GlyphBufferGlyph*>(glyphBuffer.glyphs(from));

Early return is preferred:
45     BFont* font = m_platformData.font();
 46	if (font) {
if (!font)
    return;

It seems the handling of m_smallCapsFontData is wrong on other ports.  But I'm
not convinced your handling is correct either.	It certainly is strange that
SimpleFontData.cpp would try to delete it too.

     if (m_platformData.font())
 90	    m_treatAsFixedPitch = m_platformData.font()->IsFixed();
 91	else
 92	    m_treatAsFixedPitch = false;

m_treadAsFixedPitch = m_platformData.font() &&
m_platformData.font()->IsFixed();

What is an "escapement"?  Wow a B-only term it seems:
http://www.acm.uiuc.edu/bug/Be%20Book/The%20Interface%20Kit/Font.html