[webkit-reviews] review denied: [Bug 50619] [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise : [Attachment 216051] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Denis Nomiyama (dnomi)
<d.nomiyama at samsung.com>'s request for review:
Bug 50619: [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise
https://bugs.webkit.org/show_bug.cgi?id=50619

Attachment 216051: Patch
https://bugs.webkit.org/attachment.cgi?id=216051&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.