[Webkit-unassigned] [Bug 46973] Need to swap glyphs for vertical writing

Mon Oct 18 11:27:38 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=46973

mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71023|review?                     |review-
               Flag|                            |

--- Comment #22 from mitz at webkit.org  2010-10-18 11:27:37 PST ---
(From update of attachment 71023)
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”.

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