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

[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 26949: WebCore part of the Haiku WebKit port
https://bugs.webkit.org/show_bug.cgi?id=26949

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
Why the prefix "pc"?
65     const BFont* pcDefaultFont = be_plain_font;

Are you sure you meant to levae this in?
 72	printf("FontCache::getLastResortFallbackFont -> %s\n", family);

Spacing:
 74	BView* view = static_cast<BView*> (graphicsContext->platformContext());


Style:
 50	    FontPlatformData(const FontPlatformData& f);
 48	    FontPlatformData(const FontDescription&, BFont* font);

count_font_families() sounds expensive. :)
 71	for(int i = 0; i < count_font_families(); i++) {

style:
 73	    if(!BString(familyName.string()).Compare(family)){

MOre style:
 90		if ( get_font_style(family, i, &style, &flags) == B_OK) {
 91		    if((bItalic && bBold) &&  (!strcmp( style, "BoldItalic")
 92			    || !strcmp(style, "Bold Oblique"))) {


You could also convert "style" to be a String and then style == "Bold Oblique"
would read a bit better.

Style:
53	   } else {
 54		character = characterBuffer[i];
 55	    }

Doesn't ICU, and other parts of WebCore already have code for dealign with
this:
	 if(isUtf16) {
 50		UChar lead = characterBuffer[i * 2];
 51		UChar trail = characterBuffer[i * 2 + 1];
 52		character = U16_GET_SUPPLEMENTARY(lead, trail);
 53	    } else {
 54		character = characterBuffer[i];
 55	    } 

Style:
 46	BFont *font = m_platformData.font();