Bug 15229 - [gtk] abstract font management by using pango
Summary: [gtk] abstract font management by using pango
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC OS X 10.4
: P2 Normal
Assignee: Sven Herzberg
URL:
Keywords: Gtk
Depends on:
Blocks: 16942
  Show dependency treegraph
 
Reported: 2007-09-18 02:42 PDT by Sven Herzberg
Modified: 2008-03-12 08:06 PDT (History)
7 users (show)

See Also:


Attachments
Proposed Patch v1 (15.75 KB, patch)
2007-09-21 05:14 PDT, Sven Herzberg
no flags Details | Formatted Diff | Diff
Proposed Patch v2 (15.57 KB, patch)
2007-09-21 08:03 PDT, Sven Herzberg
aroben: review-
Details | Formatted Diff | Diff
Proposed patch (v3) (16.58 KB, patch)
2007-12-18 21:58 PST, Sven Herzberg
no flags Details | Formatted Diff | Diff
Updated patch that applies cleanly (16.58 KB, patch)
2007-12-19 03:39 PST, Richard Hult
no flags Details | Formatted Diff | Diff
Pango font backend. (29.36 KB, patch)
2008-03-10 07:02 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add Pango font backend. (29.37 KB, patch)
2008-03-10 07:14 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add Pango font backend. (29.67 KB, patch)
2008-03-10 08:59 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add Pango font backend. (28.88 KB, patch)
2008-03-10 11:42 PDT, Xan Lopez
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sven Herzberg 2007-09-18 02:42:45 PDT
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.
Comment 1 Sven Herzberg 2007-09-18 02:43:23 PDT
I almost have a patch ready.
Comment 2 Sven Herzberg 2007-09-21 05:14:38 PDT
Created attachment 16339 [details]
Proposed Patch v1

Proposed patch; applies to svn rev 25680.
Comment 3 Mark Rowe (bdash) 2007-09-21 06:26:44 PDT
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.
Comment 4 Sven Herzberg 2007-09-21 08:03:51 PDT
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.
Comment 5 Adam Roben (:aroben) 2007-09-21 11:24:28 PDT
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.
Comment 6 Alp Toker 2007-09-22 10:56:58 PDT
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+.
Comment 7 Alp Toker 2007-10-21 17:03:47 PDT
Hey, any word on when this patch will be updated to SVN head and marked for review?
Comment 8 Sven Herzberg 2007-10-22 01:01:02 PDT
This week
Comment 9 Sven Herzberg 2007-12-18 21:58:34 PST
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
Comment 10 Richard Hult 2007-12-19 03:39:12 PST
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.
Comment 11 Mikkel Kruse Johnsen 2007-12-19 04:17:41 PST
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
Comment 12 Alp Toker 2007-12-19 09:54:34 PST
Comment on attachment 17981 [details]
Proposed patch (v3)

Patch obsoleted by the next one.
Comment 13 Alp Toker 2007-12-19 09:55:10 PST
Comment on attachment 17985 [details]
Updated patch that applies cleanly

r=me with some whitespace cleanups.
Comment 14 Alp Toker 2007-12-19 10:02:49 PST
Landed in r28864.
Comment 15 Alp Toker 2007-12-19 15:25:12 PST
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.
Comment 16 Alp Toker 2007-12-20 16:38:55 PST
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.
Comment 17 Mark Rowe (bdash) 2007-12-22 01:26:29 PST
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.
Comment 18 Xan Lopez 2008-03-10 07:02:57 PDT
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.
Comment 19 Xan Lopez 2008-03-10 07:14:10 PDT
Created attachment 19632 [details]
Add Pango font backend.

Fix configure.ac to use proper variable for font_backend default value.
Comment 20 Xan Lopez 2008-03-10 08:59:39 PDT
Created attachment 19635 [details]
Add Pango font backend.

Also guard against no font backend defined.
Comment 21 Xan Lopez 2008-03-10 11:42:12 PDT
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.
Comment 22 Xan Lopez 2008-03-10 14:17:12 PDT
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).
Comment 23 Richard Hult 2008-03-12 06:30:42 PDT
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!
Comment 24 Alp Toker 2008-03-12 07:46:36 PDT
(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.
Comment 25 Alp Toker 2008-03-12 08:01:47 PDT
Comment on attachment 19639 [details]
Add Pango font backend.

r=me
Comment 26 Alp Toker 2008-03-12 08:06:04 PDT
Landed in r30989.

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