Attachment 347224 Details for Bug 188619 – Patch

[patch] Patch

bug-188619-20180815164928.patch (text/plain), 16.73 KB, created by zalan on 2018-08-15 16:49:30 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2018-08-15 16:49:30 PDT

Size: 16.73 KB

patch

obsolete

>Subversion Revision: 234903
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 45f39ffa7cb52927e6309e931190956946750dd3..ad47633a522c4708a2cd7076402c3d0f0e1663e3 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,48 @@
>+2018-08-15  Zalan Bujtas  <zalan@apple.com>
>+
>+        [lFC][Floating] Add estimated margin top computation.
>+        https://bugs.webkit.org/show_bug.cgi?id=188619
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In order to figure out whether a box should avoid a float, we need to know the final positions of both (ignore relative positioning for now).
>+        In block formatting context the final position for a normal flow box includes
>+        1. the static position and 
>+        2. the corresponding (non)collapsed margins.
>+        Now the vertical margins are computed when all the descendants are finalized, because the margin values might be depending on the height of the box
>+        (and the height might be based on the content).
>+        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.
>+        (if this holds true for all the cases, the estimated prefix could be removed and just use marginTop() instead.)
>+
>+        Covered by existing block-only tests.
>+
>+        * layout/blockformatting/BlockFormattingContext.cpp:
>+        (WebCore::Layout::BlockFormattingContext::layout const):
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginTop const):
>+        (WebCore::Layout::BlockFormattingContext::computeEstimatedMarginTopForAncestors const):
>+        (WebCore::Layout::BlockFormattingContext::computeFloatingPosition const):
>+        (WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const):
>+        (WebCore::Layout::BlockFormattingContext::computeInFlowHeightAndMargin const):
>+        * layout/blockformatting/BlockFormattingContext.h:
>+        * layout/blockformatting/BlockMarginCollapse.cpp:
>+        (WebCore::Layout::BlockFormattingContext::MarginCollapse::estimatedMarginTop):
>+        * layout/displaytree/DisplayBox.cpp:
>+        (WebCore::Display::Box::Box):
>+        * layout/displaytree/DisplayBox.h:
>+        (WebCore::Display::Box::setHasValidTop):
>+        (WebCore::Display::Box::setHasValidLeft):
>+        (WebCore::Display::Box::top const):
>+        (WebCore::Display::Box::left const):
>+        (WebCore::Display::Box::topLeft const):
>+        (WebCore::Display::Box::setTopLeft):
>+        (WebCore::Display::Box::setTop):
>+        (WebCore::Display::Box::setLeft):
>+        (WebCore::Display::Box::setVerticalMargin):
>+        (WebCore::Display::Box::setEstimatedMarginTop):
>+        (WebCore::Display::Box::estimatedMarginTop const):
>+
> 2018-08-15  Christopher Reid  <chris.reid@sony.com>
> 
>         [Curl] Implement default cookie path handling correctly as outlined in RFC6265.
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index c61e55482bccc02c5ab0d3d3a8a173b08049ea87..faded22c052d80d7c7597775e5e384268f00220e 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -113,7 +113,7 @@ void BlockFormattingContext::layout(LayoutContext& layoutContext, FormattingStat
>             computeHeightAndMargin(layoutContext, layoutBox, displayBox);
>             // Finalize position with clearance.
>             if (layoutBox.hasFloatClear())
>-                computeVerticalPositionForFloatClear(floatingContext, layoutBox, displayBox);
>+                computeVerticalPositionForFloatClear(layoutContext, floatingContext, layoutBox, displayBox);
>             if (!is<Container>(layoutBox))
>                 continue;
>             auto& container = downcast<Container>(layoutBox);
>@@ -156,6 +156,39 @@ void BlockFormattingContext::computeStaticPosition(LayoutContext& layoutContext,
>     displayBox.setTopLeft(Geometry::staticPosition(layoutContext, layoutBox));
> }
> 
>+void BlockFormattingContext::computeEstimatedMarginTop(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
>+{
>+    auto estimatedMarginTop = MarginCollapse::estimatedMarginTop(layoutContext, layoutBox);
>+    displayBox.setEstimatedMarginTop(estimatedMarginTop);
>+    displayBox.moveVertically(estimatedMarginTop);
>+}
>+
>+void BlockFormattingContext::computeEstimatedMarginTopForAncestors(LayoutContext& layoutContext, const Box& layoutBox) const
>+{
>+    // We only need to estimate margin top for float related layout.
>+    ASSERT(layoutBox.isFloatingPositioned() || layoutBox.hasFloatClear());
>+
>+    // In order to figure out whether a box should avoid a float, we need to know the final positions of both (ignore relative positioning for now).
>+    // In block formatting context the final position for a normal flow box includes
>+    // 1. the static position and 
>+    // 2. the corresponding (non)collapsed margins.
>+    // Now the vertical margins are computed when all the descendants are finalized, because the margin values might be depending on the height of the box
>+    // (and the height might be based on the content).
>+    // 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.
>+
>+    for (auto* ancestor = layoutBox.containingBlock(); ancestor && !ancestor->establishesBlockFormattingContext(); ancestor = ancestor->containingBlock()) {
>+        auto* displayBox = layoutContext.displayBoxForLayoutBox(*ancestor);
>+        ASSERT(displayBox);
>+        // FIXME: with incremental layout, we might actually have a valid (non-estimated) margin top as well.
>+        if (displayBox->estimatedMarginTop())
>+            return;
>+
>+        computeEstimatedMarginTop(layoutContext, *ancestor, *displayBox);
>+    }
>+}
>+
> void BlockFormattingContext::computeFloatingPosition(LayoutContext& layoutContext, FloatingContext& floatingContext, const Box& layoutBox, Display::Box& displayBox) const
> {
>     ASSERT(layoutBox.isFloatingPositioned());
>@@ -166,13 +199,18 @@ void BlockFormattingContext::computeFloatingPosition(LayoutContext& layoutContex
>         auto& previousDisplayBox = *layoutContext.displayBoxForLayoutBox(*previousInFlowBox);
>         displayBox.moveVertically(previousDisplayBox.nonCollapsedMarginBottom() - previousDisplayBox.marginBottom());
>     }
>+    computeEstimatedMarginTopForAncestors(layoutContext, layoutBox);
>     displayBox.setTopLeft(floatingContext.positionForFloat(layoutBox));
>     floatingContext.floatingState().append(layoutBox);
> }
> 
>-void BlockFormattingContext::computeVerticalPositionForFloatClear(const FloatingContext& floatingContext, const Box& layoutBox, Display::Box& displayBox) const
>+void BlockFormattingContext::computeVerticalPositionForFloatClear(LayoutContext& layoutContext, const FloatingContext& floatingContext, const Box& layoutBox, Display::Box& displayBox) const
> {
>     ASSERT(layoutBox.hasFloatClear());
>+    if (floatingContext.floatingState().isEmpty())
>+        return;
>+
>+    computeEstimatedMarginTopForAncestors(layoutContext, layoutBox);
>     if (auto verticalPositionWithClearance = floatingContext.verticalPositionWithClearance(layoutBox))
>         displayBox.setTop(*verticalPositionWithClearance);
> }
>@@ -208,9 +246,13 @@ void BlockFormattingContext::computeInFlowHeightAndMargin(LayoutContext& layoutC
> {
>     auto heightAndMargin = Geometry::inFlowHeightAndMargin(layoutContext, layoutBox);
>     displayBox.setContentBoxHeight(heightAndMargin.height);
>-    displayBox.moveVertically(heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin).top);
>-    displayBox.setVerticalMargin(heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin));
>+    auto marginValue = heightAndMargin.collapsedMargin.value_or(heightAndMargin.margin);
>+    displayBox.setVerticalMargin(marginValue);
>     displayBox.setVerticalNonCollapsedMargin(heightAndMargin.margin);
>+
>+    // This box has already been moved by the estimated vertical margin. No need to move it again.
>+    if (!displayBox.estimatedMarginTop())
>+        displayBox.moveVertically(marginValue.top);
> }
> 
> void BlockFormattingContext::computeInFlowWidthAndMargin(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>index 3247ed568132f9f9642a738acfeddb65064b2e48..e8687a3a44daf3cf356d48cead32d1762068029a 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h
>@@ -57,10 +57,12 @@ private:
> 
>     void computeStaticPosition(LayoutContext&, const Box&, Display::Box&) const override;
>     void computeFloatingPosition(LayoutContext&, FloatingContext&, const Box&, Display::Box&) const;
>-    void computeVerticalPositionForFloatClear(const FloatingContext&, const Box&, Display::Box&) const;
>+    void computeVerticalPositionForFloatClear(LayoutContext&, const FloatingContext&, const Box&, Display::Box&) const;
>     void computeInFlowPositionedPosition(LayoutContext&, const Box&, Display::Box&) const override;
>     void computeInFlowWidthAndMargin(LayoutContext&, const Box&, Display::Box&) const;
>     void computeInFlowHeightAndMargin(LayoutContext&, const Box&, Display::Box&) const;
>+    void computeEstimatedMarginTop(LayoutContext&, const Box&, Display::Box&) const;
>+    void computeEstimatedMarginTopForAncestors(LayoutContext&, const Box&) const;
> 
>     FormattingContext::InstrinsicWidthConstraints instrinsicWidthConstraints(LayoutContext&, const Box&) const override;
> 
>@@ -87,6 +89,7 @@ private:
>     class MarginCollapse {
>     public:
>         static LayoutUnit marginTop(const LayoutContext&, const Box&);
>+        static LayoutUnit estimatedMarginTop(const LayoutContext&, const Box&);
>         static LayoutUnit marginBottom(const LayoutContext&, const Box&);
> 
>         static bool isMarginBottomCollapsedWithParent(const LayoutContext&, const Box&);
>diff --git a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>index 8610f1bb2ee36cbc66e756cc9a1b769eab45f1d3..f57bff07e435666712695e50f57c1812c17cb243 100644
>--- a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp
>@@ -332,6 +332,18 @@ LayoutUnit BlockFormattingContext::MarginCollapse::nonCollapsedMarginBottom(cons
>     return marginValue(computedNonCollapsedMarginBottom(layoutContext, layoutBox), collapsedMarginBottomFromLastChild(layoutContext, layoutBox));
> }
> 
>+LayoutUnit BlockFormattingContext::MarginCollapse::estimatedMarginTop(const LayoutContext& layoutContext, const Box& layoutBox)
>+{
>+    ASSERT(layoutBox.isBlockLevelBox());
>+    // Can't estimate vertical margins for out of flow boxes (and we shouldn't need to do it for float boxes).
>+    ASSERT(layoutBox.isInFlow());
>+    // Can't cross block formatting context boundary. 
>+    ASSERT(!layoutBox.establishesBlockFormattingContext());
>+
>+    // Let's just use the normal path for now.
>+    return marginTop(layoutContext, layoutBox);
>+}
>+
> }
> }
> #endif
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.cpp b/Source/WebCore/layout/displaytree/DisplayBox.cpp
>index 98afcc50605dcf33afeb904b6beff768c4761942..718b16eaf5023dc4b10e919aa3dc470f270a998a 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.cpp
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.cpp
>@@ -59,9 +59,12 @@ Box::Box(const Box& other)
>     , m_contentHeight(other.m_contentHeight)
>     , m_margin(other.m_margin)
>     , m_verticalNonCollapsedMargin(other.m_verticalNonCollapsedMargin)
>+    , m_estimatedMarginTop(other.m_estimatedMarginTop)
>     , m_border(other.m_border)
>     , m_padding(other.m_padding)
> #if !ASSERT_DISABLED
>+    , m_hasValidTop(other.m_hasValidTop)
>+    , m_hasValidLeft(other.m_hasValidLeft)
>     , m_hasValidHorizontalMargin(other.m_hasValidHorizontalMargin)
>     , m_hasValidVerticalMargin(other.m_hasValidVerticalMargin)
>     , m_hasValidVerticalNonCollapsedMargin(other.m_hasValidVerticalNonCollapsedMargin)
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.h b/Source/WebCore/layout/displaytree/DisplayBox.h
>index fd096d3f33214df1632670dd205fff094378a14e..da53ae2a71de866825cd0e804375349d668e0ce8 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.h
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.h
>@@ -117,12 +117,12 @@ public:
> 
>     ~Box();
> 
>-    LayoutUnit top() const { return m_topLeft.y(); }
>-    LayoutUnit left() const { return m_topLeft.x(); }
>+    LayoutUnit top() const;
>+    LayoutUnit left() const;
>     LayoutUnit bottom() const { return top() + height(); }
>     LayoutUnit right() const { return left() + width(); }
> 
>-    LayoutPoint topLeft() const { return m_topLeft; }
>+    LayoutPoint topLeft() const;
>     LayoutPoint bottomRight() const { return { right(), bottom() }; }
> 
>     LayoutSize size() const { return { width(), height() }; }
>@@ -138,6 +138,7 @@ public:
> 
>     LayoutUnit nonCollapsedMarginTop() const;
>     LayoutUnit nonCollapsedMarginBottom() const;
>+    std::optional<LayoutUnit> estimatedMarginTop() const { return m_estimatedMarginTop; }
> 
>     LayoutUnit borderTop() const;
>     LayoutUnit borderLeft() const;
>@@ -172,9 +173,9 @@ private:
>         BoxSizing boxSizing { BoxSizing::ContentBox };
>     };
> 
>-    void setTopLeft(const LayoutPoint& topLeft) { m_topLeft = topLeft; }
>-    void setTop(LayoutUnit top) { m_topLeft.setY(top); }
>-    void setLeft(LayoutUnit left) { m_topLeft.setX(left); }
>+    void setTopLeft(const LayoutPoint&);
>+    void setTop(LayoutUnit);
>+    void setLeft(LayoutUnit);
>     void moveHorizontally(LayoutUnit offset) { m_topLeft.move(offset, { }); }
>     void moveVertically(LayoutUnit offset) { m_topLeft.move({ }, offset); }
> 
>@@ -184,6 +185,7 @@ private:
>     void setHorizontalMargin(Layout::HorizontalEdges);
>     void setVerticalMargin(Layout::VerticalEdges);
>     void setVerticalNonCollapsedMargin(Layout::VerticalEdges);
>+    void setEstimatedMarginTop(LayoutUnit marginTop) { m_estimatedMarginTop = marginTop; }
> 
>     void setBorder(Layout::Edges);
>     void setPadding(Layout::Edges);
>@@ -193,6 +195,8 @@ private:
>     void invalidateBorder() { m_hasValidBorder = false; }
>     void invalidatePadding() { m_hasValidPadding = false; }
> 
>+    void setHasValidTop() { m_hasValidTop = true; }
>+    void setHasValidLeft() { m_hasValidLeft = true; }
>     void setHasValidVerticalMargin() { m_hasValidVerticalMargin = true; }
>     void setHasValidVerticalNonCollapsedMargin() { m_hasValidVerticalNonCollapsedMargin = true; }
>     void setHasValidHorizontalMargin() { m_hasValidHorizontalMargin = true; }
>@@ -212,11 +216,14 @@ private:
> 
>     Layout::Edges m_margin;
>     Layout::VerticalEdges m_verticalNonCollapsedMargin;
>+    std::optional<LayoutUnit> m_estimatedMarginTop;
> 
>     Layout::Edges m_border;
>     Layout::Edges m_padding;
> 
> #if !ASSERT_DISABLED
>+    bool m_hasValidTop { false };
>+    bool m_hasValidLeft { false };
>     bool m_hasValidHorizontalMargin { false };
>     bool m_hasValidVerticalMargin { false };
>     bool m_hasValidVerticalNonCollapsedMargin { false };
>@@ -416,6 +423,50 @@ inline Box::Rect::operator LayoutRect() const
>     return m_rect;
> }
> 
>+inline LayoutUnit Box::top() const
>+{
>+    ASSERT(m_hasValidTop && (m_estimatedMarginTop || m_hasValidVerticalMargin));
>+    return m_topLeft.y(); 
>+}
>+
>+inline LayoutUnit Box::left() const
>+{
>+    ASSERT(m_hasValidLeft && m_hasValidHorizontalMargin);
>+    return m_topLeft.x();
>+}
>+
>+inline LayoutPoint Box::topLeft() const
>+{
>+    ASSERT(m_hasValidTop && (m_estimatedMarginTop || m_hasValidVerticalMargin));
>+    ASSERT(m_hasValidLeft && m_hasValidHorizontalMargin);
>+    return m_topLeft;
>+}
>+
>+inline void Box::setTopLeft(const LayoutPoint& topLeft)
>+{ 
>+#if !ASSERT_DISABLED
>+    setHasValidTop();
>+    setHasValidLeft();
>+#endif
>+    m_topLeft = topLeft;
>+}
>+
>+inline void Box::setTop(LayoutUnit top)
>+{
>+#if !ASSERT_DISABLED
>+    setHasValidTop();
>+#endif
>+    m_topLeft.setY(top);
>+}
>+
>+inline void Box::setLeft(LayoutUnit left)
>+{
>+#if !ASSERT_DISABLED
>+    setHasValidLeft();
>+#endif
>+    m_topLeft.setX(left);
>+}
>+
> inline void Box::setContentBoxHeight(LayoutUnit height)
> { 
> #if !ASSERT_DISABLED
>@@ -457,6 +508,7 @@ inline void Box::setVerticalMargin(Layout::VerticalEdges margin)
> #if !ASSERT_DISABLED
>     setHasValidVerticalMargin();
> #endif
>+    ASSERT(!m_estimatedMarginTop || *m_estimatedMarginTop == margin.top);
>     m_margin.vertical = margin;
> }
>

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188619: 347211 | 347224