[webkit-reviews] review denied: [Bug 46973] Need to swap glyphs for vertical writing : [Attachment 70585] [build test] Supported font fast path. Submitted to test builds on non-Mac platforms.

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Takumi Takano <takano1 at asia.apple.com>'s request for
review:
Bug 46973: Need to swap glyphs for vertical writing
https://bugs.webkit.org/show_bug.cgi?id=46973

Attachment 70585: [build test] Supported font fast path. Submitted to test
builds on non-Mac platforms.
https://bugs.webkit.org/attachment.cgi?id=70585&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=70585&action=review

> WebCore/platform/graphics/FontCache.cpp:85
> +    bool m_isVerticalLayout;

m_isVerticalLayout is a bad name for this instance variable and for the
predicates. Please add an enum { Horizontal, Vertical } FontOrientation and use
it.

> WebCore/platform/graphics/TypesettingFeatures.h:33
> +	   VerticalLayout = 1 << 2,

This is a property of the font, so it doesn’t need to be a typesetting feature.


> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:63
> +	   static const int ligaturesNotAllowedValue = 0;
> +	   static CFNumberRef ligaturesNotAllowed =
CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType,
&ligaturesNotAllowedValue);
> +	   static const void* keys[] = {   kCTFontAttributeName,	      
kCTLigatureAttributeName,   kCTVerticalFormsAttributeName };
> +	   const void* values[] = {	   fontData->platformData().ctFont(), 
ligaturesNotAllowed,	    kCFBooleanTrue };
> +	   RetainPtr<CFDictionaryRef> attributeDictionary(AdoptCF,
CFDictionaryCreate(kCFAllocatorDefault, keys, values, sizeof(keys) /
sizeof(*keys),
> +								  
&kCFCopyStringDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +
> +	   RetainPtr<CFStringRef> fontName(AdoptCF,
CTFontCopyPostScriptName(fontData->platformData().ctFont()));

This should just use SimpleFontData::getCFStringAttributes().

> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:85
> +	       RetainPtr<CFStringRef> runFontName(AdoptCF,
CTFontCopyPostScriptName(runFont));
> +	       if (CFEqual(fontName.get(), runFontName.get())) {

This is a strange way to compare fonts.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:81
> -	   static const void* keysWithKerningDisabled[] = {
kCTFontAttributeName, kCTKernAttributeName, kCTLigatureAttributeName };
> -	   const void* valuesWithKerningDisabled[] = { platformData().ctFont(),
kerningAdjustment, allowLigatures
> -	       ? ligaturesAllowed : ligaturesNotAllowed };
> +	   static const void* keysWithKerningDisabled[] = {   
kCTFontAttributeName,
> +							      
kCTKernAttributeName,
> +							      
kCTLigatureAttributeName,
> +							      
kCTVerticalFormsAttributeName };
> +	   const void* valuesWithKerningDisabled[] = {	      
platformData().ctFont(),
> +							      
kerningAdjustment,
> +							       allowLigatures ?
ligaturesAllowed : ligaturesNotAllowed,
> +							      
(typesettingFeatures & VerticalLayout) ? kCFBooleanTrue : kCFBooleanFalse };
>	   attributesDictionary.adoptCF(CFDictionaryCreate(0,
keysWithKerningDisabled, valuesWithKerningDisabled,
>	       sizeof(keysWithKerningDisabled) /
sizeof(*keysWithKerningDisabled),
>	       &kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
>      } else {
>	   // By omitting the kCTKernAttributeName attribute, we get Core
Text's standard kerning.
> -	   static const void* keysWithKerningEnabled[] = {
kCTFontAttributeName, kCTLigatureAttributeName };
> -	   const void* valuesWithKerningEnabled[] = { platformData().ctFont(),
allowLigatures ? ligaturesAllowed : ligaturesNotAllowed };
> +	   static const void* keysWithKerningEnabled[] = {    
kCTFontAttributeName,
> +							      
kCTLigatureAttributeName,
> +							      
kCTVerticalFormsAttributeName };
> +	   const void* valuesWithKerningEnabled[] = {	      
platformData().ctFont(),
> +							       allowLigatures ?
ligaturesAllowed : ligaturesNotAllowed,
> +							      
(typesettingFeatures & VerticalLayout) ? kCFBooleanTrue : kCFBooleanFalse };

Instead of making this change, you should change FontPlatformData::ctFont() to
return a font with kCTFontOrientationAttribute set to
kCTFontVerticalOrientation.

> WebCore/platform/graphics/mac/SimpleFontDataMac.mm:440
>  {
> -    NSFont* font = platformData().font();
> -    float pointSize = platformData().m_size;
> -    CGAffineTransform m = CGAffineTransformMakeScale(pointSize, pointSize);
>      CGSize advance;
> -    if (!wkGetGlyphTransformedAdvances(platformData().cgFont(), font, &m,
&glyph, &advance)) {
> -	   LOG_ERROR("Unable to cache glyph widths for %@ %f", [font
displayName], pointSize);
> -	   advance.width = 0;
> -    }
> +    CTFontGetAdvancesForGlyphs(m_platformData.ctFont(),
> +				   m_platformData.isVerticalLayout() ?
kCTFontVerticalOrientation : kCTFontHorizontalOrientation, &glyph, &advance,
1);

For horizontal orientation, this should keep using
wkGetGlyphTransformedAdvances, which takes the font’s rendering mode into
account.

> WebCore/rendering/style/RenderStyle.cpp:720
> +void RenderStyle::setWritingMode(WritingMode v)
> +{
> +    inherited_flags.m_writingMode = v;
> +
> +    FontSelector* currentFontSelector = font().fontSelector();
> +    FontDescription desc(fontDescription());
> +    desc.setIsVerticalLayout(!isHorizontalWritingMode());
> +    setFontDescription(desc);
> +    font().update(currentFontSelector);
> +}
> +

This should be done in CSSStyleSelector using the m_fontDirty bit rather than
unconditionally updating the font here.