[webkit-reviews] review denied: [Bug 26620] Haiku WebKit port : [Attachment 32076] Patch to add Haiku-specific files for WebCore/platform/graphics/.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 2 14:49:50 PDT 2009


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

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
What is a styleable port?
 45	// FIXME: subtract 1 from height and width? This was not done in
Syllable port

I don't think you meant to leaven this in:
 52	printf("FontCache::getFontDataForCharacters\n");    

WebKit generally uses 0 for NULL in C++ code:
 72	pcDefaultFont->GetFamilyAndStyle(&family, NULL);

Please use c++ style casts:
 76	//return new FontPlatformData(fontDescription, (BFont*)pcDefaultFont);

We don't commit commented out code:
 76	//return new FontPlatformData(fontDescription, (BFont*)pcDefaultFont);

Style:
83     if (!pcData->font())
 84	{

You should consider using an OwnPtr here:
 82	FontPlatformData* pcData = new FontPlatformData(fontDescription,
family);

Style:
 51 int char_unicode_to_utf8(unsigned short glyph, char* out) {

Tabs will cause the commit to fail:
54	if (glyph < 0x0080)
 55		out[i++] = (char)glyph;

Indent:
{
 74	notImplemented();
 75	return true;
 76 }

Why?
 43 int char_unicode_to_utf8(unsigned short glyph, char* out); // implemented
in FontDataHaiku.cpp

Please use C++ style casts:
 57	BView* view = (BView*)graphicsContext->platformContext();

Double converting to float:
 float Font::floatWidthForComplexText(const TextRun& run, HashSet<const
SimpleFontData*>* fallbackFonts) const
 83 {
 84	notImplemented();
 85	return 3.0;

Style:
59     if (f.m_font)
 60	{
 61	    m_font = new BFont(f.m_font);
 62	}

Extra spaces and bad variable name:
 71	font_family	fam;

STyle:
 74	for(int i = 0; i < count_font_families(); i++) {


Please break this up into smaller patches.

Please do fewer patches per-bug.


More information about the webkit-reviews mailing list