[Webkit-unassigned] [Bug 50619] Glyphs in vertical text tests are rotated 90 degrees clockwise on WebKitGtk
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 7 07:56:53 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=50619
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #75796|1 |0
is obsolete| |
--- Comment #4 from Martin Robinson <mrobinson at webkit.org> 2010-12-07 07:56:53 PST ---
(From update of attachment 75796)
View in context: https://bugs.webkit.org/attachment.cgi?id=75796&action=review
Great start. I think we should initialize things properly up front instead of waiting until the render method. Is it possible to have Fontconfig to deliver a face with vertical glyps? That might avoid the need to call substituteWithVerticalGlyphs. Why must we call substituteWithVerticalGlyphs and also rotate the font matrix?
> WebCore/platform/graphics/Font.cpp:348
> + // 0x2C7 Caron, Mandrin Chinese 3rd Tone
> + // 0x2CA Modifier Letter Acute Accent, Mandarin Chinese 2nd Tone
> + // 0x2CB Modifier Letter Grave Access, Mandarin Chinese 4th Tone
> + // 0x2D9 Dot Above, Mandarin Chinese 5th Tone
> + if ((c == 0x2C7) || (c == 0x2CA) || (c == 0x2CB) || (c == 0x2D9))
> + return true;
> +
> // The basic CJK Unified Ideographs block.
I think the changes in Font.cpp should be separated out into a separate patch. This is logically a different problem than the lack of support in WebKItGTK+.
> WebCore/platform/graphics/cairo/FontCairo.cpp:109
> for (int i = 0; i < numGlyphs; i++) {
> glyphs[i].x = offset;
> glyphs[i].y = 0.0f;
> offset += glyphBuffer.advanceAt(from + i);
> + if (isVertical)
> + glyphs[i].x = offset;
Don't we need to update the y offset because we're laying the text out vertically? why are you setting the x-offset to the offset of the next offset?
> WebCore/platform/graphics/cairo/FontCairo.cpp:146
> + if (isVertical) {
> + cairo_get_font_matrix(cr, &matrix);
> + oldMatrix = matrix;
> + cairo_matrix_rotate(&matrix, M_PI*1.5);
> + cairo_set_font_matrix(cr, &matrix);
> + }
Why not rotate this matrix only once when the font is created?
> WebCore/platform/graphics/freetype/FontPlatformData.h:62
> FontPlatformData(cairo_font_face_t* fontFace, float size, bool bold, bool italic);
> + FontPlatformData(cairo_font_face_t* fontFace, float size, bool bold, bool italic, FontOrientation orientation = Horizontal);
Please replace the old constructor instead of adding a new one, if possible.
> WebCore/platform/graphics/freetype/FontPlatformData.h:76
> + FontOrientation orientation() const { return m_orientation; } // FIXME: Implement.
>
You should remove the FIXME now. :)
> WebCore/platform/graphics/freetype/FontPlatformData.h:102
> + FontOrientation m_orientation;
> cairo_scaled_font_t* m_scaledFont;
This will generate warnings because the field initializers in the constructors are initializing things in the wrong order. It's probably better to add this last. Can't it be private?
> WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:252
> && m_scaledFont == other.m_scaledFont && m_size == other.m_size
> - && m_syntheticOblique == other.m_syntheticOblique && m_syntheticBold == other.m_syntheticBold;
> + && m_syntheticOblique == other.m_syntheticOblique && m_syntheticBold == other.m_syntheticBold && m_orientation == other.m_orientation;
Nit: please even out the line lengths here.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:45
> +static bool substituteWithVerticalGlyphs(unsigned offset, unsigned length, const SimpleFontData* fontData, FT_Face face, GlyphPage *glyphPage)
The asterisk should be with GlyphPage.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:50
> + guint scriptIndex;
> + guint featureIndex;
Define these near where you first use them and initialize them.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:56
> + RefPtr<FcPattern> pattern = fontData->platformData().m_pattern;
It's not necessary to use a RefPtr here since we do not want to take a reference.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:64
> + PangoFontMap* pangoFM;
> + PangoContext* context;
> + PangoFontDescription* description;
> + PangoFont* pangoFont;
Define these when you first use them, so it's easy to see that they are not unintialized.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:66
> + pangoFM = pango_ft2_font_map_new();
Do not use abbreviations for the PangoFontMap variable name. pangoFontMap is fine.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:68
> + description = pango_font_description_from_string((char *)fontConfigFamilyName);
Please use static_cast here.
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:71
> + return false;
Don't you need to clean up the context and fontmap here?
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:74
> + if (!fcFont)
> + return false;
Don't you need to clean up the pangoFont here?
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:85
> + pango_ot_buffer_get_glyphs(pangoBuffer, &foo, &glyphLength);
Where is foo from? Wouldn't a more descriptive name be better here?
> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:88
> + return true;
Don't you need to clean up all of the objects you created here?
> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:79
> + FT_ULong length = 0;
> + FT_Load_Sfnt_Table(face, TTAG_vhea, 0, 0, &length);
> + if (length <= 0)
> + m_orientation = Horizontal;
> + }
I'm not sure if this is the proper way to handle errors. Shouldn't you instead load the original table? Also, maybe it's better to check the return value of the freetype function.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list