[webkit-reviews] review denied: [Bug 102784] Percentage width replaced element incorrectly rendered when intrinsic size changed : [Attachment 179140] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 12 16:07:35 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 179140: Patch
https://bugs.webkit.org/attachment.cgi?id=179140&action=review
------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179140&action=review
Close! Just some minor style nits.
> Source/WebCore/rendering/RenderImage.cpp:232
> + // If image's logical width type is percent and the containing block
fits to contents,
> + // relayout is needed because the 'newWidth' can be wrong because it
was calculated from the 'old containing block width'
I would remove this command and put this text in the ChangeLog instead.
> LayoutTests/fast/css/percent-width-img-src-change.html:11
> + if (image.src.substr(image.src.length - 12, 12) == "greenbox.png")
The hardcoded 12 is unfortunate. Maybe use a regular expression like
image.src.match("greenbox[.]png$").
> LayoutTests/fast/css/percent-width-img-src-change.html:16
> + if (image.width != 100)
> + ++failures;
It would be more helpful to output which test case failed. The number of
failures isn't that useful.
> LayoutTests/fast/css/percent-width-img-src-change.html:52
> +<div style="position:absolute;top:340px;left:11px">
> +<img class="imgForChange" style="max-width:100%;"
src="resources/greenbox.png" onload="imageLoaded(this)">
Please add a test for min-width. I think if the second image is smaller than
the first image and there's a percent min-width, we won't resize properly.
More information about the webkit-reviews
mailing list