[webkit-reviews] review denied: [Bug 17727] [wx] Implement non-kerned drawing on wxGTK w/wxGC : [Attachment 24701] new patch using PANGO instead of FT for better results

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 17727: [wx] Implement non-kerned drawing on wxGTK w/wxGC
https://bugs.webkit.org/show_bug.cgi?id=17727

Attachment 24701: new patch using PANGO instead of FT for better results
https://bugs.webkit.org/attachment.cgi?id=24701&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +++ 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.