[webkit-reviews] review denied: [Bug 41535] "vertical-align: middle; " not working on a MathML element : [Attachment 66662] Revised patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied François Sausset <sausset at gmail.com>'s
request for review:
Bug 41535: "vertical-align: middle;" not working on a MathML element
https://bugs.webkit.org/show_bug.cgi?id=41535

Attachment 66662: Revised patch
https://bugs.webkit.org/attachment.cgi?id=66662&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=66662&action=review

Seems OK but needs at least a comment and perhaps some tweaks to the code.

> WebCore/platform/graphics/mac/SimpleFontDataMac.mm:281
> +	   if (!static_cast<int>(m_xHeight) && m_ascent)
> +	       m_xHeight = static_cast<float>(2 * m_ascent / 3);

This is a bit confusing. The code needs a comment explaining what it is doing.

The change log explains that this works around a particular broken font, so the
comment needs to at least say that. That’s the reason why the code exists at
all.

The technique of casting m_xHeight to int before checking it against zero is
unusual and needs to be explained as well. Why is that a good way to check?

What’s the point of checking m_ascent against zero? Is there a reason we need
the computed m_xHeight to be an integer? If so, why?

The comment should address the three why questions.