[webkit-reviews] review granted: [Bug 111005] Underline should round to match other content. : [Attachment 190743] Patch

[bugzilla-daemon at webkit.org]

Stephen White <senorblanco at chromium.org> has granted  review:
Bug 111005: Underline should round to match other content.
https://bugs.webkit.org/show_bug.cgi?id=111005

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

------- Additional Comments from Stephen White <senorblanco at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190743&action=review


OK.  Let's go with this version.  r=me

>>>>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:754
>>>>> +    r.fTop = WebCoreFloatToSkScalar(floorf(pt.y() + 0.5f));
>>>> 
>>>> Do we ever have negative values here? LayoutUnit rounds differently around
zero:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LayoutUnit.h#L225
>>> 
>>> Skia currently always rounds X.5 up (towards +inf) when rounding text
positions. As a result, this change should always keep the line in-step with
the text. If the content rounds differently, it already rounds differently from
the text. This may require some follow-up investigation, as noted in Comment #3
about roundToDevicePixels being implemented.
>> 
>> Nit:  floating point constants don't get a trailing 'f' in WebKit code.
> 
> Indeed, this silly thing is in the style guide. Why make calculations be done
with doubles only to then convert them back to float? There is no equivalent to
CGFloat in WebKit, and everything is float, so why not force 'f' instead? I
have already gone through all stages of grief except acceptance. Do I not get a
pass since GraphicsContext::rotate(float) in this file also uses 'f'?

Sure.