[webkit-reviews] review denied: [Bug 28131] [Haiku] Adding font-specific files to WebCore. : [Attachment 34556] Patch to add four 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 34556: Patch to add four font-specific files to
WebCore/platform/graphics/haiku/.
https://bugs.webkit.org/attachment.cgi?id=34556&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I would have put this in a static inline:

+    int32 numStyles = count_font_styles(family);
+    font_style fontStyle;
+    uint32 flags;
+
+    for (int i = 0; i < numStyles; i++) {
+	 if (get_font_style(family, i, &fontStyle, &flags) == B_OK) {
+	     String style(fontStyle);
+	     if ((m_oblique && m_bold) && (style == "BoldItalic" || style ==
"Bold Oblique"))
+		  break;
+	     if ((m_oblique && !m_bold) && (style == "Italic" || style ==
"Oblique"))
+		  break;
+	     if ((!m_oblique && m_bold) && style == "Bold")
+		  break;
+	     if ((!m_oblique && !m_bold) && (style == "Roma" || style == "Book"

+		 || style == "Condensed" || style == "Regular" || style ==
"Medium"))
+		  break;
+	     }
+	 }

Something like findMatchingFontStyle(family, m_bold, m_oblique);


found is not used here:
+    if (BFont().SetFamilyAndStyle(BString(familyName.string()).String(), 0) ==
B_OK) {
+	 strcpy(family, BString(familyName.string()).String());
+	 found = true;
+    }
+
+    // If no font family is found for the given name, we use a generic font.
+    if (!found) {
+	 int type = fontDescription.genericFamily();
+
+	 if (BString(familyName.string()).FindFirst("Sans") != B_ERROR)
+	     strcpy(family, "DejaVu Sans");
+	 else if (BString(familyName.string()).FindFirst("Serif") != B_ERROR)
+	     strcpy(family, "DejaVu Serif");
+	 else if (BString(familyName.string()).FindFirst("Mono") != B_ERROR)
+	     strcpy(family, "DejaVu Mono");
+	 else // This is the fallback font.
+	     strcpy(family, "DejaVu Sans");
+    }

Seems that should just be an else.

Again, I would have put the family lookup in a separate static inline.

font_family family = findMatchingFontFamily(familyName);

I think this could use on more round of cleanup.  In general, smaller functions
are better. :)