[webkit-reviews] review granted: [Bug 94822] OPENTYPE_VERTICAL support for Chromium Win : [Attachment 160187] OPENTYPE_VERTICAL support for Chromium Win

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Koji Ishii <kojiishi at gmail.com>'s
request for review:
Bug 94822: OPENTYPE_VERTICAL support for Chromium Win
https://bugs.webkit.org/show_bug.cgi?id=94822

Attachment 160187: OPENTYPE_VERTICAL support for Chromium Win
https://bugs.webkit.org/attachment.cgi?id=160187&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160187&action=review


cq- for the changelog.	Otherwise seems fine.

At some point, we may want to add a perf test to the PerformanceTests
directory.  I don't think the page cyclers actually have any pages with
vertical text.

> Source/WebCore/ChangeLog:3
> +	   [chromium] OPENTYPE_VERTICAL support for Chromium Win

I would remove [chromium] from the bug title since this patch modifies cross
platform code.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:429
> +#if ENABLE(OPENTYPE_VERTICAL)
> +	   if (verticalData) {

Nit: Checking verticalData for each glyph sounds slow. We can try it, but I bet
it would be faster to have drawGlyphs check verticalData at the beginning of
the function and call drawVerticalGlyphs that has all of the vertical code.