[webkit-reviews] review denied: [Bug 85118] REGRESSION(r95249): Iframes are printed blank : [Attachment 139879] Iframe printing fix.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Vitaly Buka
<vitalybuka at chromium.org>'s request for review:
Bug 85118: REGRESSION(r95249): Iframes are printed blank
https://bugs.webkit.org/show_bug.cgi?id=85118

Attachment 139879: Iframe printing fix.
https://bugs.webkit.org/attachment.cgi?id=139879&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139879&action=review


Thanks for tackling this. Seems close to ready.

Given that the code changes are separate for horizontal vs. vertical writing
mode, we need test cases that cover both. Also, we changed the behavior when
contentRenderer is 0 so I want to see a test that covers that too.

> Source/WebCore/page/Frame.cpp:545
> +	   if (fabs(originalSize.width()) < numeric_limits<float>::epsilon())
> +	       return resultSize;

This does not seem like the right approach. We should have some cleaner way to
avoid trying to work with originalSize rather than relying on the fact that its
width is zero. I think the change should be at the printing-specific call site
rather than here in this function.

> Source/WebCore/rendering/RenderView.cpp:652
> +bool RenderView::usePrintingLayout() const

This function sounds like something that tells the view to use a printing
layout. That’s why we use names like shouldUsePrintingLayout.

> Source/WebCore/rendering/RenderView.cpp:658
> +    if (!printing() || !m_frameView || !m_frameView->frame())
> +	   return false;
> +    FrameTree* tree = m_frameView->frame()->tree();
> +    // Only root frame should have special handling for printing.
> +    return tree && !tree->parent();

Since tree can’t be null here is how this should be written:

    if (!printing() || !m_frameView)
	return false;
    Frame* frame = m_frameView->frame();
    return frame && !frame->tree()->parent();