[webkit-reviews] review denied: [Bug 94237] iframe does not stretch in flexbox : [Attachment 159147] Patch

Fri Aug 17 11:21:28 PDT 2012

Ojan Vafai <ojan at chromium.org> has denied Shezan Baig <shezbaig.wk at gmail.com>'s
request for review:
Bug 94237: iframe does not stretch in flexbox
https://bugs.webkit.org/show_bug.cgi?id=94237

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

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

Thanks for the patch! At a high-level this looks good to me. Just needs a bit
more work.

shouldComputeSizeAsReplaced is overridden by RenderIframe. Can you add a test
to make sure seamless iframes work? Can you also add a test to make sure that
stretch will shrink the iframe if it's too large (e.g. make the flexbox's
width/height 100px and the iframe should shrink appropriately).

> Source/WebCore/ChangeLog:18
> +	   (WebCore::RenderBox::shouldComputeSizeAsReplaced): Make this public
so
> +	   that it can be used from RenderFlexibleBox.

RenderFlexibleBox inherits from RenderBox, so leaving it protected should be
sufficient.

> Source/WebCore/ChangeLog:23
> +	   (WebCore::RenderFlexibleBox::applyStretchAlignmentToChild): Set the
> +	   override width and height if the child computes its size as a
replaced
> +	   element.

I like this! As a followup patch, maybe we could cleanup
RenderDeprecatedFlexibleBox the same way so that RenderBox doesn't need to know
as much about deprecated flexboxes.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1255
>	   LayoutUnit childWidth = lineCrossAxisExtent -
crossAxisMarginExtentForChild(child);
>	   child->setOverrideLogicalContentWidth(std::max(ZERO_LAYOUT_UNIT,
childWidth));

While I'm looking at this code, it occurs to me that we only need to layout if
the width changes. Mind adding a FIXME while you're here? Also, if we did that,
I'm not sure we need the isMultiline check. I think that's just a performance
optimization to avoid layouts. Tony, WDYT?

> LayoutTests/css3/flexbox/flexitem.html:19
> +.flexboxVert {

How about s/flexboxVert/column? That's what we do in other tests as we don't
want to confuse it with vertical writing-mode.

> LayoutTests/css3/flexbox/flexitem.html:21
> +    height: 600px;

For easily test management, can you make this height 200px? It's easier when
you don't have to scroll as much to get to each testcase.

> LayoutTests/css3/flexbox/flexitem.html:25
> +    min-width: 10px;

Doesn't seem like you test this case, so no point in setting the CSS.

> LayoutTests/css3/flexbox/flexitem.html:112
> +    <div data-expected-display="block" style="-webkit-flex: 1 0 auto;
height:400px;"></div>

This div doesn't seem to add value as it's just testing the display, which is
covered by other tests.

> LayoutTests/css3/flexbox/flexitem.html:113
> +    <iframe data-expected-display="block" data-expected-height="400"
style="-webkit-flex: 1 0 auto;" src="data:text/html,<body
bgcolor=#fff>iframe</body>"></iframe>

If you leave out the src, I think you can just set a background-color on the
iframe element itself and get the same effect.

> LayoutTests/css3/flexbox/flexitem.html:116
> +<div class="flexboxVert">

Can you make the class="flexbox column"? Then you don't need to add the extra
checkLayout call and you can remove the redundant CSS styling above.

> LayoutTests/css3/flexbox/flexitem.html:129
> +<div class="flexboxVert">
> +    <iframe data-expected-display="block" data-expected-width="600"
style="-webkit-flex: 1 0 auto;" src="data:text/html,<body
bgcolor=#fff>iframe</body>"></iframe>
> +</div>

Can you add data-expected-width to the above case? Then this would be redundant
and you could delete it. Also, that way, we'd make sure stretch works on the
other replaced element types as well.