[webkit-reviews] review denied: [Bug 16768] Position and thickness of underline as text size changes : [Attachment 38787] get_underline_metrics_mac_impl_v1

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied  review:
Bug 16768: Position and thickness of underline as text size changes
https://bugs.webkit.org/show_bug.cgi?id=16768

Attachment 38787: get_underline_metrics_mac_impl_v1
https://bugs.webkit.org/attachment.cgi?id=38787&action=review

------- Additional Comments from mitz at webkit.org
> +#ifndef BUILDING_ON_TIGER
> +    float fUnderlinePosition = fAscent -
CTFontGetUnderlinePosition(toCTFontRef(m_platformData.font()));
> +    float fUnderlineThickness =
CTFontGetUnderlineThickness(toCTFontRef(m_platformData.font()));
> +#endif

I think you need to null-check m_platformData.font() here. But it is also
unnecessary to go through Core Text (and thus exclude Tiger), because -[NSFont
underlinePosition] and -[NSFont underlineThickness] are available in Mac OS X
v10.0 and later.

> +#ifndef BUILDING_ON_TIGER
> +    m_underlinePosition = std::max(lroundf(fUnderlinePosition), m_ascent +
1L);
> +    m_underlineThickness = std::max(lroundf(fUnderlineThickness), 1L);
> +    // Note that we use the default (hard coded) position/thickness on
Tiger.
> +    // FIXME: whould be better to update WebKitLibraries/ so that we can get
proper metrics on Tiger?
> +#endif

If you use the aforementioned NSFont methods you won’t need to make any changes
for Tiger.

Instead of using 'std::max' you should add 'using namespace std;' near the
beginning of the file and just use 'max' here. I don’t think you need the
#import <algorithm> statement in this file.

There is no need to for the "1L" literal form. You can use roundf() and
max<int>().

I am curious how you arrived at the formulas for m_underlinePosition and
m_underlineThickness. Do they match what AppKit does on Mac OS X? In particular
I don’t think AppKit derives the underline position from the font’s
-underlinePosition.