[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 #216051|review?, commit-queue-      |review-
               Flag|                            |




--- Comment #53 from Martin Robinson <mrobinson at webkit.org>  2013-11-05 12:32:18 PST ---
(From update of attachment 216051)
View in context: https://bugs.webkit.org/attachment.cgi?id=216051&action=review

Looking pretty good. I have some stylistic concerns, but you should also try to regenerate pixel/test results for tests that use vertical writing modes.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:42
> +namespace {
> +

This anonymous namespace is unnecessary.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:106
> -static cairo_font_options_t* getDefaultFontOptions()
> +cairo_font_options_t* getDefaultFontOptions()

This should remain static.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:118
> +void rotateCairoMatrixForVerticalOrientation(cairo_matrix_t* matrix)

This should be static.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:173
> -    cairo_glyph_t cglyph = { glyph, 0, 0 };
> -    cairo_text_extents_t extents;
> -    cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cglyph, 1, &extents);
> +    float w = 0.0;
> +    if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS) {
> +        cairo_glyph_t cglyph = { glyph, 0, 0 };
> +        cairo_text_extents_t extents;
> +        cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cglyph, 1, &extents);
> +        w = static_cast<float>(platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance);
> +    }
>  
> -    float w = (float)m_spaceWidth;
> -    if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance)
> -        w = (float)extents.x_advance;
> +    if (!static_cast<unsigned>(w))
> +        w = static_cast<float>(m_spaceWidth);
>  

Please use early returns, avoid casting, and abbreviated variable names. For instance:

if (cairo_scaled_font_status(m_platformData.scaledFont()) != CAIRO_STATUS_SUCCESS)
    return m_spaceWidth;

cairo_glyph_t cairoGlyph = { glyph, 0, 0 };
cairo_text_extents_t extents;
cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cairoGlyph, 1, &extents);
float width = platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance;
return width ? width : m_spaceWidth;

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:92
> +    // If no advance in x, assume font has vertical ghyphs.

ghyphs -> glyphs

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:101
> -        *advance = doubleToHarfBuzzPosition(glyphExtents.x_advance);
> +        *advance = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.x_advance : -glyphExtents.y_advance);
> +
>      if (extents) {
>          extents->x_bearing = doubleToHarfBuzzPosition(glyphExtents.x_bearing);
> -        extents->y_bearing = doubleToHarfBuzzPosition(glyphExtents.y_bearing);
> -        extents->width = doubleToHarfBuzzPosition(glyphExtents.width);
> -        extents->height = doubleToHarfBuzzPosition(glyphExtents.height);
> +        extents->y_bearing = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.y_bearing : -glyphExtents.y_bearing);
> +        extents->width = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.width : -glyphExtents.height);
> +        extents->height = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.height : glyphExtents.width);
>      }

Certainly these casts can be avoided? It's probably best to save the results of the test into a boolean:

bool hasVerticalGlyphs = glyphExtents.x_advance;

This will allow you to remove the comment too.

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