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

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> 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 32371: Patch to add Haiku-specific font files for
WebCore/platform/graphics/.
https://bugs.webkit.org/attachment.cgi?id=32371&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>

> +// Fix temp routine to check the conversion works, please move it ASAP      

> +int charUnicodeToUtf8(unsigned short glyph, char* out)
> +{
> +    int i = 0;
> +
> +    if (glyph < 0x0080)
> +	   out[i++] = (char)glyph;
C++ style casts please :D

> +    else if (glyph < 0x0800) { //two bytes
> +	   out[i++] = 0xc0 | (( glyph ) >> 6 );
> +	   out[i++] = 0x80 | (( glyph ) & 0x3F );
> +    } else if (glyph > 0x0800) { //3 bytes
> +	   out[i++] = 0xe0 | (( glyph ) >> 12 );
> +	   out[i++] = 0x80 | ( (( glyph ) >> 6 ) & 0x3F );
> +	   out[i++] = 0x80 | (( glyph ) & 0x3F);
> +    }
Should have excess braces, eg.
out[i++] = 0x80 | ( (( glyph ) >> 6 ) & 0x3F ); => out[i++] = 0x80 | ((glyph >>
6 ) & 0x3F );
Shouldn't have spaces around the content of parenthesis, eg.
out[i++] = 0x80 | ( (( glyph ) >> 6 ) & 0x3F ); => out[i++] = 0x80 | ((glyph >>
6) & 0x3F);


> +FontPlatformData::FontPlatformData(const FontDescription& fontDescription,
const AtomicString& familyName)
> +    : m_font(0)
> +{
> +    bool bFound = false;
> +    bool bItalic = fontDescription.italic();
> +    bool bBold = fontDescription.weight() == FontWeightBold;
> +    font_family family;
> +
> +    for (int i = 0; i < count_font_families(); i++) {
> +	   get_font_family(i, &family, 0);
> +	   if (!BString(familyName.string()).Compare(family)) {
> +	       m_font = new BFont();
> +	       m_font->SetFamilyAndStyle(family, 0);
> +	       m_font->SetSize(fontDescription.computedSize());
> +
> +	       bFound = true;
> +	       break;
> +	   }
> +    }
> +
> +    if (bFound) {
> +	   int32 numStyles = count_font_styles(family);
> +	   font_style fontStyle;
> +	   uint32 flags;
> +	   bFound = false;
> +
> +	   for (int i = 0; i < numStyles; i++) {
> +	       if (get_font_style(family, i, &fontStyle, &flags) == B_OK) {
> +		   String style(fontStyle);
> +		   if ((bItalic && bBold)
> +		       && (style == "BoldItalic" || style == "Bold Oblique")) {

There's no reason for this line break, we don't have an 80-cloumn limit in our
style guidelines :D

> +			bFound = true;
> +			break;
> +		    }
> +		   if ((bItalic && !bBold)
> +		       && (style == "Italic" || style == "Oblique")) {
ditto

...
> +		    }
> +		   if ((!bItalic && !bBold)
> +		       && (style == "Roma"
> +			   || style == "Book" || style == "Condensed"
> +			   || style == "Regular" || style == "Medium")) {
You may want multiple lines here for clarity, but 4 lines seems excessive