[webkit-reviews] review denied: [Bug 46973] Need to swap glyphs for vertical writing : [Attachment 71023] Incorporated additional feedback from mitz.

Mon Oct 18 11:27:36 PDT 2010

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 71023: Incorporated additional feedback from mitz.
https://bugs.webkit.org/attachment.cgi?id=71023&action=review

------- Additional Comments from mitz at webkit.org
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”.