ssia.
Created attachment 284634 [details] Patch
Created attachment 284637 [details] Patch
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.
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.
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.
Created attachment 284641 [details] Patch
Created attachment 284644 [details] Patch
Comment on attachment 284644 [details] Patch Clearing flags on attachment: 284644 Committed r203751: <http://trac.webkit.org/changeset/203751>
All reviewed patches have been landed. Closing bug.