[webkit-reviews] review granted: [Bug 94198] fast/dom/HTMLImageElement/image-alt-text.html and fast/dom/HTMLInputElement/input-image-alt-text.html are failing : [Attachment 159005] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Hajime Morrita <morrita at google.com>'s
request for review:
Bug 94198: fast/dom/HTMLImageElement/image-alt-text.html and
fast/dom/HTMLInputElement/input-image-alt-text.html are failing
https://bugs.webkit.org/show_bug.cgi?id=94198

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

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


Generally the approach looks fine.

> Source/WebCore/ChangeLog:8
> +	   Fix alt text not being displayed for img elements

This is wrapped kind of narrow for change log!

> Source/WebCore/rendering/RenderImage.cpp:121
> +	   IntSize paddedTextSize(paddingWidth +
min(ceilf(font.width(RenderBlock::constructTextRun(this, font, m_altText,
style()))), maxAltTextWidth), paddingHeight + min(font.fontMetrics().height(),
maxAltTextHeight));

I’m a little surprised by the addition of the call to ceilf here. Nothing in
the change log mentions that.

> Source/WebCore/rendering/RenderImage.cpp:301
> +	       static const int borderWidth = 1;

The static isn’t needed or helpful here.