[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]

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
https://bugs.webkit.org/show_bug.cgi?id=15312

Attachment 29354: Make textarea metrics more closely match IEs.
https://bugs.webkit.org/attachment.cgi?id=29354&action=review

------- 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) :
roundf(m_spaceWidth);

Can an SVG font ever be treated as fixed pitch?

+	 m_avgCharWidth =
widthForGlyph(glyphPageZero->glyphDataForCharacter(digitZeroChar).glyph);

What if the '0' glyph is missing?

+	 m_maxCharWidth =
widthForGlyph(glyphPageZero->glyphDataForCharacter(letter4E00).glyph);

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 =
widthForGlyph(glyphPageZero->glyphDataForCharacter(letterWChar).glyph);

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
period.

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

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
field?

+    // 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)
can't
+    // 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.