Bug 46973 – Need to swap glyphs for vertical writing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Takumi Takano]

In Japanese text, some glyphs (punctuations, parentheses...etc) should be swapped with ones that designed for vertical writing.

[Takumi Takano]

Created attachment 69449 [details]
Proposed patch file

[WebKit Review Bot]

Attachment 69449 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 69449 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4220035

[WebKit Review Bot]

Attachment 69449 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4231041

[WebKit Review Bot]

Attachment 69449 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4197029

[WebKit Review Bot]

Attachment 69449 [details] did not build on win:
Build output: http://queues.webkit.org/results/4224052

[chris fleizach]

Comment on attachment 69449 [details]
Proposed patch file

you'll need to fix build and style errors

[Alexey Proskuryakov]

Also, this definitely looks like a change that requires a regression test.

[Takumi Takano]

Created attachment 69600 [details]
[build test] fixed a style error and non-Mac platform errors

[build test] I'm checking if my changes build on non-Mac platforms.

[Takumi Takano]

Created attachment 70585 [details]
[build test] Supported font fast path. Submitted to test builds on non-Mac platforms.

Comment on attachment 70585 [details]
[build test] Supported font fast path. Submitted to test builds on non-Mac platforms.

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.

[Takumi Takano]

Thanks for the review.  I'll fix them except following one.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:81
....
> Instead of making this change, you should change FontPlatformData::ctFont() to return a font with kCTFontOrientationAttribute set to kCTFontVerticalOrientation.
> 

Apparently CTFont's CTFontGetGlyphsForCharacters() doesn't retrieve vertical variant glyphs even the font has kCTFontVerticalOrientation attribute, probably because glyph swapping for vertical layout is handled in CoreText's line layout engine, not at CTFont's level. I checked the source of CTFontGetGlyphsForCharacters() and confirmed it just directly consults font's cmap.

Can I leave this part as is?

[Takumi Takano]

...and a CTLine created from CTFont+kCTFontOrientationAttribute but without kCTVerticalFormsAttributeName doesn't do glyph swapping also. As far as I tested, we need to explicitly specify kCTVerticalFormsAttributeName in a CTLine to make glyph swapping to be happened.

[Takumi Takano]

Created attachment 70827 [details]
[another build test] Incorporated mitz's feedback. Submitted to test builds on non-Mac platforms.

Incorporated mitz's feedback except the one for SimpleFontDataCoreText.cpp.  Apparently making CTFont with kCTFontOrientationAttribute=kCTFontVerticalOrientation doesn't help to simplify the code. Please see my comments above...

[Early Warning System Bot]

Attachment 70827 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4391038

[Takumi Takano]

Created attachment 70830 [details]
[another build test] Fixed build error on non-Mac platforms.

[Takumi Takano]

Created attachment 70832 [details]
[another build test] Fixed build error on non-Mac platforms. Forgot to add review flag...

Forgot to add review flag...

[Takumi Takano]

Created attachment 70837 [details]
[another build test] Fixed another build error on non-Mac platforms...

Comment on attachment 70837 [details]
[another build test] Fixed another build error on non-Mac platforms...

View in context: https://bugs.webkit.org/attachment.cgi?id=70837&action=review

> WebCore/ChangeLog:11
> +        Doesn't affect any tests yet.

Can you add tests that are affected by this? If not, why not?

> WebCore/ChangeLog:104
> +

Please add function-level comments about the changes.

> WebCore/loader/CachedFont.cpp:120
> +FontPlatformData CachedFont::platformDataFromCustomData(float size, bool bold, bool italic, FontOrientation fontOrientation, FontRenderingMode renderingMode)

Please rename the parameter to “orientation”. There’s no need for “font” in this context.

> WebCore/loader/CachedFont.h:30
> +#include "FontDescription.h"

Please define FontOrientation in its own header so that you don’t have to include FontDescription in this header.

> WebCore/loader/CachedFont.h:66
> +    FontPlatformData platformDataFromCustomData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Please drop the parameter name “fontOrientation” altogether. WebKit style is not to name parameters in declarations when there is no ambiguity.

