[webkit-reviews] review denied: [Bug 26949] WebCore part of the Haiku WebKit port : [Attachment 32924] Patch to add Haiku-specific font files for WebCore/platform/graphics/.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 7 11:05:03 PDT 2009


Eric Seidel <eric at webkit.org> has denied Maxime Simon
<simon.maxime at gmail.com>'s request for review:
Bug 26949: WebCore part of the Haiku WebKit port
https://bugs.webkit.org/show_bug.cgi?id=26949

Attachment 32924: Patch to add Haiku-specific font files for
WebCore/platform/graphics/.
https://bugs.webkit.org/attachment.cgi?id=32924&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
What is this and why?
 // Fix temp routine to check the conversion works, please move it ASAP    
 44 int charUnicodeToUtf8(unsigned short glyph, char* out)

It's not mentioned in the ChangeLog.  I needs a more decriptive name if it's
just a hack.

convertUTF16ToUTF8RenderingHACK or something.

We don't use argument names in headers when they don't add value:
47	   FontPlatformData(const FontDescription& fontDescription, const
AtomicString& family);
 48	    FontPlatformData(const FontDescription& fontDescription, BFont*
font);
 49	    FontPlatformData(float size, bool bold, bool italic);
 50	    FontPlatformData(const FontPlatformData& fontPlatformData);
"family" can stay.  but the fontDescription at least shoudl go.

No hungarian notation here ;)
 66	bool bFound = false;
 67	bool bItalic = fontDescription.italic();
 68	bool bBold = fontDescription.weight() == FontWeightBold;
We ain't no microsoft...

Why isn't this an OwnPtr<BFont>:
 63	    BFont* m_font;

Your leaking BFonts.

Why is this correct?
	if (isUtf16) {
 50		UChar lead = characterBuffer[i * 2];
 51		UChar trail = characterBuffer[i * 2 + 1];
 52		character = U16_GET_SUPPLEMENTARY(lead, trail);
 53	    } else

Not all characters will be composed characters.

Use the height of the X, can't you ask the font for the height of a glyph at a
certain size?
 52	    m_xHeight = font->Size(); // Not sure about this

This information should be on the BFont:
 53	    m_unitsPerEm = 1; // FIXME!

You'd have more luck posting this in smaller pices.  Some of this was just
fine, but some of the font code is wrong here too.  If you broke it out, I
coudl more easily approve the OK parts.


More information about the webkit-reviews mailing list