[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 #43 from Denis Nomiyama (dnomi) <d.nomiyama at samsung.com>  2013-10-31 08:53:11 PST ---
(From update of attachment 213358)
View in context: https://bugs.webkit.org/attachment.cgi?id=213358&action=review

Never late, thank you very much! I will modify the patch to enable OPENTYPE_VERTICAL by default on all Freetype ports.
I've also noticed there are some characters in my comments that don't display properly by Bugzilla (e.g. matrix multiplication operator), so I will replace them.

>> Source/WebCore/platform/graphics/FontCache.cpp:241
>> +};
> 
> You probably want to describe in the ChangeLog why the new hash function is necessary.

Sure, I will do it.

>> 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.

Good point. I will do it.

>> 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.

Ops sorry.

>> 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.

The code changed quite significantly since the initial patch, in particular after the Chromium code clean up. I will re-test this function and will double check the use cases.

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98
>> +
> 
> This looks like a lot of repetition, so why not add two small helpers, heightFromTextExtentsAndOrientation and widthFromTextExtentsAndOrientation.

Ok I'll add them.

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:198
>> +    }
> 
> 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. :)

Cool nice one. I'll change it.

>> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:112
>> +    }
> 
> There's a lot of repetition here as well.

Sure, I'll simplify this.

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