Bug 194198 - Make setNeedsLayout on the root more explicitly about triggering its side-effects
Summary: Make setNeedsLayout on the root more explicitly about triggering its side-eff...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks: 194199
  Show dependency treegraph
 
Reported: 2019-02-02 17:20 PST by Simon Fraser (smfr)
Modified: 2019-02-04 12:54 PST (History)
8 users (show)

See Also:


Attachments
Patch (25.52 KB, patch)
2019-02-02 17:32 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (25.77 KB, patch)
2019-02-02 17:45 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.51 MB, application/zip)
2019-02-02 19:06 PST, Build Bot
no flags Details
Patch (25.93 KB, patch)
2019-02-02 20:34 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-02-02 17:20:57 PST
Make setNeedsLayout on the root more explicitly about triggering its side-effects
Comment 1 Simon Fraser (smfr) 2019-02-02 17:32:04 PST Comment hidden (obsolete)
Comment 2 Simon Fraser (smfr) 2019-02-02 17:45:26 PST Comment hidden (obsolete)
Comment 3 Build Bot 2019-02-02 19:06:55 PST Comment hidden (obsolete)
Comment 4 Build Bot 2019-02-02 19:06:56 PST Comment hidden (obsolete)
Comment 5 Simon Fraser (smfr) 2019-02-02 20:34:30 PST
Created attachment 360995 [details]
Patch
Comment 6 Antti Koivisto 2019-02-03 10:08:06 PST
Comment on attachment 360995 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2019-02-03 17:48:19 PST
Comment on attachment 360995 [details]
Patch

Clearing flags on attachment: 360995

Committed r240912: <https://trac.webkit.org/changeset/240912>
Comment 8 WebKit Commit Bot 2019-02-03 17:48:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-02-03 17:49:30 PST
<rdar://problem/47776051>
Comment 10 Michael Catanzaro 2019-02-04 07:48:48 PST
Committed r240923: <https://trac.webkit.org/changeset/240923>
Comment 11 zalan 2019-02-04 08:07:16 PST
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.