[webkit-reviews] review denied: [Bug 102784] Percentage width replaced element incorrectly rendered when intrinsic size changed : [Attachment 176390] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 28 10:03:19 PST 2012


Tony Chang <tony at chromium.org> has denied KyungTae Kim <ktf.kim at samsung.com>'s
request for review:
Bug 102784: Percentage width replaced element incorrectly rendered when
intrinsic size changed
https://bugs.webkit.org/show_bug.cgi?id=102784

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176390&action=review


Upon further discussion and inspection, I think the bug is in
RenderBox::computeReplacedLogicalWidthUsing.  Specifically, the if (cw > 0 ||)
check seems fishy.  I'm not entirely sure what the check is trying to do, but
it seems like an accident that the first time cw is 0 (because we haven't done
a layout yet) and the second time we use the old cw value.  I think we want the
code to fall through to intrinsicLogicalWidth() on relayout.

> LayoutTests/fast/css/percent-width-img-src-change-expected.html:7
> +<!-- The below boxes should have intrinsic size of greenbox-100px.png
(100x100) -->
> +

This should be a dumpAsText test.  You can use check-layout.js or
js-test-pre.js.  The benefits are that the test output will be clearer (it will
say PASS or FAIL with a useful message) and it will run faster.


More information about the webkit-reviews mailing list