> WebCore/platform/graphics/FontCache.cpp:58
> +                             bool isPrinterFont = false, FontRenderingMode renderingMode = NormalRenderingMode, FontOrientation fontOrientation = Horizontal)

Again, please drop “font” from the parameter name.

> WebCore/platform/graphics/FontCache.cpp:85
> +    FontOrientation m_fontOrientation;

And rename this to m_orientation.

> WebCore/platform/graphics/FontDescription.h:100
> +    FontOrientation fontOrientation() const { return m_fontOrientation; }

Rename this accessor to orientation().

> WebCore/platform/graphics/FontDescription.h:120
> +    void setFontOrientation(FontOrientation fontOrientation) { m_fontOrientation = fontOrientation; }

And this one to setOrientation().

> WebCore/platform/graphics/FontDescription.h:129
> +    FontOrientation m_fontOrientation; // Font's orientation. Horizontal or vertical.

I don’t think the comment is necessary.

> WebCore/platform/graphics/cairo/FontCustomPlatformData.h:25
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/cairo/FontCustomPlatformData.h:42
> +    FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Don’t name the parameter.

> WebCore/platform/graphics/cairo/FontPlatformDataFreeType.h:58
> +    FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);

Why is this change needed? You should omit the parameter name, if this is needed.

> WebCore/platform/graphics/cg/FontPlatformData.h:27
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/cg/FontPlatformData.h:53
> +    FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);

Why is this change needed? If this is needed, you should omit the parameter name.

> WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:37
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:61
> +    FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);

Why is this change needed? If this is needed, you should omit the parameter name.

> WebCore/platform/graphics/chromium/FontPlatformDataLinux.h:34
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/chromium/FontPlatformDataLinux.h:77
> +    FontPlatformData(float textSize, bool fakeBold, bool fakeItalic, FontOrientation fontOrientation = Horizontal)

Why is this change needed? If this is needed, you should omit the parameter name.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:27
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:62
> +    FontPlatformData(float size, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation = Horizontal)

I’m not sure it’s a good idea to have a default value for the orientation.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:76
> +    FontPlatformData(NSFont *nsFont, bool syntheticBold = false, bool syntheticOblique = false, FontOrientation fontOrientation = Horizontal);

Please omit the parameter name.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:78
> +    FontPlatformData(CGFontRef cgFont, ATSUFontID fontID, float size, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation = Horizontal)

And rename it to “orientation” here.

> WebCore/platform/graphics/cocoa/FontPlatformData.h:100
> +    FontOrientation fontOrientation() const { return m_fontOrientation; }

Please rename this accessor to orientation().

> WebCore/platform/graphics/cocoa/FontPlatformData.h:104
> +    FontOrientation m_fontOrientation;

And this member variable to m_orientation.

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:47
> +FontPlatformData::FontPlatformData(NSFont *nsFont, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation)

Please rename the parameter to “orientation”.

> WebCore/platform/graphics/gtk/FontPlatformDataPango.h:58
> +    FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);

Why is this change needed? If this is needed, you should omit the parameter name.

> WebCore/platform/graphics/haiku/FontCustomPlatformData.h:24
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/haiku/FontCustomPlatformData.h:41
> +        FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Omit the parameter name.

> WebCore/platform/graphics/haiku/FontPlatformData.h:48
> +        FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);

Why is this change needed? If this is needed, you should omit the parameter name.

> WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:41
> +FontPlatformData FontCustomPlatformData::fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation, FontRenderingMode)

Rename the parameter to “orientation”.

> WebCore/platform/graphics/mac/FontCustomPlatformData.h:24
> +#include "FontDescription.h"

Use a separate header for the FontOrientation enum.

> WebCore/platform/graphics/mac/FontCustomPlatformData.h:45
> +    FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Omit the parameter name.

> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:77
> +            RetainPtr<CGFontRef> runCGFont(AdoptCF, CTFontCopyGraphicsFont(runFont, 0));
> +            if (CFEqual(fontData->platformData().cgFont(), runCGFont.get())) {

Is there a reason to compare CGFonts and not CTFonts here?

> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:83
> +                Vector<CGGlyph, 512> glyphs(static_cast<size_t>(glyphCount));
> +                CTRunGetGlyphs(ctRun, CFRangeMake(0, 0), glyphs.data());
> +                Vector<CFIndex, 512> stringIndices(static_cast<size_t>(glyphCount));
> +                CTRunGetStringIndices(ctRun, CFRangeMake(0, 0), stringIndices.data());

This forces copying, but CTRunGetGlyphsPtr() and CTRunGetStringIndicesPtr() don’t. Of course, those aren’t guaranteed to work, so you need try them first, then use the above as a fallback if they fail.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:46
> +    unsigned key = (typesettingFeatures + 1) | (platformData().fontOrientation() << 8);

There’s no need to include the orientation in the key, because it’s constant for a given SimpleFontData instance.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:52
> +    bool allowLigatures = (!ignoreDefaultFeatures && platformData().allowsLigatures()) || (typesettingFeatures & Ligatures);

You can just change platformData().allowsLigatures() to return false for the vertical orientation, and avoid adding the ignoreDefaultFeatures parameter to this method.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:70
> +        static const void* keysWithKerningDisabled[] = {    kCTFontAttributeName,
> +                                                            kCTKernAttributeName,
> +                                                            kCTLigatureAttributeName,
> +                                                            kCTVerticalFormsAttributeName };
> +        const void* valuesWithKerningDisabled[] = {         platformData().ctFont(),
> +                                                            kerningAdjustment,
> +                                                            allowLigatures ? ligaturesAllowed : ligaturesNotAllowed,
> +                                                            (platformData().fontOrientation() == Vertical) ? kCFBooleanTrue : kCFBooleanFalse };

WebKit style is to not align things like this. Please just add the key and the value at the end.

> WebCore/platform/graphics/mac/SimpleFontDataMac.mm:421
> +    boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(),
> +                    m_platformData.fontOrientation() == Vertical ? kCTFontVerticalOrientation : kCTFontHorizontalOrientation, &glyph, NULL, 1);

WebKit style is to use 0, not NULL.

> WebCore/platform/graphics/qt/FontCustomPlatformData.h:25
> +#include "FontDescription.h"

Use a separate header for FontOrientation.

> WebCore/platform/graphics/qt/FontCustomPlatformData.h:41
> +    FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Omit the parameter name.

> WebCore/platform/graphics/qt/FontPlatformData.h:66
> +    FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);

I don’t think this change is needed. If it is, omit the parameter name.

> WebCore/platform/graphics/skia/FontCustomPlatformData.h:35
> +#include "FontDescription.h"

Use a separate header for FontOrientation.

> WebCore/platform/graphics/skia/FontCustomPlatformData.h:66
> +    FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal,

Omit the parameter name.

> WebCore/platform/graphics/win/FontCustomPlatformData.h:24
> +#include "FontDescription.h"

Use separate header.

> WebCore/platform/graphics/win/FontCustomPlatformData.h:46
> +    FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Omit parameter name.

> WebCore/platform/graphics/win/FontCustomPlatformDataCairo.h:43
> +    FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);

Don’t this this is needed. Omit parameter name.

> WebCore/platform/graphics/win/FontPlatformDataCairoWin.h:63
> +    FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);

Ditto.

> WebCore/platform/graphics/wince/FontPlatformData.h:45
> +        FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);

Ditto.

> WebCore/platform/graphics/wx/FontCustomPlatformData.h:24
> +#include "FontDescription.h"

Use different header.

> WebCore/platform/graphics/wx/FontCustomPlatformData.h:41
> +        FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);

Omit parameter name.

> WebCore/platform/graphics/wx/FontPlatformData.h:94
> +    FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal)

Don’t think you need to change this. If you do, omit the parameter name.

[Takumi Takano]

Created attachment 71023 [details]
Incorporated additional feedback from mitz.

[Takumi Takano]

Thank you very much for your feedback again. I've fixed them except following two.

