[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