Bug 21402 – Add a clip in RenderView::layout to avoid doing offscreen paint

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Julien Chaffraix]

Currently in RenderView.cpp, there is a FIXME about adding a clip to the LayoutState.

Path forthcoming.

[Julien Chaffraix]

Created attachment 24114 [details]
Proposed fix: add a clip to the viewRect

[Eric Seidel (no email)]

Comment on attachment 24114 [details]
Proposed fix: add a clip to the viewRect

This one is for Hyatt.

[Julien Chaffraix]

Comment on attachment 24114 [details]
Proposed fix: add a clip to the viewRect

As discussed with David Hyatt and Dan Bernstein, the win is odd and should be confirmed on ToT (I had done some rough testing on ToT and good experiment on an old bramch) before thinking of integrating this patch.

Clearing the review flag to give me some time for performance experiments on ToT.

[Ahmad Saleem]

I checked and we don't have FIXME in Webkit Source, which this patch was trying to modify and also this is current source:

https://searchfox.org/wubkat/source/Source/WebCore/rendering/RenderView.cpp#183


void RenderView::layout()
{
    StackStats::LayoutCheckPoint layoutCheckPoint;
    if (!document().paginated())
        m_pageLogicalSize = { };

    if (shouldUsePrintingLayout()) {
        if (!m_pageLogicalSize)
            m_pageLogicalSize = LayoutSize(logicalWidth(), 0_lu);
        m_minPreferredLogicalWidth = m_pageLogicalSize->width();
        m_maxPreferredLogicalWidth = m_minPreferredLogicalWidth;
    }

    // Use calcWidth/Height to get the new width/height, since this will take the full page zoom factor into account.
    bool relayoutChildren = !shouldUsePrintingLayout() && (width() != viewWidth() || height() != viewHeight());
    if (relayoutChildren) {
        setChildNeedsLayout(MarkOnlyThis);

        for (auto& box : childrenOfType<RenderBox>(*this)) {
            if (box.hasRelativeLogicalHeight()
                || box.style().logicalHeight().isPercentOrCalculated()
                || box.style().logicalMinHeight().isPercentOrCalculated()
                || box.style().logicalMaxHeight().isPercentOrCalculated()
                || box.isSVGRootOrLegacySVGRoot()
                )
                box.setChildNeedsLayout(MarkOnlyThis);
        }
    }

    ASSERT(!frameView().layoutContext().layoutState());
    if (!needsLayout())
        return;

    ensureLayoutState().setViewportSize(frameView().size());

    LayoutStateMaintainer statePusher(*this, { }, false, valueOrDefault(m_pageLogicalSize).height(), m_pageLogicalHeightChanged);

    m_pageLogicalHeightChanged = false;

    RenderBlockFlow::layout();

#ifndef NDEBUG
    frameView().layoutContext().checkLayoutState();
#endif
    clearNeedsLayout();
}

________

Do we need anything else? Or we can close now?

[Simon Fraser (smfr)]

I don't think we need this.