[webkit-reviews] review granted: [Bug 99442] Regression r130057: Improper preferred width calculation when an inline replaced object, wrapped in an inline flow, follows some text. : [Attachment 171600] Patch

[bugzilla-daemon at webkit.org]

Levi Weintraub <leviw at chromium.org> has granted Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 99442: Regression r130057: Improper preferred width calculation when an
inline replaced object, wrapped in an inline flow, follows some text.
https://bugs.webkit.org/show_bug.cgi?id=99442

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

------- Additional Comments from Levi Weintraub <leviw at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171600&action=review


I'm happy to CQ+ this with a comment showing the math to get this 94px. Better
still would be determining the correct width programmatically, as this is less
prone to failure and "shows the math."

> Source/WebCore/rendering/RenderBlock.cpp:5795
> +    bool shouldBreakLineAfterText = false;

This is a much better name, thank you.

>
LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:19
> +    description("Improper preferred logical width is computed for blocks
with an inline replaced object following some text. The end width from the text
object is carried forward to the next line containing the replaced object,
thereby increasing its width.");

Nit: super long line.

>
LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:23
> +    shouldBe("getWidth('div1')", "'94px'");
> +    shouldBe("getWidth('div2')", "'94px'");
> +    shouldBe("getWidth('div3')", "'94px'");

I'd like a comment about where this 94px comes from so someone looking at a
test failure can quickly figure it out.

>
LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:46
> +<div>Test for Bugzilla bug:<a
href="https://bugs.webkit.org/show_bug.cgi?id=99442"> 99442</a> Regression
r130057: Improper preferred width calculation when an inline replaced object,
wrapped in an inline flow, follows some text.</div>

Nit: super long line.