[webkit-reviews] review denied: [Bug 50619] [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise : [Attachment 213358] 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 213358: Patch
https://bugs.webkit.org/attachment.cgi?id=213358&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213358&action=review


Okay. Apologies for the *very* late review. This is looking a lot better. I
think you could reduce the repetition quite a bit. Let's also just turn on
OPENTYPE_VERTICAL by default on all Freetype ports. I don't think it's useful
to support multiple configurations. Another option is just to keep one code
path, but ensure that the orientation is never vertical when OPENTYPE_VERTICAL
is disabled.

> Source/WebCore/platform/graphics/FontCache.cpp:241
> +struct FontVerticalDataCacheKeyHash {
> +    static unsigned hash(const FontCache::FontFileKey& fontFileKey)
> +    {
> +	   return PtrHash<const FontCache::FontFileKey*>::hash(&fontFileKey);
> +    }
> +
> +    static bool equal(const FontCache::FontFileKey& a, const
FontCache::FontFileKey& b)
> +    {
> +	   return a == b;
> +    }
> +
> +    static const bool safeToCompareToEmptyOrDeleted = true;
> +};

You probably want to describe in the ChangeLog why the new hash function is
necessary.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:366
> +    // Reverse the byte order.

I think it makes sense to expand this commend to explain *why* you are
reversing the byte order as well.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:367
> +    uint32_t tag = FT_MAKE_TAG(table & 0xff,  table & 0xff00, table &
0xff0000, table & 0xff000000);

Extra space here.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:385
> +void FontPlatformData::setOrientation(FontOrientation orientation)

Is this ever called for us? The only place I see this called is in
SimpleFontData::verticalRightOrientationFontData, but that appears to only be
called by the Mac port.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:396
> +    cairo_font_face_t* fontFace;
> +    fontFace = cairo_scaled_font_get_font_face(m_scaledFont);

Should be one line.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:402
> +    cairo_font_options_t* options = getDefaultFontOptions();
> +    setCairoFontOptionsFromFontConfigPattern(options, m_pattern.get());

Not sure I understand why this is called again.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:410
> +    if (orientation == Vertical) {
> +	   // This transformation is the same as the one done at
initializeWithFontFace().
> +	   // V = H · R · T, where R rotates by -90 degrees and T translates
by font
> +	   // size towards y axis.
> +	   cairo_matrix_rotate(&fontMatrix, -M_PI_2);
> +	   cairo_matrix_translate(&fontMatrix, 0.0, 1.0);

If this is necessary (see above), then this should move into a helper
function/method.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:419
> +	   // The transformation matrix (H) for reversing the changes applied
for vertical
> +	   // orientation can be obtained by re-arranging the equation used to
calculate
> +	   // the matrix (V) for vertical orientation (e.g. V = H · R · T).
> +	   // V = H · R · T  ==>  V · (R · T)^-1 = H · (R · T) · (R ·
T)^-1  ==>  H = V · T^-1 · R^-1
> +	   // where R^-1 rotates by 90 degrees and T^-1 translates by font size
against y axis.
> +	   cairo_matrix_translate(&fontMatrix, 0.0, -1.0);
> +	   cairo_matrix_rotate(&fontMatrix, M_PI_2);
> +    }

Instead of trying to undo the rotation, why not either save the original matrix
or produce it again via a helper. That would allow you to eliminate this code.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98
> +	   cairo_scaled_font_text_extents(m_platformData.scaledFont(), "x",
&textExtents);
> +	   // In case of vertical orientation, glyphs are rotated and height is
actually width.
> +	   m_fontMetrics.setXHeight(narrowPrecisionToFloat(textExtents.width));

> +
> +	   cairo_scaled_font_text_extents(m_platformData.scaledFont(), " ",
&textExtents);
> +	   // In case of vertical orientation, width is based on the y_advance
as fonts are rotated -90 degrees
> +	   m_spaceWidth = narrowPrecisionToFloat(-textExtents.y_advance);
> +

This looks like a lot of repetition, so why not add two small helpers,
heightFromTextExtentsAndOrientation and widthFromTextExtentsAndOrientation.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:103
> +	   if (!isTextOrientationFallback()) {
> +	       FT_Face freeTypeFace =
cairo_ft_scaled_font_lock_face(m_platformData.scaledFont());
> +	       m_fontMetrics.setUnitsPerEm(freeTypeFace->units_per_EM);
> +	       cairo_ft_scaled_font_unlock_face(m_platformData.scaledFont());
> +	   }

Not sure I understand why this is necessary now...

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:198
> +    if (platformData().orientation() == Horizontal) {
> +	   if (cairo_scaled_font_status(m_platformData.scaledFont()) ==
CAIRO_STATUS_SUCCESS && extents.x_advance)
> +	       w = static_cast<float>(extents.x_advance);
> +    } else {
> +	   if (cairo_scaled_font_status(m_platformData.scaledFont()) ==
CAIRO_STATUS_SUCCESS && extents.y_advance) {
> +	       // In case of vertical orientation, width is based on y_advance
as fonts are rotated -90 degrees
> +	       w = static_cast<float>(-extents.y_advance);
> +	   }
> +    }

Or:

if (cairo_scaled_font_status(m_platformData.scaledFont()) ==
CAIRO_STATUS_SUCCESS && extents.x_advance)
    w = platformData().orientation() == Horizontal ? extents.x_advance :
-extents.y_advance;

or use the helper described above. :)

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:112
> +#if ENABLE(OPENTYPE_VERTICAL)
> +    if (!(*advance)) {
> +	   // There is no advance in x, so assuming this font has vertical
ghyphs.
> +	   *advance = doubleToHarfBuzzPosition(-glyphExtents.y_advance);
> +	   if (extents) {
> +	       extents->x_bearing =
doubleToHarfBuzzPosition(glyphExtents.x_bearing);
> +	       extents->y_bearing =
doubleToHarfBuzzPosition(-glyphExtents.y_bearing);
> +	       extents->width = doubleToHarfBuzzPosition(-glyphExtents.height);

> +	       extents->height = doubleToHarfBuzzPosition(glyphExtents.width);
> +	   }
> +    } else {
> +	   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);

> +	   }
> +    }

There's a lot of repetition here as well.