[webkit-reviews] review denied: [Bug 51450] Glyphs in vertical text tests are rotated 90 degrees clockwise on Chromium Windows : [Attachment 77598] An updated patch for 51450

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Chun-Lung Huang
<alvincl.huang at gmail.com>'s request for review:
Bug 51450: Glyphs in vertical text tests are rotated 90 degrees clockwise on
Chromium Windows
https://bugs.webkit.org/show_bug.cgi?id=51450

Attachment 77598: An updated patch for 51450
https://bugs.webkit.org/attachment.cgi?id=77598&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77598&action=review

Thank you for making the patch.
Would you attach some layout test result images please?

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:573
> +FontPlatformData* FontCache::createFontPlatformData(const FontDescription&
fontDescription, const AtomicString& oldFamily)

I don't think "oldFamily" is an appropriate name. 
How about keeping it as "family", and ...

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:583
> +    AtomicString family;

.. renaming this to "updatedFamilyName" and replacing the following "family"
with "updatedFamilyName"?

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:586
> +    // Create FontPlatformData should consider the orientation.
> +    // For vertical: add "@" in front of the oldFamily name. (Windows)

This comment doesn't add any additional information.  Please remove it.

> WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:73
> -    FontOrientation orientation() const { return Horizontal; } // FIXME:
Implement.
> +    FontOrientation orientation() const { return m_orientation; } // FIXME:
Implement.

Please remove the FIXME comment.

> WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:83
> +    // Create FontCustomPlatformData should consider the orientation.
> +    // For vertical: add "@" in front of the m_name. (Windows)

This comment doesn't add any additional information.  Please remove it.