[Webkit-unassigned] [Bug 50619] [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 30 12:52:51 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=50619
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #213358|review? |review-
Flag| |
--- Comment #42 from Martin Robinson <mrobinson at webkit.org> 2013-10-30 12:51:35 PST ---
(From update of attachment 213358)
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.
--
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