Bug 15229 – [gtk] abstract font management by using pango

• Component: New Bugs
• Status: RESOLVED FIXED

[Sven Herzberg]

Right now, the FontData and FontPlatformData are really specific for Linux/X11. To enable webkit/gtk on other platforms, we should switch to an approach using pango as far as possible.

[Sven Herzberg]

I almost have a patch ready.

[Sven Herzberg]

Created attachment 16339 [details]
Proposed Patch v1

Proposed patch; applies to svn rev 25680.

[Mark Rowe (bdash)]

Comment on attachment 16339 [details]
Proposed Patch v1

I didn't look very closely at the actual code in the patch as I'm not particularly familiar with font handling in WebCore, but I will point out a few areas in which the code doesn't match the WebKit style guidelines (<http://webkit.org/coding/coding-style.html>).

* Many newly-added function calls have unneeded whitespace between the function name and the opening parenthesis.

* There are several places where you have extra whitespace around, or incorrect placement of, the * in declarations of pointer variables.  In several places you have extra whitespace around the = operator in assignments.  More generally, we don't typically use whitespace to line up assignments and declarations.

* In several places you have broken function calls across multiple lines for no apparent reason.  In cases where it's wrapped due to one argument being the result of a function call, it may be preferable to use a local variable to simplify the code rather than jamming it all into the function call.  In other cases, having all the arguments on the same line may be preferable.

* When checking if a pointer is NULL, !theVariable is preferred over theVariable == 0.

* Declaring local variables at the point of first use is preferred over declaring at the top of a function then initializing later.


Sorry for being so picky!  Hopefully someone can review the real meat of your patch soon.

[Sven Herzberg]

Created attachment 16340 [details]
Proposed Patch v2

(In reply to comment #3)
> * Many newly-added function calls have unneeded whitespace between the function
> name and the opening parenthesis.

Fixed.

> * There are several places where you have extra whitespace around, or incorrect
> placement of, the * in declarations of pointer variables.  In several places
> you have extra whitespace around the = operator in assignments.  More
> generally, we don't typically use whitespace to line up assignments and
> declarations.

Fixed.

> * In several places you have broken function calls across multiple lines for no
> apparent reason.  In cases where it's wrapped due to one argument being the
> result of a function call, it may be preferable to use a local variable to
> simplify the code rather than jamming it all into the function call.  In other
> cases, having all the arguments on the same line may be preferable.

Fixed. I just didn't fix this for two calls:
FontPlatformDataGdk.cpp: FontPlatformData::init(),
GlyphPageTreeNodeGdk.cpp: GlyphPage::fill() - introducing a local variable will require adding braces - which in turn will lead up to code that looks almost as scrammed as it does now.

> * When checking if a pointer is NULL, !theVariable is preferred over
> theVariable == 0.

I haven't done that, it was a plain int that I compared to, but this is fixed too.

> * Declaring local variables at the point of first use is preferred over
> declaring at the top of a function then initializing later.

Fixed.

> Sorry for being so picky!  Hopefully someone can review the real meat of your
> patch soon.

Well, thanks for the review.

[Adam Roben (:aroben)]

Comment on attachment 16340 [details]
Proposed Patch v2

+    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.

[Alp Toker]

This is looking good. I'd actually like to keep the old (non-Pango) font backend code around in the tree so we can continue to do performance comparisons and possibly make use of it as a fallback in certain cases. It could also be useful for avoiding the Pango dependency in any new ports that use Cairo but not Gtk+.

[Alp Toker]

Hey, any word on when this patch will be updated to SVN head and marked for review?

[Sven Herzberg]

This week

[Sven Herzberg]

Created attachment 17981 [details]
Proposed patch (v3)

This patch is updated for the new backend name (GTK instead of GDK) and the new source locations (WebCore/platform/graphics/gtk instead of WebCore/platform/gdk). Also, most of the issues should be fixed

[Richard Hult]

Created attachment 17985 [details]
Updated patch that applies cleanly

The v3 patch doesn't apply cleanly for me, I had to tweak the last chunk.

[Mikkel Kruse Johnsen]

Just compiled on mac with the latest patch and it works. I now have text.

http://www.linet.dk/screenshots/WebKitGtk-MacOSX-Leopard.png

Using jhbuild with native Gtk for mac. http://developer.imendio.com/projects/gtk-macosx/build-instructions

[Alp Toker]

Comment on attachment 17981 [details]
Proposed patch (v3)

Patch obsoleted by the next one.

[Alp Toker]

Comment on attachment 17985 [details]
Updated patch that applies cleanly

r=me with some whitespace cleanups.

[Alp Toker]

Landed in r28864.

[Alp Toker]

Sven: The change broke the build so I had to throw together this fix:

  http://trac.webkit.org/projects/webkit/changeset/28876

Can you look over it and see if it can be made more correct? We need to support older versions of Pango, but should aim to use the best code path available as your patch does too.

[Alp Toker]

Commits r28880, r28876, r28865, r28864 were reverted in r28921 to fix this P1 bug:

  http://bugs.webkit.org/show_bug.cgi?id=16542
  [GTK] Text is missing with old Pango version

These patches will need to be fixed to work with older Pango versions (using #if macros where necessary) since most of our users don't have bleeding edge setups. Future work should be based on the versions of the patches that were landed rather than the ones in this bug report since they were modified to include coding style fixes before landing.

[Mark Rowe (bdash)]

Comment on attachment 17985 [details]
Updated patch that applies cleanly

Clearing r+ flag as the patch as-is not suitable to be landed, and the change in which it was rolled out should be used as the basis for future work.

[Xan Lopez]

Created attachment 19631 [details]
Pango font backend.

Took Herzi's patch and massaged it into a new optional font backend as discussed with alp. The existing backend is called the 'freetype' backend, although the files are still suffixed with Gtk.

I've done two code changes:
- Use pango_cairo_font_map_get_default to create the font map instead of pango_cairo_font_map_new.
- Free the hash table in the destructor.

The custom font stuff is unimplemented in the pango backend.

[Xan Lopez]

Created attachment 19632 [details]
Add Pango font backend.

Fix configure.ac to use proper variable for font_backend default value.

[Xan Lopez]

Created attachment 19635 [details]
Add Pango font backend.

Also guard against no font backend defined.

[Xan Lopez]

Created attachment 19639 [details]
Add Pango font backend.

Do not free the hash table for each instance, it's a class member... (we are leaking this again now, but I don't know where to free it).

Remove dead code from FontCustomPlatformDataPango.cpp.

[Xan Lopez]

Just one comment: discussed the hash table leakage with Sven and Tim Janik, and while it's technically leaked it does not really matter because we'll imediately exit the program afterwards (being a class variable).

[Richard Hult]

I've just tried the latest incarnation of the patch on GTK+ OS X and it works nicely (I couldn't get it building with qmake though, only auto*). Thanks!

[Alp Toker]

(In reply to comment #23)
> I've just tried the latest incarnation of the patch on GTK+ OS X and it works
> nicely (I couldn't get it building with qmake though, only auto*). Thanks!
> 

Time to get this patch in ;-)

qmake is no longer supported btw.

[Alp Toker]

Comment on attachment 19639 [details]
Add Pango font backend.

r=me

[Alp Toker]

Landed in r30989.

This will need more work to become the default font backend.