Bug 17727 – [wx] Implement non-kerned drawing on wxGTK w/wxGC

• Component: WebKit wx
• Status: RESOLVED FIXED

[Kevin Ollivier]

wx currently only draws text kerned, so we need to provide our own implementation of non-kerned drawing there.

[Kevin Ollivier]

Created attachment 19611 [details]
Implementation for wxGC mode

Here is the implementation using wxGC (which is basically Cairo under the hood).

[Alp Toker]

Hey, we have three copies of some of this code already, and there is at least one bug bug in the part you ended up copying (which we now have in three places). Wonder if we can find a better way to share this Cairo code. It really is mostly identical for each port.

[Kevin Ollivier]

I much prefer sharing code as well, but which copy should I base the shared code off of? I see there are now Pango and non-Pango versions... We (wx) can use Pango I think, but does GTK need to support both? 

[Darin Adler]

Comment on attachment 19611 [details]
Implementation for wxGC mode

r=me

[Darin Adler]

Kevin, why haven't you landed this yet?

[Kevin Ollivier]

Sorry, there were a couple issues I found with this patch that turned out to be really hard to resolve, which in turn led me to think the way I approached this problem might not have been the right one. I actually did recently get a chance to redo the implementation using Pango which gives much better results. Should I just clear the review field for this patch until I've got the new one ready to upload? 

[Kevin Ollivier]

Created attachment 24701 [details]
new patch using PANGO instead of FT for better results

Here's the new patch I promised. This one has been better tested and doesn't have the issues the old one did.

[Adam Roben (:aroben)]

Comment on attachment 24701 [details]
new patch using PANGO instead of FT for better results

> +++ WebCore/platform/graphics/GlyphBuffer.h	(working copy)
> @@ -33,7 +33,7 @@
>  
>  #if PLATFORM(CG)
>  #include <ApplicationServices/ApplicationServices.h>
> -#elif PLATFORM(CAIRO)
> +#elif PLATFORM(CAIRO) || (PLATFORM(WX) && defined(__WXGTK__))

Should we just make PLATFORM(CAIRO) be defined globally when building wxgtk?

