Changeset 238431 in webkit

Timestamp:

Nov 21, 2018, 3:25:13 PM (8 years ago)

Author:

Alan Bujtas

Message:

[LFC] LayoutState should always be initialized with the initial containing block.
​https://bugs.webkit.org/show_bug.cgi?id=191896

Reviewed by Antti Koivisto.

There should always be only one LayoutState per layout tree (it does not mean that layout always starts at the ICB).
The ICB is a special formatting context root because it does not have a parent formatting context. All the other formatting contexts
first need to be laid out (partially at least e.g margin) in their parent formatting context.
Having a non-null parent formatting context as root could lead to undefined behaviour.

• layout/LayoutFormattingState.cpp:

(WebCore::Layout::LayoutState::LayoutState):
(WebCore::Layout::LayoutState::initializeRoot): Deleted.

• layout/LayoutFormattingState.h:
• layout/Verification.cpp:

(WebCore::Layout::LayoutState::verifyAndOutputMismatchingLayoutTree const):

• page/FrameViewLayoutContext.cpp:

(WebCore::layoutUsingFormattingContext):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• layout/LayoutFormattingState.cpp (modified) (2 diffs)
• layout/LayoutFormattingState.h (modified) (3 diffs)
• layout/Verification.cpp (modified) (1 diff)
• page/FrameViewLayoutContext.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-11-21  Zalan Bujtas  <zalan@apple.com>
+
+        [LFC] LayoutState should always be initialized with the initial containing block.
+        https://bugs.webkit.org/show_bug.cgi?id=191896
+
+        Reviewed by Antti Koivisto.
+
+        There should always be only one LayoutState per layout tree (it does not mean that layout always starts at the ICB).
+        The ICB is a special formatting context root because it does not have a parent formatting context. All the other formatting contexts
+        first need to be laid out (partially at least e.g margin) in their parent formatting context.
+        Having a non-null parent formatting context as root could lead to undefined behaviour.
+
+        * layout/LayoutFormattingState.cpp:
+        (WebCore::Layout::LayoutState::LayoutState):
+        (WebCore::Layout::LayoutState::initializeRoot): Deleted.
+        * layout/LayoutFormattingState.h:
+        * layout/Verification.cpp:
+        (WebCore::Layout::LayoutState::verifyAndOutputMismatchingLayoutTree const):
+        * page/FrameViewLayoutContext.cpp:
+        (WebCore::layoutUsingFormattingContext):
+
 2018-11-21  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/layout/LayoutFormattingState.cpp

@@ -46 +46 @@
 WTF_MAKE_ISO_ALLOCATED_IMPL(LayoutState);
 
-LayoutState::LayoutState()
+LayoutState::LayoutState(const Container& initialContainingBlock, const LayoutSize& containerSize)
+    : m_initialContainingBlock(makeWeakPtr(initialContainingBlock))
 {
-}
+    // LayoutState is always initiated with the ICB.
+    ASSERT(!initialContainingBlock.parent());
+    ASSERT(initialContainingBlock.establishesBlockFormattingContext());
 
-void LayoutState::initializeRoot(const Container& root, const LayoutSize& containerSize)
-{
-    ASSERT(root.establishesFormattingContext());
-
-    m_root = makeWeakPtr(root);
-    auto& displayBox = displayBoxForLayoutBox(root);
-
-    // FIXME: m_root could very well be a formatting context root with ancestors and resolvable border and padding (as opposed to the topmost root)
+    auto& displayBox = displayBoxForLayoutBox(initialContainingBlock);
     displayBox.setHorizontalMargin({ });
     displayBox.setHorizontalNonComputedMargin({ });
@@ -64 +60 @@
     displayBox.setBorder({ });
     displayBox.setPadding({ });
+    displayBox.setTopLeft({ });
     displayBox.setContentBoxHeight(containerSize.height());
     displayBox.setContentBoxWidth(containerSize.width());
-    displayBox.setTopLeft({ });
 
-    m_formattingContextRootListForLayout.add(&root);
+    m_formattingContextRootListForLayout.add(&initialContainingBlock);
 }
 

trunk/Source/WebCore/layout/LayoutFormattingState.h

@@ -50 +50 @@
 class FormattingState;
 
-// LayoutState is the entry point for layout. It takes a (formatting root)container which acts as the root of the layout context.
+// LayoutState is the entry point for layout. It takes the initial containing block which acts as the root of the layout context.
 // LayoutState::layout() generates the display tree for the root container's subtree (it does not run layout on the root though).
-// Note, while the root container is suppposed to be the entry point for the initial layout, it does not necessarily need to be the entry point of any
+// Note, while the initial containing block is entry point for the initial layout, it does not necessarily need to be the entry point of any
 // subsequent layouts (subtree layout). A non-initial, subtree layout could be initiated on multiple formatting contexts.
 // Each formatting context has an entry point for layout, which potenitally means multiple entry points per layout frame.
@@ -59 +59 @@
     WTF_MAKE_ISO_ALLOCATED(LayoutState);
 public:
-    LayoutState();
+    LayoutState(const Container& initialContainingBlock, const LayoutSize&);
 
-    void initializeRoot(const Container&, const LayoutSize&);
     void updateLayout();
     void styleChanged(const Box&, StyleDiff);
@@ -87 +86 @@
 
 private:
+    const Container& initialContainingBlock() const { return *m_initialContainingBlock; }
     void layoutFormattingContextSubtree(const Box&);
 
-    WeakPtr<const Container> m_root;
+    WeakPtr<const Container> m_initialContainingBlock;
     HashSet<const Container*> m_formattingContextRootListForLayout;
     HashMap<const Box*, std::unique_ptr<FormattingState>> m_formattingStates;

trunk/Source/WebCore/layout/Verification.cpp

@@ -322 +322 @@
 {
     TextStream stream;
-    auto mismatchingGeometry = verifyAndOutputSubtree(stream, *this, renderView, *m_root.get());
+    auto mismatchingGeometry = verifyAndOutputSubtree(stream, *this, renderView, initialContainingBlock());
     if (!mismatchingGeometry)
         return;
 #if ENABLE(TREE_DEBUGGING)
     showRenderTree(&renderView);
-    showLayoutTree(*m_root.get(), this);
+    showLayoutTree(initialContainingBlock(), this);
 #endif
     WTFLogAlways("%s", stream.release().utf8().data());

trunk/Source/WebCore/page/FrameViewLayoutContext.cpp

@@ -57 +57 @@
 {
     auto initialContainingBlock = Layout::TreeBuilder::createLayoutTree(renderView);
-    auto layoutState = std::make_unique<Layout::LayoutState>();
-    layoutState->initializeRoot(*initialContainingBlock, renderView.size());
+    auto layoutState = std::make_unique<Layout::LayoutState>(*initialContainingBlock, renderView.size());
     layoutState->setInQuirksMode(renderView.document().inQuirksMode());
     layoutState->updateLayout();