> > WebCore/platform/graphics/cocoa/FontPlatformData.h:62
> > +    FontPlatformData(float size, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation = Horizontal)
> 
> I’m not sure it’s a good idea to have a default value for the orientation.
> 

Otherwise CachedFont.cpp:124, FontCache.cpp:241, SimpleFontData.cpp:67 cause compile errors on FontPlatformData creation as now we don't add "orientation" to non-Mac platform. Or, do you think it is better to add another constructor that takes only a size, a bold flag, and an oblique flag?


> > WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:77
> > +            RetainPtr<CGFontRef> runCGFont(AdoptCF, CTFontCopyGraphicsFont(runFont, 0));
> > +            if (CFEqual(fontData->platformData().cgFont(), runCGFont.get())) {
> 
> Is there a reason to compare CGFonts and not CTFonts here?
> 

CFEqual for CTFont considers font's CTFontDescriptor that contains various attributes on top of the font data itself. I wanted to avoid possible small discrepancies CT's layout engine might added.

Comment on attachment 71023 [details]
Incorporated additional feedback from mitz.

View in context: https://bugs.webkit.org/attachment.cgi?id=71023&action=review

The patch looks good! My comments are all pretty minor.

> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:175
> +    return (m_orientation == Horizontal) && (![[m_font coveredCharacterSet] characterIsMember:'a']);

Please remove the extraneous parentheses.

> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:68
> +        static Vector<CGGlyph, 512> *glyphVector = 0;
> +        static Vector<CFIndex, 512> *indexVector = 0;

WebKit style is to put the space on the other side of the star. However, I suggest allocating these on the stack unconditionally instead of using pointers and heap allocation.

> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:70
> +        for (CFIndex r = 0; r < runCount && !done ; r++) {

WebKit style is to write “++r” in for loops. It doesn’t make a difference for a type like CFIndex, but it’s better for some iterator types, so we do this for consistency.

> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:79
> +            if (CFEqual(fontData->platformData().cgFont(), runCGFont.get())) { // Use CGFont here as CFEqual for CTFont counts all attributes for font

Please put the comment on its own line and terminate it with a period!

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:65
> +            ? ligaturesAllowed : ligaturesNotAllowed, (platformData().orientation() == Vertical) ? kCFBooleanTrue : kCFBooleanFalse };

Parentheses unneeded here.

> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:72
> +        const void* valuesWithKerningEnabled[] = { platformData().ctFont(), allowLigatures ? ligaturesAllowed : ligaturesNotAllowed, (platformData().orientation() == Vertical) ? kCFBooleanTrue : kCFBooleanFalse };

And here.

> LayoutTests/fast/text/international/vertical-text-metrics-test.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" 

Is there any reason to use this legacy doctype for this test? If the test works with the HTML5 doctype or without a doctype, please change.

> LayoutTests/fast/text/international/vertical-text-metrics-test.html:17
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();

It’s good to have this dumps-as-text test for the metrics. Can you also make a pixel test for the glyph variants?

> LayoutTests/fast/text/international/vertical-text-metrics-test.html:23
> +		var elem = pElems[i];
> +		print("width=" + elem.offsetWidth);

Tabs here will prevent this patch from being landed.

> LayoutTests/fast/text/international/vertical-text-metrics-test.html:49
> +<span id="horizontal_TB">abãããã</span><br>
> +<span id="horizontal_BT">abãããã</span><br>
> +<span id="vertical_RL">abãããã</span><br>
> +<span id="vertical_LR">abãããã</span><br>

Is this testing only the simple text code path? It would be good to test the complex text code path as well. One way to force things down that path is to use “text-rendering: optimizelegibility”.

[Takumi Takano]

Created attachment 71131 [details]
Made some more changes pointed by mitz

Added some more changes pointed by mitz.
One exception is for the comment below;
> > LayoutTests/fast/text/international/vertical-text-metrics-test.html:17
> > +    if (window.layoutTestController)
> > +        layoutTestController.dumpAsText();
> 
> It’s good to have this dumps-as-text test for the metrics. Can you also make a pixel test for the glyph variants?

We cannot get correct pixel images at this time, because laying out the text box for vertical writing mode (e.g., flowing text vertically and stacking text boxes tb-rl) hasn't been finished yet...

(In reply to comment #23)
> Created an attachment (id=71131) [details]
> Made some more changes pointed by mitz
> 
> Added some more changes pointed by mitz.
> One exception is for the comment below;
> > > LayoutTests/fast/text/international/vertical-text-metrics-test.html:17
> > > +    if (window.layoutTestController)
> > > +        layoutTestController.dumpAsText();
> > 
> > It’s good to have this dumps-as-text test for the metrics. Can you also make a pixel test for the glyph variants?
> 
> We cannot get correct pixel images at this time, because laying out the text box for vertical writing mode (e.g., flowing text vertically and stacking text boxes tb-rl) hasn't been finished yet…

The images need not be “correct”, they are useful if they can help to detect unintended changes in behavior, which could be regressions. So I think a pixel test would be quite valuable.

[WebKit Commit Bot]

Comment on attachment 71131 [details]
Made some more changes pointed by mitz

Rejecting patch 71131 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
, Tests=304,  1 wallclock secs ( 0.63 cusr +  0.13 csys =  0.76 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21599 test cases.
fast/text/international/vertical-text-metrics-test.html -> failed

Exiting early after 1 failures. 15784 tests run.
212.08s total testing time

15783 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4578001

[Takumi Takano]

Created attachment 71269 [details]
Added pixel test. Changed tests to explicitly specify a font family.

[Early Warning System Bot]

Attachment 71269 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4509007

[Takumi Takano]

Created attachment 71270 [details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)

[WebKit Review Bot]

Attachment 71269 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4564012

[Eric Seidel (no email)]

Comment on attachment 71131 [details]
Made some more changes pointed by mitz

Cleared Dan Bernstein's review+ from obsolete attachment 71131 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 71270 [details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)

Rejecting patch 71270 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71270]" exit_code: 2
Cleaning working directory
Updating working directory
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/4520022

[Eric Seidel (no email)]

Comment on attachment 71270 [details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)

I think this was a false rejection from a run-away commit-node.

[WebKit Commit Bot]

Comment on attachment 71270 [details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)

Rejecting patch 71270 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
l.
Files=14, Tests=304,  1 wallclock secs ( 0.74 cusr +  0.18 csys =  0.92 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21605 test cases.
fast/text/international/vertical-text-glyph-test.html -> failed

Exiting early after 1 failures. 15787 tests run.
239.82s total testing time

15786 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4613003

Created attachment 71365 [details]
Same as attachment 71270 [details], but with results updated after r70172

[WebKit Commit Bot]

Comment on attachment 71365 [details]
Same as attachment 71270 [details], but with results updated after r70172

Clearing flags on attachment: 71365

Committed r70225: <http://trac.webkit.org/changeset/70225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Committed r70250.

[James Robinson]

r70250 changed some #if guards from USE(CORE_TEXT) to PLATFORM(MAC):

http://trac.webkit.org/changeset/70225/trunk/WebCore/platform/graphics/SimpleFontData.h

however PLATFORM(MAC) is only set for the Apple Mac port and so this broke the Chromium mac compile.  Was there a specific reason to move away from the USE(CORE_TEXT) macro?  Could these be changed from PLATFORM(MAC) to USE(CG) or something else that's also true for Chromium mac?  thanks.

[James Robinson]

(In reply to comment #38)
> r70250 changed some #if guards from USE(CORE_TEXT) to PLATFORM(MAC):
> 
> http://trac.webkit.org/changeset/70225/trunk/WebCore/platform/graphics/SimpleFontData.h
> 
> however PLATFORM(MAC) is only set for the Apple Mac port and so this broke the Chromium mac compile.  Was there a specific reason to move away from the USE(CORE_TEXT) macro?  Could these be changed from PLATFORM(MAC) to USE(CG) or something else that's also true for Chromium mac?  thanks.

Chromium mac build fix landed in http://trac.webkit.org/changeset/70272.