[webkit-reviews] review denied: [Bug 86608] Add seamless layout code (and pass all the remaining seamless tests) : [Attachment 142553] now with more splelnig fixes

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 86608: Add seamless layout code (and pass all the remaining seamless tests)
https://bugs.webkit.org/show_bug.cgi?id=86608

Attachment 142553: now with more splelnig fixes
https://bugs.webkit.org/attachment.cgi?id=142553&action=review

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


I'm almost comfortable r+'ing this. Mainly, I'd like to see the vertical
writing mode tests for the scary bit in layoutSeamlessly. You really should try
to get Hyatt to take a quick look at this patch if you can. If nothing else,
just have him verify that the code in layoutSeamlessly is sane, that's the only
part that I'm wary of since it's doing something quite different from normal
layout methods.

> Source/WebCore/page/FrameView.cpp:2928
> +	   if (iframeRenderer->flattenFrame() || iframeRenderer->isSeamless())

Doesn't this change behavior for frame flattening if frameFlatteningEnabled is
false? i.e. we will frame flatten even though the setting was false. I think
you need to check !m_frame->settings() ||
!m_frame->settings()->frameFlatteningEnabled() here as well.

> Source/WebCore/rendering/RenderIFrame.cpp:174
> +    // Normally we would set our height to 0 before laying out our kids.
> +    // However RenderView (the root of the child document's rendering tree)
> +    // assumes its container is of a fixed size, not dynamically grown to
it.
> +    // So we set our height to min(max-height, MAX_LAYOUT_UNIT) to avoid
generating unnecessary
> +    // vertical scrollbars then we'll shrink the height after child layout.
> +    LayoutUnit maxHeight = MAX_LAYOUT_UNIT;
> +    if (!style()->logicalMaxHeight().isUndefined())
> +	   maxHeight = std::min<LayoutUnit>(maxHeight,
style()->logicalMaxHeight().value());
> +    setLogicalHeight(maxHeight);

This scares me, e.g., I highly doubt it does the right thing with vertical
writing mode. You could add some vertical writing mode tests to verify.
Specifically, write tests with enough content to wrap into multiple lines *and*
overflow horizontally out of their container. Make sure they render the same as
a vertical writing mode test that uses a div. In fact, you could write these as
reftests! :)

> Source/WebCore/rendering/RenderIFrame.cpp:178
> +    // Replaced elements do not respect padding, so we just add border to
the child's height.

I'm on the fence as to whether seamless iframes should respect padding. I think
they probably should. It's not a big deal either way. Can you just add a FIXME
for now?

> Source/WebCore/rendering/RenderIFrame.cpp:204
> +	   // Note we do not return so as to share the layer and overflow
updates below.

Pet peeve nit: s/Note we/We/ :)

> Source/WebCore/rendering/RenderIFrame.cpp:207
> +	   // No kids to layout as a replaced element.

Grammar nit: No kids to layout as *this is* a replaced element.