[webkit-reviews] review denied: [Bug 15610] [GTK] Text rendering using Pango : [Attachment 18330] pangorendering.diff

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 8 15:28:37 PST 2008


Alp Toker <alp at atoker.com> has denied Xan Lopez <xan.lopez at gmail.com>'s request
for review:
Bug 15610: [GTK] Text rendering using Pango
http://bugs.webkit.org/show_bug.cgi?id=15610

Attachment 18330: pangorendering.diff
http://bugs.webkit.org/attachment.cgi?id=18330&action=edit

------- Additional Comments from Alp Toker <alp at atoker.com>
This works really well. There are a few coding style issues to be sorted out
before it can be landed though.

>diff --git a/WebCore/platform/graphics/gtk/FontGtk.cpp
b/WebCore/platform/graphics/gtk/FontGtk.cpp
>index 3b3a228..790d762 100644
>--- a/WebCore/platform/graphics/gtk/FontGtk.cpp
>+++ b/WebCore/platform/graphics/gtk/FontGtk.cpp
>@@ -1,6 +1,10 @@
> /*
>  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
>  * Copyright (C) 2006 Michael Emmel mike.emmel at gmail.com 
>+ * Copyright (c) 2007 Hiroyuki Ikezoe All rights reserved.
>+ * Copyright (c) 2007 Kouhei Sutou All rights reserved.
>+ * Copyright (C) 2007 Alp Toker <alp at atoker.com>
>+ * Copyright (C) 2008 Xan Lopez <xan at gnome.org>
>  * All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>@@ -33,12 +37,144 @@
> #include "SimpleFontData.h"
> #include <cairo.h>
> 
>+#include <pango/pango.h>
>+#include <pango/pangocairo.h>
>+
> namespace WebCore {
> 
>+/* From Mozilla, with small changes */
>+
>+#define IS_HIGH_SURROGATE(u)	((UChar)(u) >= (UChar)0xd800 && (UChar)(u) <=
(UChar)0xdbff)
>+#define IS_LOW_SURROGATE(u)  ((UChar)(u) >= (UChar)0xdc00 && (UChar)(u) <=
(UChar)0xdfff)
>+
>+static void
>+utf16_to_utf8 (const UChar* aText, gint aLength, char *&text, gint &length)

