[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





--- Comment #54 from Denis Nomiyama (dnomi) <d.nomiyama at samsung.com>  2013-11-06 11:35:21 PST ---
(From update of attachment 216051)
View in context: https://bugs.webkit.org/attachment.cgi?id=216051&action=review

Thanks again :) My comments below. I noticed EFL still doesn't compile but I think know the problem. And I'll regenerate the test results.

>> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:42
>> +
> 
> This anonymous namespace is unnecessary.

Ok. I will remove it.

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

Ok

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

Ok

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:173
>>  
> 
> 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;

Just beautiful! I'll change it.

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

Ops

>> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:101
>>      }
> 
> 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.

Unfortunately there are corner cases where cairo_scaled_font_glyph_extents() returns x_advance with values like 3.0e-16 instead of zero, and they aren't converted into zero without casting.
To void this problem, I will use y_advance instead, which has not shown this problem.

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