>  float SimpleFontData::platformWidthForGlyph(Glyph glyph) const
>  {
> +#if __WXGTK__ && USE(WXGC)
> +    PangoFont* pangoFont = createPangoFontForFont(m_font.font());

Do you need to free this font somewhere? I guess WXGC might mean "garbage collection"?

It seems kind of strange that all non-wxgtk/wxgc cases will fall into the Windows-only code path.

> +++ WebCore/platform/wx/wxcode/gtk/non-kerned-drawing.cpp	(working copy)
> @@ -33,15 +33,163 @@
>  #include <wx/gdicmn.h>
>  #include <vector>
>  
> +#if USE(WXGC)
> +#include <cairo.h>
> +#include <assert.h>
> +
> +#include <pango/pango.h>
> +#include <pango/pangocairo.h>
> +
> +// Use cairo-ft i a recent enough Pango version isn't available

Typo: i -> if

> +PangoFontMap* g_fontMap = 0;

This should be marked static. That will give it internal linkage. Once you do that you won't have to explicitly initialize to 0, either.

> +PangoFontMap* pangoFontMap()
> +{
> +    return g_fontMap;
> +}

Should this function perform initialization of g_fontMap?

> +
> +PangoFont* createPangoFontForFont(const wxFont wxfont)
> +{
> +const char* face = wxfont.GetFaceName().mb_str(wxConvUTF8);
> +    char const* families[] = {
> +      face,
> +      NULL
> +    };

Indentation seems wrong here. We also use 0 instead of NULL in C++ code, though maybe this file is trying to follow wx coding style?

> +
> +   switch (wxfont.GetFamily()) {
> +        case wxFONTFAMILY_ROMAN:
> +            families[1] = "serif";
> +            break;
> +        case wxFONTFAMILY_SWISS:
> +            families[1] = "sans";
> +            break;
> +        case wxFONTFAMILY_MODERN:
> +            families[1] = "monospace";
> +            break;
> +        default:
> +            families[1] = "sans";
> +    }
> +    
> +    PangoFontDescription* description = pango_font_description_new();
> +    pango_font_description_set_absolute_size(description, wxfont.GetPointSize() * PANGO_SCALE);
> + 
> +    PangoFont* pangoFont = 0;
> +    PangoContext* pangoContext = 0;
> +    // FIXME: Map all FontWeight values to Pango font weights.
> +    if (wxfont.GetWeight() == wxFONTWEIGHT_BOLD)
> +        pango_font_description_set_weight(description, PANGO_WEIGHT_BOLD);
> +    if (wxfont.GetStyle() == wxFONTSTYLE_ITALIC)
> +        pango_font_description_set_style(description, PANGO_STYLE_ITALIC);
> +
> +    if (!g_fontMap)
> +        g_fontMap = pango_cairo_font_map_get_default();
> +
> +    pangoContext = pango_cairo_font_map_create_context(PANGO_CAIRO_FONT_MAP(g_fontMap));

> +    for (unsigned int i = 0; !pangoFont && i < G_N_ELEMENTS(families); i++) {

We normally just say "unsigned" instead of "unsigned int".

> +        pango_font_description_set_family(description, "sans"); //families[i]);

This looks wrong.

> +PangoGlyph pango_font_get_glyph(PangoFont* font, PangoContext* context, gunichar wc)
> +{
> +    PangoGlyph result = 0;
> +    gchar buffer[7];
> +
> +    gint  length = g_unichar_to_utf8(wc, buffer);
> +    g_return_val_if_fail(length, 0);
> +
> +    GList* items = pango_itemize(context, buffer, 0, length, NULL, NULL);
> +
> +    if (g_list_length(items) == 1) {
> +        PangoItem* item = reinterpret_cast<PangoItem*>(items->data);

static_cast would be better here.

>  void drawTextWithSpacing(GraphicsContext* graphicsContext, const SimpleFontData* font, const wxColour& color, const GlyphBuffer& glyphBuffer, int from, int numGlyphs, const FloatPoint& point)
>  {
>  #if USE(WXGC)
>      wxGCDC* dc = static_cast<wxGCDC*>(graphicsContext->platformContext());
> +    wxGraphicsContext* gc = dc->GetGraphicsContext();
> +    gc->PushState();
> +    cairo_t* cr = (cairo_t*)gc->GetNativeContext();
> +
> +    wxFont wxfont = font->getWxFont();
> +    PangoFont* pangoFont = createPangoFontForFont(wxfont);
> +    PangoContext* pangoContext = pango_cairo_font_map_create_context(PANGO_CAIRO_FONT_MAP(g_fontMap));
> +    cairo_scaled_font_t* scaled_font = createScaledFontForFont(wxfont); 
> +    ASSERT(scaled_font);
> +
> +    cairo_glyph_t* glyphs = NULL;
> +    glyphs = (cairo_glyph_t*)malloc (sizeof(cairo_glyph_t) * numGlyphs);

Extra space after "malloc". static_cast would be better than a C-style cast.

>  wxFontProperties::wxFontProperties(wxFont* font):
>  m_ascent(0), m_descent(0), m_lineGap(0), m_lineSpacing(0), m_xHeight(0)
>  {
> +#if USE(WXGC)
> +    cairo_font_extents_t font_extents;
> +    cairo_text_extents_t text_extents;
> +    cairo_scaled_font_t* scaled_font = WebCore::createScaledFontForFont(*font);
> +    
> +    cairo_scaled_font_extents(scaled_font, &font_extents);
> +    m_ascent = static_cast<int>(font_extents.ascent);
> +    m_descent = static_cast<int>(font_extents.descent);
> +    m_lineSpacing = static_cast<int>(font_extents.height);
> +    cairo_scaled_font_text_extents(scaled_font, "x", &text_extents);
> +    m_xHeight = text_extents.height;
> +    cairo_scaled_font_text_extents(scaled_font, " ", &text_extents);
> +    //m_spaceWidth =  static_cast<int>(text_extents.x_advance);

We don't like to commit commented-out code.

r- so this can be cleaned up a little more before going in.

[Kevin Ollivier]

Created attachment 25665 [details]
Revised patch with Adam's comments addressed

[Kevin Ollivier]

(In reply to comment #8)
> (From update of attachment 24701 [details] [review])
> > +++ WebCore/platform/graphics/GlyphBuffer.h	(working copy)
> > @@ -33,7 +33,7 @@
> >  
> >  #if PLATFORM(CG)
> >  #include <ApplicationServices/ApplicationServices.h>
> > -#elif PLATFORM(CAIRO)
> > +#elif PLATFORM(CAIRO) || (PLATFORM(WX) && defined(__WXGTK__))
> 
> Should we just make PLATFORM(CAIRO) be defined globally when building wxgtk?

We do not use all of the Cairo port, in fact we don't use most of it. As a result, we'd probably actually end up with many more #ifdefs, mostly #if PLATFORM(CAIRO) && !PLATFORM(WX), for things that the wx port implements separately from Cairo.

These special platform-specific defines will eventually go away, as this code is eventually going to be part of wx once we craft a proper API for it. If we move it there now, though, wxWebKit could no longer be used with the current stable release (2.8.9) and there aren't any releases of the 2.9 cycle yet, which would cause problems for some projects currently using wxWebKit.

> >  float SimpleFontData::platformWidthForGlyph(Glyph glyph) const
> >  {
> > +#if __WXGTK__ && USE(WXGC)
> > +    PangoFont* pangoFont = createPangoFontForFont(m_font.font());
> 
> Do you need to free this font somewhere? I guess WXGC might mean "garbage
> collection"?

No, sorry, I overlooked this. WXGC actually stands for wxGraphicsContext, which is a frontend to the various vector-based drawing APIs (GDI+, Cairo, and Core Graphics). 

> It seems kind of strange that all non-wxgtk/wxgc cases will fall into the
> Windows-only code path.

The fallback is actually a wx codepath, not Windows, and in the latest path I removed the special case so that there's no special case for wxGTK/Cairo in SimpleFontData. The necessary code was moved into WebCore/platform/wx/wxcode/gtk/fontprops.cpp

> > +++ WebCore/platform/wx/wxcode/gtk/non-kerned-drawing.cpp	(working copy)
> > @@ -33,15 +33,163 @@
> >  #include <wx/gdicmn.h>
> >  #include <vector>
> >  
> > +#if USE(WXGC)
> > +#include <cairo.h>
> > +#include <assert.h>
> > +
> > +#include <pango/pango.h>
> > +#include <pango/pangocairo.h>
> > +
> > +// Use cairo-ft i a recent enough Pango version isn't available
> 
> Typo: i -> if
> 
> > +PangoFontMap* g_fontMap = 0;
> 
> This should be marked static. That will give it internal linkage. Once you do
> that you won't have to explicitly initialize to 0, either.

Done, thanks!

> > +PangoFontMap* pangoFontMap()
> > +{
> > +    return g_fontMap;
> > +}
> 
> Should this function perform initialization of g_fontMap?

yeah, this is definitely better. I've implemented that in the latest patch.

> > +
> > +PangoFont* createPangoFontForFont(const wxFont wxfont)
> > +{
> > +const char* face = wxfont.GetFaceName().mb_str(wxConvUTF8);
> > +    char const* families[] = {
> > +      face,
> > +      NULL
> > +    };
> 
> Indentation seems wrong here. We also use 0 instead of NULL in C++ code, though
> maybe this file is trying to follow wx coding style?

No, not sure how that got messed up, but I fixed it.

> > +
> > +   switch (wxfont.GetFamily()) {
> > +        case wxFONTFAMILY_ROMAN:
> > +            families[1] = "serif";
> > +            break;
> > +        case wxFONTFAMILY_SWISS:
> > +            families[1] = "sans";
> > +            break;
> > +        case wxFONTFAMILY_MODERN:
> > +            families[1] = "monospace";
> > +            break;
> > +        default:
> > +            families[1] = "sans";
> > +    }
> > +    
> > +    PangoFontDescription* description = pango_font_description_new();
> > +    pango_font_description_set_absolute_size(description, wxfont.GetPointSize() * PANGO_SCALE);
> > + 
> > +    PangoFont* pangoFont = 0;
> > +    PangoContext* pangoContext = 0;
> > +    // FIXME: Map all FontWeight values to Pango font weights.
> > +    if (wxfont.GetWeight() == wxFONTWEIGHT_BOLD)
> > +        pango_font_description_set_weight(description, PANGO_WEIGHT_BOLD);
> > +    if (wxfont.GetStyle() == wxFONTSTYLE_ITALIC)
> > +        pango_font_description_set_style(description, PANGO_STYLE_ITALIC);
> > +
> > +    if (!g_fontMap)
> > +        g_fontMap = pango_cairo_font_map_get_default();
> > +
> > +    pangoContext = pango_cairo_font_map_create_context(PANGO_CAIRO_FONT_MAP(g_fontMap));
> 
> > +    for (unsigned int i = 0; !pangoFont && i < G_N_ELEMENTS(families); i++) {
> 
> We normally just say "unsigned" instead of "unsigned int".

Okay, done.

> > +        pango_font_description_set_family(description, "sans"); //families[i]);
> 
> This looks wrong.

Yeah, I think I added that when debugging and forgot to undo it. :( Fixed now.

> > +PangoGlyph pango_font_get_glyph(PangoFont* font, PangoContext* context, gunichar wc)
> > +{
> > +    PangoGlyph result = 0;
> > +    gchar buffer[7];
> > +
> > +    gint  length = g_unichar_to_utf8(wc, buffer);
> > +    g_return_val_if_fail(length, 0);
> > +
> > +    GList* items = pango_itemize(context, buffer, 0, length, NULL, NULL);
> > +
> > +    if (g_list_length(items) == 1) {
> > +        PangoItem* item = reinterpret_cast<PangoItem*>(items->data);
> 
> static_cast would be better here.

Done.

> >  void drawTextWithSpacing(GraphicsContext* graphicsContext, const SimpleFontData* font, const wxColour& color, const GlyphBuffer& glyphBuffer, int from, int numGlyphs, const FloatPoint& point)
> >  {
> >  #if USE(WXGC)
> >      wxGCDC* dc = static_cast<wxGCDC*>(graphicsContext->platformContext());
> > +    wxGraphicsContext* gc = dc->GetGraphicsContext();
> > +    gc->PushState();
> > +    cairo_t* cr = (cairo_t*)gc->GetNativeContext();
> > +
> > +    wxFont wxfont = font->getWxFont();
> > +    PangoFont* pangoFont = createPangoFontForFont(wxfont);
> > +    PangoContext* pangoContext = pango_cairo_font_map_create_context(PANGO_CAIRO_FONT_MAP(g_fontMap));
> > +    cairo_scaled_font_t* scaled_font = createScaledFontForFont(wxfont); 
> > +    ASSERT(scaled_font);
> > +
> > +    cairo_glyph_t* glyphs = NULL;
> > +    glyphs = (cairo_glyph_t*)malloc (sizeof(cairo_glyph_t) * numGlyphs);
> 
> Extra space after "malloc". static_cast would be better than a C-style cast.

Done. :)

> >  wxFontProperties::wxFontProperties(wxFont* font):
> >  m_ascent(0), m_descent(0), m_lineGap(0), m_lineSpacing(0), m_xHeight(0)
> >  {
> > +#if USE(WXGC)
> > +    cairo_font_extents_t font_extents;
> > +    cairo_text_extents_t text_extents;
> > +    cairo_scaled_font_t* scaled_font = WebCore::createScaledFontForFont(*font);
> > +    
> > +    cairo_scaled_font_extents(scaled_font, &font_extents);
> > +    m_ascent = static_cast<int>(font_extents.ascent);
> > +    m_descent = static_cast<int>(font_extents.descent);
> > +    m_lineSpacing = static_cast<int>(font_extents.height);
> > +    cairo_scaled_font_text_extents(scaled_font, "x", &text_extents);
> > +    m_xHeight = text_extents.height;
> > +    cairo_scaled_font_text_extents(scaled_font, " ", &text_extents);
> > +    //m_spaceWidth =  static_cast<int>(text_extents.x_advance);
> 
> We don't like to commit commented-out code.

Yes, sorry, another oversight. Fixed now.

> r- so this can be cleaned up a little more before going in.
> 

Thanks a lot for taking a look at this! 

[Diggilin]

Any chance this can get the fast lane?
wxWebKit is useless under linux without it...

[Nikolas Zimmermann]

Adding Adam to the CC list, in case he forgot about this long-pending bug.

[Diggilin]

Created attachment 27588 [details]
Updated the patch to support recent changes and satisfied a FIXME

[Kevin Ollivier]

Landed in r40965, thanks! :-)