Bug 194198 – Make setNeedsLayout on the root more explicitly about triggering its side-effects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Make setNeedsLayout on the root more explicitly about triggering its side-effects

[Simon Fraser (smfr)]

Created attachment 360988 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 360989 [details]
Patch

[EWS Watchlist]

Comment on attachment 360989 [details]
Patch

Attachment 360989 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11011001

New failing tests:
tiled-drawing/header-and-footer-hit-testing-in-frame.html

[EWS Watchlist]

Created attachment 360991 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Simon Fraser (smfr)]

Created attachment 360995 [details]
Patch

[Antti Koivisto]

Comment on attachment 360995 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 360995 [details]
Patch

Clearing flags on attachment: 360995

Committed r240912: <https://trac.webkit.org/changeset/240912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47776051>

[Michael Catanzaro]

Committed r240923: <https://trac.webkit.org/changeset/240923>

[zalan]

Comment on attachment 360995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360995&action=review

> Source/WebCore/ChangeLog:8
> +        Calling setNeedsLayout() on the FrameView or RenderView is an odd concept; the render tree

I think calling setNeedsLayout on the RenderView is an ok concept. It just means that the ICB got resized and now it needs to propagate this new size to its descendants (like stretchy html/body in quirks mode, or just width in general).

> Source/WebCore/page/FrameView.cpp:2928
> +    RenderView* renderView = this->renderView();
> +    if (renderView->usesCompositing()) {
> +        if (auto* rootLayer = renderView->layer())
> +            renderView->layer()->setNeedsCompositingConfigurationUpdate();

I'd prefer early returns here:

{
    auto* renderView = this->renderView();
    if (!renderView->usesCompositing())
        return;
    auto* rootLayer = renderView->layer()
    if (!rootLayer)
        return;
    rootLayer->setNeedsCompositingConfigurationUpdate();
}

or just fix:
if (auto* rootLayer = renderView->layer())
     renderView->layer()->setNeedsCompositingConfigurationUpdate();
->
     rootLayer->setNeedsCompositingConfigurationUpdate();

> Source/WebCore/page/FrameView.cpp:2938
> +    RenderView* renderView = this->renderView();
> +    if (renderView->usesCompositing()) {
> +        if (auto* rootLayer = renderView->layer())
> +            renderView->layer()->setNeedsCompositingGeometryUpdate();
> +    }

same as above

> Source/WebCore/page/FrameViewLayoutContext.cpp:326
>          renderView->setNeedsLayout();
> +        scheduleLayout();

It's odd that the RenderView is the only renderer in the tree that does not schedule layout. I think this should be fixed in setNeedsLayout.