[webkit-reviews] review granted: [Bug 15312] WebKit applies an incorrect min width for <input type="text"> fields : [Attachment 29764] WebCore side of patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 28 21:33:10 PDT 2009


mitz at webkit.org has granted 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 29764: WebCore side of patch.
https://bugs.webkit.org/attachment.cgi?id=29764&action=review

------- Additional Comments from mitz at webkit.org
> +    if (glyphPageZero) {
> +	   static int weights[] = { 64, 14, 27, 35, 100, 20, 14, 42, 63, 3, 6,
35, 20, 56, 56, 17, 4, 49, 56, 71, 31, 10, 18, 3, 18, 2, 166 };
> +	   float sum = 0.f;
> +	   int numGlyphs = 27;
> +	   ASSERT(numGlyphs == sizeof(weights)/sizeof(int));

Please add spaces around the / sign.

> +	   // Compute the weighted sum of the space character and the lowercase
letters in the Latin alphabet.
> +	   for (int i = 0; i < numGlyphs; i++) {
> +	       Glyph glyph = glyphPageZero->glyphDataForCharacter((i < 26 ? i +
'a' : ' ')).glyph;
> +	       if (glyph)
> +		   sum += widthForGlyph(glyph) * weights[i];
> +	   }
> +	   m_avgCharWidth = sum / 1000;

It may be better to add up the total weight of found glyphs and divide by that
instead of 1000. Consider a font that contains a space glyph but no Latin
(Geeza Pro is an example, if I remember correctly).

r=me , but I suggest making the above change before landing the patch.


More information about the webkit-reviews mailing list