[webkit-reviews] review denied: [Bug 15312] WebKit applies an incorrect min width for <input type="text"> fields : [Attachment 29354] Make textarea metrics more closely match IEs.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 10 08:44:14 PDT 2009

mitz at webkit.org has denied Ojan Vafai <ojan at chromium.org>'s request for review:
Bug 15312: WebKit applies an incorrect min width for <input type="text"> fields

Attachment 29354: Make textarea metrics more closely match IEs.

------- Additional Comments from mitz at webkit.org
+	 (WebCore::SimpleFontData::initCharWidthsFromZeroAndW):

This is in the change log but no longer in the patch.

+	 m_adjustedSpaceWidth = m_treatAsFixedPitch ? ceilf(m_spaceWidth) :

Can an SVG font ever be treated as fixed pitch?

+	 m_avgCharWidth =

What if the '0' glyph is missing?

+	 m_maxCharWidth =

This is wrong. Code point U+4E00 is not in glyph page zero. It is in glyph page
0x4e00 / GlyphPage::size. It also doesn't check for the missing glyph case. I
think you should come up with a way to get the width of the U+4E00, it if
exists, without instantiating a glyph page.

+	     m_maxCharWidth =

Not checking for the missing glyph.

+const uint32 kOS2CompatibilityTable = 'OS/2';
+const int kOS2CompatibilityTableMinValidLength = 68;
+const int kOS2xAvgCharWidthOffset = 2;

We don't use the 'k' prefix for constants in WebCore. I think a better name for
the first constant would be OS2CompatibilityTableTag. Since these are only used
in platformCharWidthInit(), why not move them into that function?

+	 // retrieve the (big-endian) average character width for this font

Sentence-like comments should have sentence capitalization and end with a

+	 SInt16 iAvgWidth = buffer[kOS2xAvgCharWidthOffset] * 256 +
buffer[kOS2xAvgCharWidthOffset + 1];
+	 m_avgCharWidth = scaleEmToUnits(iAvgWidth, m_unitsPerEm) *

Given that iAvgWidth may be invalid, I think you should not leave it up to the
cross-platform function to discover it. You should check the validity here.
Then you can set m_avgCharWidth only if iAvgWidth is valid. Have you looked
into making the validity check more robust by examining the OS/2 table version

+    // Compute the maximum width of a character in this font.
+    m_maxCharWidth = 0.f;
+    int numGlyphsInFont = CGFontGetNumberOfGlyphs(m_font.cgFont());
+    for (Glyph glyphNumber = 0; glyphNumber < numGlyphsInFont; glyphNumber++)
+	 m_maxCharWidth = std::max(m_maxCharWidth, widthForGlyph(glyphNumber));

This looks very very expensive. In addition to taking a considerable amount of
time, it will allocate many GlyphWidthPage instances in the glyph width map.

+    // if there was no OS/2 table or it was malformed, we (by definition)
+    // match Windows metrics, so we fall back to the previous WebKit behavior
of using
+    // the width of the digit '0' as a reasonable estimate.

This comment is no long accurate, because you changed the behavior to consider
glyphs other than '0'. It should also be written in sentence style.

More information about the webkit-reviews mailing list