Attachment 358974 Details for Bug 193375 – Patch

[patch] Patch

bug-193375-20190111191553.patch (text/plain), 14.86 KB, created by zalan on 2019-01-11 19:16:05 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2019-01-11 19:16:05 PST

Size: 14.86 KB

patch

obsolete

>Subversion Revision: 239863
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index da837e764364a8367dd180a8a456daa05946636a..0284be90941712134bad8bf3e6c83680bf7510df 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,43 @@
>+2019-01-11  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][BFC][MarginCollapsing] Move estimatedMarginBefore flag from state/display box to BlockFormattingContext
>+        https://bugs.webkit.org/show_bug.cgi?id=193375
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The estimated marginBefore is a pre-computed, temporary value. We need to keep it around until the final vertical margin value is computed.
>+        Neither BlockFormattingState nor Display should hold temporary values.
>+
>+        * layout/blockformatting/BlockFormattingContext.cpp:
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginBefore const):
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginBeforeForAncestors const):
>+        (WebCore::Layout::BlockFormattingContext::hasPrecomputedMarginBefore const):
>+        (WebCore::Layout::BlockFormattingContext::computeFloatingPosition const):
>+        (WebCore::Layout::BlockFormattingContext::computePositionToAvoidFloats const):
>+        (WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const):
>+        (WebCore::Layout::BlockFormattingContext::computeHeightAndMargin const):
>+        (WebCore::Layout::BlockFormattingContext::setEstimatedMarginBefore const):
>+        (WebCore::Layout::BlockFormattingContext::hasEstimatedMarginBefore const):
>+        (WebCore::Layout::hasPrecomputedMarginBefore): Deleted.
>+        * layout/blockformatting/BlockFormattingContext.h:
>+        (WebCore::Layout::BlockFormattingContext::removeEstimatedMarginBefore const):
>+        (WebCore::Layout::BlockFormattingContext::estimatedMarginBefore const):
>+        * layout/blockformatting/BlockFormattingState.h:
>+        (WebCore::Layout::BlockFormattingState::setHasEstimatedMarginBefore): Deleted.
>+        (WebCore::Layout::BlockFormattingState::clearHasEstimatedMarginBefore): Deleted.
>+        (WebCore::Layout::BlockFormattingState::hasEstimatedMarginBefore const): Deleted.
>+        * layout/displaytree/DisplayBox.cpp:
>+        (WebCore::Display::Box::Box):
>+        * layout/displaytree/DisplayBox.h:
>+        (WebCore::Display::Box::setHasEstimatedMarginBefore):
>+        (WebCore::Display::Box::invalidateEstimatedMarginBefore):
>+        (WebCore::Display::Box::top const):
>+        (WebCore::Display::Box::topLeft const):
>+        (WebCore::Display::Box::setEstimatedMarginBefore): Deleted.
>+        (WebCore::Display::Box::estimatedMarginBefore const): Deleted.
>+        * page/FrameViewLayoutContext.cpp:
>+        (WebCore::layoutUsingFormattingContext):
>+
> 2019-01-11  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC][MarginCollapsing] Adjust vertical position when box margin collapses through.
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index c3b1d823124069b01218067b4dcce52ca54bbfda..765baf9b932ff5a403f670539fa0fa99c5a98c27 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -188,7 +188,7 @@ void BlockFormattingContext::computeEstimatedMarginBefore(const Box& layoutBox)
> {
>     auto& layoutState = this->layoutState();
>     auto estimatedMarginBefore = MarginCollapse::estimatedMarginBefore(layoutState, layoutBox);
>-    blockFormattingState().setHasEstimatedMarginBefore(layoutBox);
>+    setEstimatedMarginBefore(layoutBox, estimatedMarginBefore);
> 
>     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>     auto nonCollapsedValues = UsedVerticalMargin::NonCollapsedValues { estimatedMarginBefore.nonCollapsedValue, { } };
>@@ -196,7 +196,7 @@ void BlockFormattingContext::computeEstimatedMarginBefore(const Box& layoutBox)
>     auto verticalMargin = UsedVerticalMargin { nonCollapsedValues, collapsedValues };
>     displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
> #if !ASSERT_DISABLED
>-    displayBox.setEstimatedMarginBefore(estimatedMarginBefore);
>+    displayBox.setHasEstimatedMarginBefore();
> #endif
> }
> 
>@@ -214,10 +214,9 @@ void BlockFormattingContext::computeEstimatedMarginBeforeForAncestors(const Box&
>     // So when we get to the point where we intersect the box with the float to decide if the box needs to move, we don't yet have the final vertical position.
>     //
>     // The idea here is that as long as we don't cross the block formatting context boundary, we should be able to pre-compute the final top margin.
>-    auto& formattingState = blockFormattingState();
>     for (auto* ancestor = layoutBox.containingBlock(); ancestor && !ancestor->establishesBlockFormattingContext(); ancestor = ancestor->containingBlock()) {
>         // FIXME: with incremental layout, we might actually have a valid (non-estimated) margin top as well.
>-        if (formattingState.hasEstimatedMarginBefore(*ancestor))
>+        if (hasEstimatedMarginBefore(*ancestor))
>             return;
> 
>         computeEstimatedMarginBefore(*ancestor);
>@@ -242,10 +241,10 @@ void BlockFormattingContext::precomputeVerticalPositionForFormattingRootIfNeeded
> }
> 
> #ifndef NDEBUG
>-static bool hasPrecomputedMarginBefore(const BlockFormattingState& formattingState, const Box& layoutBox)
>+bool BlockFormattingContext::hasPrecomputedMarginBefore(const Box& layoutBox) const
> {
>     for (auto* ancestor = layoutBox.containingBlock(); ancestor && !ancestor->establishesBlockFormattingContext(); ancestor = ancestor->containingBlock()) {
>-        if (formattingState.hasEstimatedMarginBefore(*ancestor))
>+        if (hasEstimatedMarginBefore(*ancestor))
>             continue;
>         return false;
>     }
>@@ -257,7 +256,7 @@ void BlockFormattingContext::computeFloatingPosition(const FloatingContext& floa
> {
>     auto& layoutState = this->layoutState();
>     ASSERT(layoutBox.isFloatingPositioned());
>-    ASSERT(hasPrecomputedMarginBefore(blockFormattingState(), layoutBox));
>+    ASSERT(hasPrecomputedMarginBefore(layoutBox));
> 
>     auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox);
>     // 8.3.1 Collapsing margins
>@@ -277,7 +276,7 @@ void BlockFormattingContext::computePositionToAvoidFloats(const FloatingContext&
>     ASSERT(layoutBox.establishesBlockFormattingContext());
>     ASSERT(!layoutBox.isFloatingPositioned());
>     ASSERT(!layoutBox.hasFloatClear());
>-    ASSERT(hasPrecomputedMarginBefore(blockFormattingState(), layoutBox));
>+    ASSERT(hasPrecomputedMarginBefore(layoutBox));
> 
>     if (floatingContext.floatingState().isEmpty())
>         return;
>@@ -296,7 +295,7 @@ void BlockFormattingContext::computeVerticalPositionForFloatClear(const Floating
>     // For formatting roots, we already precomputed final position.
>     if (!layoutBox.establishesFormattingContext())
>         computeEstimatedMarginBeforeForAncestors(layoutBox);
>-    ASSERT(hasPrecomputedMarginBefore(blockFormattingState(), layoutBox));
>+    ASSERT(hasPrecomputedMarginBefore(layoutBox));
> 
>     if (auto verticalPositionWithClearance = floatingContext.verticalPositionWithClearance(layoutBox))
>         layoutState.displayBoxForLayoutBox(layoutBox).setTop(*verticalPositionWithClearance);
>@@ -384,18 +383,15 @@ void BlockFormattingContext::computeHeightAndMargin(const Box& layoutBox) const
> 
>     // Out of flow boxes don't need vertical adjustment after margin collapsing.
>     if (layoutBox.isOutOfFlowPositioned()) {
>+        ASSERT(!hasEstimatedMarginBefore(layoutBox));
>         displayBox.setContentBoxHeight(heightAndMargin.height);
>         displayBox.setVerticalMargin(verticalMargin);
>         return;
>     }
> 
>-    auto& formattingState = this->blockFormattingState();
>-    if (formattingState.hasEstimatedMarginBefore(layoutBox)) {
>-        formattingState.clearHasEstimatedMarginBefore(layoutBox);
>-        ASSERT(displayBox.estimatedMarginBefore()->usedValue() == verticalMargin.before());
>-    } else
>-        displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
>-
>+    ASSERT(!hasEstimatedMarginBefore(layoutBox) || estimatedMarginBefore(layoutBox).usedValue() == verticalMargin.before());
>+    removeEstimatedMarginBefore(layoutBox);
>+    displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin));
>     // Adjust the previous sibling's margin bottom now that this box's vertical margin is computed.
>     if (MarginCollapse::marginBeforeCollapsesWithPreviousSiblingMarginAfter(layoutState, layoutBox))
>         MarginCollapse::updateCollapsedMarginAfter(layoutState, *layoutBox.previousInFlowSibling(), verticalMargin);
>@@ -524,6 +520,20 @@ LayoutUnit BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing
>     return containingBlockContentBoxTop + verticalMargin.before();
> }
> 
>+void BlockFormattingContext::setEstimatedMarginBefore(const Box& layoutBox, const EstimatedMarginBefore& estimatedMarginBefore) const
>+{ 
>+    // Can't cross formatting context boundary.
>+    ASSERT(&layoutState().formattingStateForBox(layoutBox) == &formattingState());
>+    m_estimatedMarginBeforeList.set(&layoutBox, estimatedMarginBefore);
>+}
>+
>+bool BlockFormattingContext::hasEstimatedMarginBefore(const Box& layoutBox) const
>+{
>+    // Can't cross formatting context boundary.
>+    ASSERT(&layoutState().formattingStateForBox(layoutBox) == &formattingState());
>+    return m_estimatedMarginBeforeList.contains(&layoutBox);
>+}
>+
> }
> }
> 
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>index a2cfdad2017fae77103e8081c311e6f556608e10..8c837b0abfe33ec66f3702d16f6956d8f27988b3 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>@@ -28,6 +28,8 @@
> #if ENABLE(LAYOUT_FORMATTING_CONTEXT)
> 
> #include "FormattingContext.h"
>+#include "MarginTypes.h"
>+#include <wtf/HashMap.h>
> #include <wtf/IsoMalloc.h>
> 
> namespace WebCore {
>@@ -126,6 +128,17 @@ private:
>         static bool shouldIgnoreMarginBefore(const LayoutState&, const Box&);
>         static bool shouldIgnoreMarginAfter(const LayoutState&, const Box&);
>     };
>+
>+    void setEstimatedMarginBefore(const Box&, const EstimatedMarginBefore&) const;
>+    void removeEstimatedMarginBefore(const Box& layoutBox) const { m_estimatedMarginBeforeList.remove(&layoutBox); }
>+    bool hasEstimatedMarginBefore(const Box&) const;
>+#ifndef NDEBUG
>+    EstimatedMarginBefore estimatedMarginBefore(const Box& layoutBox) const { return m_estimatedMarginBeforeList.get(&layoutBox); }
>+    bool hasPrecomputedMarginBefore(const Box&) const;
>+#endif
>+
>+private:
>+    mutable HashMap<const Box*, EstimatedMarginBefore> m_estimatedMarginBeforeList;
> };
> 
> }
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingState.h b/Source/WebCore/layout/blockformatting/BlockFormattingState.h
>index bb456b5927fefe45c637831d5b24f717092bad2a..b28ef695f148c3f91afabd838c98b7776df436ac 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingState.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingState.h
>@@ -28,7 +28,6 @@
> #if ENABLE(LAYOUT_FORMATTING_CONTEXT)
> 
> #include "FormattingState.h"
>-#include "MarginTypes.h"
> #include <wtf/IsoMalloc.h>
> 
> namespace WebCore {
>@@ -48,13 +47,8 @@ public:
>     bool hasPositiveAndNegativeVerticalMargin(const Box& layoutBox) const { return m_positiveAndNegativeVerticalMargin.contains(&layoutBox); }
>     PositiveAndNegativeVerticalMargin positiveAndNegativeVerticalMargin(const Box& layoutBox) const { return m_positiveAndNegativeVerticalMargin.get(&layoutBox); }
> 
>-    void setHasEstimatedMarginBefore(const Box& layoutBox) { m_estimatedMarginBeforeList.add(&layoutBox); }
>-    void clearHasEstimatedMarginBefore(const Box& layoutBox) { m_estimatedMarginBeforeList.remove(&layoutBox); }
>-    bool hasEstimatedMarginBefore(const Box& layoutBox) const { return m_estimatedMarginBeforeList.contains(&layoutBox); }
>-
> private:
>     HashMap<const Box*, PositiveAndNegativeVerticalMargin> m_positiveAndNegativeVerticalMargin;
>-    HashSet<const Box*> m_estimatedMarginBeforeList;
> };
> 
> }
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.cpp b/Source/WebCore/layout/displaytree/DisplayBox.cpp
>index 631ef71b983c715ad7e2ead54c0c152ec95102ea..0eeeaecd374aabdb3aeef00c8f3124a6cae0af39 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.cpp
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.cpp
>@@ -74,7 +74,7 @@ Box::Box(const Box& other)
>     , m_hasValidPadding(other.m_hasValidPadding)
>     , m_hasValidContentHeight(other.m_hasValidContentHeight)
>     , m_hasValidContentWidth(other.m_hasValidContentWidth)
>-    , m_estimatedMarginBefore(other.m_estimatedMarginBefore)
>+    , m_hasEstimatedMarginBefore(other.m_hasEstimatedMarginBefore)
> #endif
> {
> }
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.h b/Source/WebCore/layout/displaytree/DisplayBox.h
>index 9aefb18d7759735820f34b0a0333302e5dc72719..16782c44ebcb765869f1691174131572b17df065 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.h
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.h
>@@ -175,8 +175,7 @@ public:
>     Rect contentBox() const;
> 
> #if !ASSERT_DISABLED
>-    void setEstimatedMarginBefore(Layout::EstimatedMarginBefore marginBefore) { m_estimatedMarginBefore = marginBefore; }
>-    Optional<Layout::EstimatedMarginBefore> estimatedMarginBefore() const { return m_estimatedMarginBefore; }
>+    void setHasEstimatedMarginBefore() { m_hasEstimatedMarginBefore = true; }
> #endif
> 
> private:
>@@ -207,7 +206,7 @@ private:
>     void invalidateMargin();
>     void invalidateBorder() { m_hasValidBorder = false; }
>     void invalidatePadding() { m_hasValidPadding = false; }
>-    void invalidateEstimatedMarginBefore() { m_estimatedMarginBefore = { }; }
>+    void invalidateEstimatedMarginBefore() { m_hasEstimatedMarginBefore = false; }
> 
>     void setHasValidTop() { m_hasValidTop = true; }
>     void setHasValidLeft() { m_hasValidLeft = true; }
>@@ -248,7 +247,7 @@ private:
>     bool m_hasValidPadding { false };
>     bool m_hasValidContentHeight { false };
>     bool m_hasValidContentWidth { false };
>-    Optional<Layout::EstimatedMarginBefore> m_estimatedMarginBefore;
>+    bool m_hasEstimatedMarginBefore { false };
> #endif
> };
> 
>@@ -443,7 +442,7 @@ inline Box::Rect::operator LayoutRect() const
> 
> inline LayoutUnit Box::top() const
> {
>-    ASSERT(m_hasValidTop && (m_estimatedMarginBefore || m_hasValidVerticalMargin));
>+    ASSERT(m_hasValidTop && (m_hasEstimatedMarginBefore || m_hasValidVerticalMargin));
>     return m_topLeft.y();
> }
> 
>@@ -455,7 +454,7 @@ inline LayoutUnit Box::left() const
> 
> inline LayoutPoint Box::topLeft() const
> {
>-    ASSERT(m_hasValidTop && (m_estimatedMarginBefore || m_hasValidVerticalMargin));
>+    ASSERT(m_hasValidTop && (m_hasEstimatedMarginBefore || m_hasValidVerticalMargin));
>     ASSERT(m_hasValidLeft && m_hasValidHorizontalMargin);
>     return m_topLeft;
> }

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193375: 358974