Bug 160215 – Move RenderView::shouldDisableLayoutStateForSubtree to SubtreeLayoutStateMaintainer.

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

[zalan]

ssia.

[zalan]

Created attachment 284634 [details]
Patch

[zalan]

Created attachment 284637 [details]
Patch

[Darin Adler]

Comment on attachment 284637 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:204
>      bool m_disableLayoutState { false };

The name of this isn’t great. It keeps track of whether we already disabled layout state, and it’s confusing that its name is verb.

[Darin Adler]

Comment on attachment 284637 [details]
Patch

I think the old factoring was clearer, where one function makes the decision about whether to disable layout state, and another does the disabling. I’m not sure that this is an improvement. It’s especially poor to put the actual work of disabling layout state inside a loop just before a return statement. Too subtle I think.

[Simon Fraser (smfr)]

Comment on attachment 284637 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:190
> +    void disableLayoutStateForSubtree()

disableIfNeeded?

> Source/WebCore/page/FrameView.cpp:196
> +        for (auto* renderer = m_layoutRoot; renderer; renderer = renderer->container()) {
> +            if (renderer->hasTransform() || renderer->hasReflection()) {
> +                m_disableLayoutState = true;
> +                m_layoutRoot->view().disableLayoutState();
> +                return;

Slight preference for assignment to m_disableLayoutState in one place. You could hoist the loop into a separate function.

[zalan]

Created attachment 284641 [details]
Patch

[zalan]

Created attachment 284644 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284644 [details]
Patch

Clearing flags on attachment: 284644

Committed r203751: <http://trac.webkit.org/changeset/203751>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.