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.
I almost have a patch ready.
Created attachment 16339 [details] Proposed Patch v1 Proposed patch; applies to svn rev 25680.
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.
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 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.
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+.
Hey, any word on when this patch will be updated to SVN head and marked for review?
This week
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
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.
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 on attachment 17981 [details] Proposed patch (v3) Patch obsoleted by the next one.
Comment on attachment 17985 [details] Updated patch that applies cleanly r=me with some whitespace cleanups.
Landed in r28864.
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.
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 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.
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.
Created attachment 19632 [details] Add Pango font backend. Fix configure.ac to use proper variable for font_backend default value.
Created attachment 19635 [details] Add Pango font backend. Also guard against no font backend defined.
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.
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).
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!
(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 on attachment 19639 [details] Add Pango font backend. r=me
Landed in r30989. This will need more work to become the default font backend.