[webkit-reviews] review denied: [Bug 15229] [gtk] abstract font management by using pango : [Attachment 16340] Proposed Patch v2

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has denied Sven Herzberg <sven at imendio.com>'s
request for review:
Bug 15229: [gtk] abstract font management by using pango
http://bugs.webkit.org/show_bug.cgi?id=15229

Attachment 16340: Proposed Patch v2
http://bugs.webkit.org/attachment.cgi?id=16340&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
+    static PangoFontMap* m_fontMap;
+    static GHashTable	* m_hashTable;

These can just become static globals in FontPlatformDataGdk.cpp.

+    gchar const* families[] = {
+	 familyName.domString().utf8().data(),

I'm worried that this will give you a junk pointer. String::utf8 returns a
CString, and CString::data returns a pointer to its internal null-terminated
string, but the temporary CString immediately goes out of scope, which should
free the buffer. I think you'll need to put the CString in a local variable.

+	 PangoFontFamily**families = 0;

You're missing a space after the **.

+	 int n_families = 0;

We'd normally call this familyCount or something like that.

+	 pango_font_map_list_families (m_fontMap, &families, &n_families);
+		 g_hash_table_insert (m_hashTable,

Please remove the spaces before the open parenthesis.

+    bool result(pango_font_description_equal(thisDesc, otherDesc));

I think you should just use the assignment operator here.

+static PangoGlyph pango_font_get_glyph (PangoFont* font, PangoContext*
context, gunichar wc)

Please remove the space before the open parenthesis.

+    gint  length = g_unichar_to_utf8(wc, buffer);

Looks like you've got an extra space after gint.

+    GList* items = pango_itemize(context, buffer, 0, length, NULL, NULL);
+    g_list_foreach(items, (GFunc)pango_item_free, NULL);

Since this is C++ code, please use 0 instead of NULL.

+	     g_warning ("didn't get 1 glyph but %d", glyphs->num_glyphs);

I think it would be better to use our LOG() macros here.

There are a few other places that have a space before the parameter list of a
function that I haven't explicitly called out here.

I don't think adding braces around the for loops would make the code seem
crammed. I do think it would allow you to make the function calls match our
coding style better, though.

This looks sane overall (though I don't know Pango), but we definitely at least
need to fix the CString issue.