[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 #28 from Denis Nomiyama (dnomi) <d.nomiyama at samsung.com>  2013-07-17 10:19:08 PST ---
(In reply to comment #25)

Many thanks for reviewing the patch. I have modified it according to the comments.
Initially I added a configuration argument just to be on the safe side since this is my first time adding a feature guarded by ENABLE_*. I also agree this feature can be unconditional mainly because it is only trigged when the content has vertical text. This type of scenario is currently always incorrect, and horizontal texts should not be affected by this change.
I have removed all changes done on unrelated code to fix coding style problems.

I just found a problem when handling the glyph extents of vertical ghyphs. In particular, y_advance is negative and CairoGetGlyphWidthAndExtents() in the complex path had to be modified as it uses Cairo scaled fonts from SimpleFontData.


> > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:47
> > +#if ENABLE(OPENTYPE_VERTICAL)
> > +static FontCache::FontFileKey lastFontID = 0;
> > +#endif
> > +
> 
> It seems that FontFileKey is an AtomicString on other platforms?

I had similar concerns about this. The idea of defining FontFileKey as integer is based on Chromium, where the ID was provided by Skia. On the plus side, it should be more efficient than a string. I have also tried to find out what other ports do, but I could not find any other code that calls FontCache::getVerticalData(). Strange.

> > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:390
> > +    error = FT_Load_Sfnt_Table(fontConfigFace, tag, 0, (FT_Byte*)buffer->data(), &tableSize);
> 
> Please use C++ casts. Why is a cast necessary here?

It is a sign/const convertion issue.
error: invalid conversion from 'const char*' to 'FT_Byte* {aka unsigned char*}'
I have modified the code to use C++ casts. My bad..

> > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:399
> > +void FontPlatformData::setOrientation(FontOrientation orientation)
> 
> Can you walk me through how this is called?

In my investigations this function is only called to change the orientation from vertical to horizontal from SimpleFontData::verticalRightOrientationFontData(), which is called by glyphDataAndPageForNonCJKCharacterWithGlyphOrientation() when non-CJK characters need to be displayed among other CJK characters. It can happen when the vertical text is composed by CJK characters with some non-CJK characters as well.

> > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:115
> > +            const FT_Tag vorgTag = FT_MAKE_TAG('V', 'O', 'R', 'G');
> > +            error = FT_Load_Sfnt_Table(fontConfigFace, vorgTag, 0, 0, &vorgSize);
> > +            if ((!error) && (vorgSize > 0))
> > +                m_hasVerticalGlyphs = true;
> > +        }
> 
> I think you could move this entire code segment into a couple helper functions.
> 
> bool tableLengthIsNonZero(FT_Face face, FT_TAG table)
> {
>     FT_ULong size;
>     return FT_Load_Sfnt_Table(face, table, 0, 0, &size) && size > 0;
> }
> 
> bool hasVerticalGlyphs(FT_Face freeTypeFace)
> {
>     return tableLengthIsNonZero(freeTypeFace, FT_MAKE_TAG('v', 'h', 'e', 'a')) || tableLengthIsNonZero(freeTypeFace, FT_MAKE_TAG('V', 'O', 'R', G'));
> }
> 
> On a more general note, you are looking for these tables, but you are not reading data from them. Why?

Ops, great catch. m_hasVerticalGlyphs was initialized in two places, first at SimpleFontData::platformInit() and later on at SimpleFontData::SimpleFontData(). I have removed the first one, which I introduced.

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