[webkit-reviews] review granted: [Bug 95892] REGRESSION(r122501): replaced elements with percent width are wrongly size when inserted inside an auto-table layout : [Attachment 163666] Updated change. Fix existing vertical-writing mode bug + took Ojan & Elliot's comments into account.

Wed Sep 12 12:07:16 PDT 2012

Ojan Vafai <ojan at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 95892: REGRESSION(r122501): replaced elements with percent width are
wrongly size when inserted inside an auto-table layout
https://bugs.webkit.org/show_bug.cgi?id=95892

Attachment 163666: Updated change. Fix existing vertical-writing mode bug +
took Ojan & Elliot's comments into account.
https://bugs.webkit.org/attachment.cgi?id=163666&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163666&action=review

Please address the test comments before committing. I can show you a
check-layout.js example in person.

>
LayoutTests/fast/replaced/vertical-writing-mode-max-logical-width-replaced.html
:1
> +<!DOCTYPE html>

You should make this a check-layout.js test! You could replace all of your
script except the dumpAsText part with data-expected-width=200
data-expected-height=100 attributes on the image.

> LayoutTests/fast/table/bad-replaced-sizing-preferred-logical-widths.html:20
> +	   <iframe src="resources/iframe.html" width="100%"></iframe>

Can you just use a data url or srcdoc attribute? Then you don't need to add
another file.

> LayoutTests/fast/table/bad-replaced-sizing-preferred-logical-widths.html:43
> +    // 50% is a somewhat arbitrary threshold but it should shield us from
the viewport size.

How about instead setting a fixed width on the html or body element and then
here you can assert actual pixel sizes? Then this could also just be a
check-layout.js test.