Bug 15229 - [gtk] abstract font management by using pango
: [gtk] abstract font management by using pango
Status: RESOLVED FIXED
: WebKit
New Bugs
: 523.x (Safari 3)
: PC Mac OS X 10.4
: P2 Normal
Assigned To:
:
: Gtk
:
: 16942
  Show dependency treegraph
 
Reported: 2007-09-18 02:42 PST by
Modified: 2008-03-12 08:06 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-09-18 02:42:45 PST
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 From 2007-09-18 02:43:23 PST -------
I almost have a patch ready.
------- Comment #2 From 2007-09-21 05:14:38 PST -------
Created an attachment (id=16339) [details]
Proposed Patch v1

Proposed patch; applies to svn rev 25680.
------- Comment #3 From 2007-09-21 06:26:44 PST -------
(From update of attachment 16339 [details])
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 From 2007-09-21 08:03:51 PST -------
Created an attachment (id=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 From 2007-09-21 11:24:28 PST -------
(From update of attachment 16340 [details])
+    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 From 2007-09-22 10:56:58 PST -------
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 From 2007-10-21 17:03:47 PST -------
Hey, any word on when this patch will be updated to SVN head and marked for review?
------- Comment #8 From 2007-10-22 01:01:02 PST -------
This week
------- Comment #9 From 2007-12-18 21:58:34 PST -------
Created an attachment (id=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 From 2007-12-19 03:39:12 PST -------
Created an attachment (id=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 From 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 From 2007-12-19 09:54:34 PST -------
(From update of attachment 17981 [details])
Patch obsoleted by the next one.
------- Comment #13 From 2007-12-19 09:55:10 PST -------
(From update of attachment 17985 [details])
r=me with some whitespace cleanups.
------- Comment #14 From 2007-12-19 10:02:49 PST -------
Landed in r28864.
------- Comment #15 From 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 From 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 From 2007-12-22 01:26:29 PST -------
(From update of attachment 17985 [details])
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 From 2008-03-10 07:02:57 PST -------
Created an attachment (id=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 From 2008-03-10 07:14:10 PST -------
Created an attachment (id=19632) [details]
Add Pango font backend.

Fix configure.ac to use proper variable for font_backend default value.
------- Comment #20 From 2008-03-10 08:59:39 PST -------
Created an attachment (id=19635) [details]
Add Pango font backend.

Also guard against no font backend defined.
------- Comment #21 From 2008-03-10 11:42:12 PST -------
Created an attachment (id=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 From 2008-03-10 14:17:12 PST -------
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 From 2008-03-12 06:30:42 PST -------
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 From 2008-03-12 07:46:36 PST -------
(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 From 2008-03-12 08:01:47 PST -------
(From update of attachment 19639 [details])
r=me
------- Comment #26 From 2008-03-12 08:06:04 PST -------
Landed in r30989.

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