[webkit-reviews] review denied: [Bug 25566] REGRESSION: text inputs with "size" are often smaller than before, but sometimes bigger (originally noticed in bugs.webkit.org text fields) : [Attachment 45929] Improve text control intrinsic widths. Linter errors fixed.

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Ojan Vafai <ojan at chromium.org>'s request for review:
Bug 25566: REGRESSION: text inputs with "size" are often smaller than before,
but sometimes bigger (originally noticed in bugs.webkit.org text fields)
https://bugs.webkit.org/show_bug.cgi?id=25566

Attachment 45929: Improve text control intrinsic widths. Linter errors fixed.
https://bugs.webkit.org/attachment.cgi?id=45929&action=review

------- Additional Comments from mitz at webkit.org
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 769fd6f..6fcc124 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog

The entire history and rationale for the change don’t really belong in the
change log entry. The references to the bugs should suffice.

>      m_avgCharWidth = 0.f;
> +    m_maxCharWidth = 0.f;

The preferred style is "0", without the ".f".

> +    CFDataRef os2Table = CGFontCopyTableForTag(m_platformData.cgFont(),
'OS/2');
> +    if (os2Table) {

Since it’s only used in the scope of the if statement, you can define os2Table
in the if statement itself:
       if (CFDataRef os2Table = CGFontCopyTableForTagm_platformData.cgFont(),
'OS/2'))

You are also failing release the CFDataRef. You should call CFRelease(os2Table)
at the end of the block. Alternatively, you can use a RetainPtr<CFDataRef> like
this:
       RetainPtr<CFDataRef> os2Table(AdoptCF, CGFontCopyTableFromTag(…

> +	   const UInt8* os2 = CFDataGetBytePtr(os2Table);
> +	   SInt16 os2AvgCharWidth = os2[2]*256 + os2[3];

You must check that os2Table is at least 4 bytes long before doing this!

> +    CFDataRef headTable = CGFontCopyTableForTag(m_platformData.cgFont(),
'head');
> +    if (headTable) {
> +	   const UInt8* head = CFDataGetBytePtr(headTable);
> +	   ushort uxMin = head[36]*256 + head[37];
> +	   ushort uxMax = head[40]*256 + head[41];
> +	   SInt16 xMin = static_cast<SInt16>(uxMin);
> +	   SInt16 xMax = static_cast<SInt16>(uxMax);
> +	   float diff = static_cast<float>(xMax - xMin);
> +	   m_maxCharWidth = scaleEmToUnits(diff, m_unitsPerEm) *
m_platformData.m_size;
> +    }

Same comments apply here.

> +    static HashMap<AtomicString, bool>* fontFamiliesWithInvalidCharWidthMap
= 0;
> +    
> +    if (!fontFamiliesWithInvalidCharWidthMap) {
> +	   fontFamiliesWithInvalidCharWidthMap = new HashMap<AtomicString,
bool>;
> +	   
> +	   for (unsigned i = 0; i < sizeof(fontFamiliesWithInvalidCharWidth) /
sizeof(fontFamiliesWithInvalidCharWidth[0]); i++)
> +	      
fontFamiliesWithInvalidCharWidthMap->set(AtomicString(fontFamiliesWithInvalidCh
arWidth[i]), true);
> +    }
> +
> +    return !fontFamiliesWithInvalidCharWidthMap->get(family);

It looks like you’re using a HashMap where everything maps to 'true'. You can
use a HashSet!

r- based on the above issues, but ultimately I think Hyatt should weigh in on
questions of substance.