Should be no space before (

>+{
>+  gboolean need_copy = FALSE;
>+  int i;
>+
>+  for (i = 0; i < aLength; i++) {

You can put the int i within the for statement.

>+    if (!aText[i] || IS_LOW_SURROGATE(aText[i])) {
>+	need_copy = TRUE;
>+	break;
>+    }
>+    else if (IS_HIGH_SURROGATE(aText[i])) {
>+	if (i < aLength - 1 && IS_LOW_SURROGATE(aText[i+1]))
>+	  i++;
>+	else {
>+	  need_copy = TRUE;
>+	  break;
>+	}
>+    }
>+  }
>+
>+  if (need_copy) {
>+
>+    /* Pango doesn't correctly handle nuls.  We convert them to 0xff. */
>+    /* Also "validate" UTF-16 text to make sure conversion doesn't fail. */
>+
>+    UChar *p = (UChar *)g_memdup(aText, aLength * sizeof(aText[0]));

Watch out for the position of *.

>+
>+    /* don't need to reset i */
>+    for (i = 0; i < aLength; i++) {
>+	if (!p[i] || IS_LOW_SURROGATE(p[i]))
>+	  p[i] = 0xFFFD;
>+	else if (IS_HIGH_SURROGATE(p[i])) {
>+	  if (i < aLength - 1 && IS_LOW_SURROGATE(aText[i+1]))
>+	    i++;
>+	  else
>+	    p[i] = 0xFFFD;
>+	}
>+    }
>+
>+    aText = p;
>+  }
>+
>+  glong items_written;
>+  text = g_utf16_to_utf8(aText, aLength, NULL, &items_written, NULL);
>+  length = items_written;
>+
>+  if (need_copy)
>+    g_free((gpointer) aText);
>+
>+}
>+
>+// FIXME: having this code here is silly
>+static gchar* convertUniCharToUTF8(const UChar* characters, gint length, int
from, int to)
>+{
>+    gchar *utf8 = 0;
>+    gint new_length = 0;
>+    utf16_to_utf8(characters, length, utf8, new_length);
>+    if (!utf8) return NULL;
>+
>+    if (from > 0) {
>+	  // discard the first 'from' characters 
>+	  // FIXME: we should do this before the conversion probably
>+	  gchar *str_left = g_utf8_offset_to_pointer(utf8, from);
>+	  gchar *tmp = g_strdup(str_left);
>+	  g_free(utf8);
>+	  utf8 = tmp;
>+    }
>+
>+    gchar *pos = utf8;
>+    gint len = strlen(pos);
>+    GString *ret = g_string_new_len(NULL, len);
>+
>+    // replace line break by space
>+    while (len > 0) {
>+	  gint index, start;
>+	  pango_find_paragraph_boundary(pos, len, &index, &start);
>+	  g_string_append_len(ret, pos, index);
>+	  if (index == start)
>+	      break;
>+	  g_string_append_c(ret, ' ');
>+	  pos += start;
>+	  len -= start;
>+    }
>+    return g_string_free(ret, FALSE);
>+}
>+
>+static void setPangoAttributes(const Font* font, const TextRun& run,
PangoLayout* layout)
>+{
>+    PangoAttrList* list = pango_attr_list_new();
>+    PangoAttribute* attr;
>+
>+    attr = pango_attr_size_new_absolute((int)(font->size() * PANGO_SCALE));
>+    attr->end_index = G_MAXUINT;
>+    pango_attr_list_insert_before(list, attr);
>+
>+    if (!run.spacingDisabled()) {
>+	  attr = pango_attr_letter_spacing_new(font->letterSpacing() *
PANGO_SCALE);
>+	  attr->end_index = G_MAXUINT;
>+	  pango_attr_list_insert_before(list, attr);
>+    }
>+
>+    // Pango does not yet support synthesising small caps
>+    /*
>+    printf ("small caps: %d\n", font->isSmallCaps());
>+
>+    if (font->isSmallCaps()) {
>+	  attr = pango_attr_variant_new(PANGO_VARIANT_SMALL_CAPS);
>+	  attr->end_index = G_MAXUINT;
>+	  pango_attr_list_insert_before(list, attr);
>+    }
>+    */

Commented-out code is against coding style, though I personally wouldn't mind
leaving this snippet around so it doesn't get lost. If you want to remove it,
you could replace it with a link to this bug and a comment.

>+
>+    pango_layout_set_attributes(layout, list);
>+    pango_attr_list_unref(list);
>+
>+    pango_layout_set_auto_dir(layout, FALSE);
>+
>+    PangoContext* pango_context = pango_layout_get_context(layout);
>+    PangoDirection direction = run.rtl() ? PANGO_DIRECTION_RTL :
PANGO_DIRECTION_LTR;
>+    pango_context_set_base_dir(pango_context, direction);
>+}
>+
> void Font::drawGlyphs(GraphicsContext* graphicsContext, const SimpleFontData*
font, const GlyphBuffer& glyphBuffer,
>			int from, int numGlyphs, const FloatPoint& point) const

> {
>     cairo_t* context = graphicsContext->platformContext();
>+    cairo_save(context);
> 
>     // Set the text color to use for drawing.
>     float red, green, blue, alpha;
>@@ -59,26 +195,86 @@ void Font::drawGlyphs(GraphicsContext* graphicsContext,
const SimpleFontData* fo
>	  offset += glyphBuffer.advanceAt(from + i);
>     }
>     cairo_show_glyphs(context, glyphs, numGlyphs);
>+
>+    cairo_restore(context);
> }
> 
>-void Font::drawComplexText(GraphicsContext*, const TextRun&, const
FloatPoint&, int from, int to) const
>+void Font::drawComplexText(GraphicsContext* context, const TextRun& run,
const FloatPoint& point, int from, int to) const
> {
>-    notImplemented();
>+    cairo_t* cr = context->platformContext();
>+    cairo_save(cr);
>+
>+    PangoLayout* layout = pango_cairo_create_layout(cr);
>+
>+    gchar* utf8 = convertUniCharToUTF8(run.characters(), run.length(), from,
to);
>+    pango_layout_set_text(layout, utf8, -1);
>+    g_free(utf8);
>+
>+    setPangoAttributes(this, run, layout);
>+
>+    // Set the text color to use for drawing.
>+    float red, green, blue, alpha;
>+    Color penColor = context->fillColor();
>+    penColor.getRGBA(red, green, blue, alpha);
>+    cairo_set_source_rgba(cr, red, green, blue, alpha);
>+
>+    // Our layouts are single line
>+    cairo_move_to(cr, point.x(), point.y());
>+    PangoLayoutLine *layout_line = pango_layout_get_line_readonly(layout, 0);

>+    pango_cairo_show_layout_line(cr, layout_line);
>+
>+    g_object_unref(layout);
>+    cairo_restore(cr);
> }
> 
>-float Font::floatWidthForComplexText(const TextRun&) const
>+float Font::floatWidthForComplexText(const TextRun& run) const
> {
>-    notImplemented();
>-    return 0.0f;
>+    if (run.length() == 0)
>+	  return 0.0f;
>+
>+    // FIXME: we should create the layout with our actual context
>+    PangoFontMap* map = pango_cairo_font_map_get_default();
>+    PangoContext* pango_context =
pango_cairo_font_map_create_context(PANGO_CAIRO_FONT_MAP(map));
>+    PangoLayout* layout = pango_layout_new(pango_context);
>+    g_object_unref(pango_context);

This chunk could move into a static helper function and be re-used.

Also, I'd recommend removing FIXME/TODOs or clarifying them.


More information about the webkit-reviews mailing list