[Webkit-unassigned] [Bug 50619] [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50619


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #77168|review?                     |review-
               Flag|                            |




--- Comment #15 from Martin Robinson <mrobinson at webkit.org>  2010-12-24 11:10:09 PST ---
(From update of attachment 77168)
View in context: https://bugs.webkit.org/attachment.cgi?id=77168&action=review

This patch is looking better and better! I still have a few concerns though.

> WebCore/platform/graphics/cairo/FontCairo.cpp:56
> +static void rotateBackIfNecessary(cairo_t *context, const SimpleFontData* font)

The placement of the asterisk on "context" is incorrect.

> WebCore/platform/graphics/cairo/FontCairo.cpp:60
> +    if (isHorizontalOnly) {

It's probably better to use an early return here like (untested!):

if ((!font->platformData().orientation() == Vertical) || (!font->orientation() == Horizontal))
    return;

> WebCore/platform/graphics/cairo/FontCairo.cpp:63
> +        cairo_matrix_rotate(&fontMatrix, M_PI*0.5);

Is there any reason this is better than M_PI / 2?

> WebCore/platform/graphics/cairo/FontCairo.cpp:70
> +    rotateBackIfNecessary(context, font);

I think this can be avoided.

> WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:278
> +        cairo_matrix_rotate(&fontMatrix, -M_PI*0.5);

Again, is this better in some way than "/ 2"?

> WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:48
> +static bool substituteWithVerticalGlyphs(unsigned offset, unsigned length, const SimpleFontData* fontData, FT_Face face, GlyphPage* glyphPage)

Is this called for many glyphs? If so is there a lot of overhead for doing this over and over again for each one?

> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:59
> +    // if the font doesn't have vertical metrics, we don't rotate it 90 degress counterclockwise 
> +    if (m_orientation == Vertical) {
> +        FT_Face face = cairo_ft_scaled_font_lock_face(m_platformData.scaledFont());
> +        if (!FT_HAS_VERTICAL(face))
> +            m_orientation = Horizontal;
> +        cairo_ft_scaled_font_unlock_face(m_platformData.scaledFont());
> +    }
> +

I still think it might be better to do this check in  FontPlatformData::initializeWithFontFace so that we can avoid undoing the rotation in drawGlyphs above.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.