[Webkit-unassigned] [Bug 41535] "vertical-align: middle; " not working on a MathML element

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41535


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66662|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2010-10-13 17:38:18 PST ---
(From update of attachment 66662)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.