Attachment 341250 Details for Bug 185972 – Patch

[patch] Patch

bug-185972-20180524215950.patch (text/plain), 18.22 KB, created by zalan on 2018-05-24 21:59:51 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2018-05-24 21:59:51 PDT

Size: 18.22 KB

patch

obsolete

>Subversion Revision: 232152
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d8ae730671004cdf1f166851563f6acd99af6bc5..86ec0602251177c6400f129eabd0847e315ab2dc 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,54 @@
>+2018-05-24  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC] Implement border and padding computation
>+        https://bugs.webkit.org/show_bug.cgi?id=185972
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch also removes redundant Display::Box methods and adds a lightweight Edge struct.
>+        (Since padding is optional, if during layout we mistakenly try to access paddingTop/Left/Bottom/Right, Display::Box will assert!)
>+
>+        * layout/FormattingContext.cpp:
>+        (WebCore::Layout::FormattingContext::computeBorderAndPadding const):
>+        * layout/FormattingContext.h:
>+        * layout/FormattingContextGeometry.cpp:
>+        (WebCore::Layout::FormattingContext::Geometry::computedBorder):
>+        (WebCore::Layout::FormattingContext::Geometry::computedPadding):
>+        * layout/blockformatting/BlockFormattingContext.cpp:
>+        (WebCore::Layout::BlockFormattingContext::layout const):
>+        * layout/displaytree/DisplayBox.cpp:
>+        (WebCore::Display::Box::marginBox const):
>+        (WebCore::Display::Box::paddingBox const):
>+        (WebCore::Display::Box::contentBox const):
>+        * layout/displaytree/DisplayBox.h:
>+        (WebCore::Display::Box::Edges::Edges):
>+        (WebCore::Display::Box::setHasValidPosition):
>+        (WebCore::Display::Box::setWidth):
>+        (WebCore::Display::Box::setHeight):
>+        (WebCore::Display::Box::setMargin):
>+        (WebCore::Display::Box::setBorder):
>+        (WebCore::Display::Box::setPadding):
>+        (WebCore::Display::Box::marginTop const):
>+        (WebCore::Display::Box::marginLeft const):
>+        (WebCore::Display::Box::marginBottom const):
>+        (WebCore::Display::Box::marginRight const):
>+        (WebCore::Display::Box::paddingTop const):
>+        (WebCore::Display::Box::paddingLeft const):
>+        (WebCore::Display::Box::paddingBottom const):
>+        (WebCore::Display::Box::paddingRight const):
>+        (WebCore::Display::Box::borderTop const):
>+        (WebCore::Display::Box::borderLeft const):
>+        (WebCore::Display::Box::borderBottom const):
>+        (WebCore::Display::Box::borderRight const):
>+        (WebCore::Display::Box::invalidateSize): Deleted.
>+        (WebCore::Display::Box::setHasValidSize): Deleted.
>+        (WebCore::Display::Box::setHasValidGeometry): Deleted.
>+        (WebCore::Display::Box::setRect): Deleted.
>+        (WebCore::Display::Box::setSize): Deleted.
>+        * layout/layouttree/LayoutBox.cpp:
>+        (WebCore::Layout::Box::isPaddingEnabled const):
>+        * layout/layouttree/LayoutBox.h:
>+
> 2018-05-24  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC] Implement FormattingContext::placeInFlowPositionedChildren
>diff --git a/Source/WebCore/layout/FormattingContext.cpp b/Source/WebCore/layout/FormattingContext.cpp
>index f9b9d2a1a966ec103a8003f008a31b7070e37193..65dd98aab142c231bbc7244709a2a31a00b5783f 100644
>--- a/Source/WebCore/layout/FormattingContext.cpp
>+++ b/Source/WebCore/layout/FormattingContext.cpp
>@@ -155,6 +155,13 @@ LayoutUnit FormattingContext::marginRight(const Box&) const
>     return 0;
> }
> 
>+void FormattingContext::computeBorderAndPadding(LayoutContext& layoutContext, const Box& layoutBox, Display::Box& displayBox) const
>+{
>+    displayBox.setBorder(Geometry::computedBorder(layoutContext, layoutBox));
>+    if (auto padding = Geometry::computedPadding(layoutContext, layoutBox))
>+        displayBox.setPadding(*padding);
>+}
>+
> void FormattingContext::placeInFlowPositionedChildren(LayoutContext& layoutContext, const Container& container) const
> {
>     // If this container also establishes a formatting context, then positioning already has happend in that the formatting context.
>diff --git a/Source/WebCore/layout/FormattingContext.h b/Source/WebCore/layout/FormattingContext.h
>index 296dd31684853e9b331ac33a11c8c95fc33cefd2..c5868579782fe38d54bedf0e2dfddbeaefda300a 100644
>--- a/Source/WebCore/layout/FormattingContext.h
>+++ b/Source/WebCore/layout/FormattingContext.h
>@@ -27,6 +27,7 @@
> 
> #if ENABLE(LAYOUT_FORMATTING_CONTEXT)
> 
>+#include "DisplayBox.h"
> #include "FloatingState.h"
> #include <wtf/IsoMalloc.h>
> #include <wtf/WeakPtr.h>
>@@ -36,10 +37,6 @@ namespace WebCore {
> class LayoutPoint;
> class LayoutUnit;
> 
>-namespace Display {
>-class Box;
>-}
>-
> namespace Layout {
> 
> class Box;
>@@ -86,6 +83,8 @@ protected:
>     virtual LayoutUnit marginBottom(const Box&) const;
>     virtual LayoutUnit marginRight(const Box&) const;
> 
>+    void computeBorderAndPadding(LayoutContext&, const Box&, Display::Box&) const;
>+
>     void placeInFlowPositionedChildren(LayoutContext&, const Container&) const;
>     void layoutOutOfFlowDescendants(LayoutContext&s) const;
> 
>@@ -113,6 +112,9 @@ protected:
> 
>         static LayoutUnit replacedHeight(LayoutContext&, const Box&);
>         static LayoutUnit replacedWidth(LayoutContext&, const Box&);
>+
>+        static Display::Box::Edges computedBorder(LayoutContext&, const Box&);
>+        static std::optional<Display::Box::Edges> computedPadding(LayoutContext&, const Box&);
>     };
> 
> private:
>diff --git a/Source/WebCore/layout/FormattingContextGeometry.cpp b/Source/WebCore/layout/FormattingContextGeometry.cpp
>index a464b38aa960f31e677f737c236ce79cb40f77e4..2f3c954fcfde3162714167d28fe41fd300c2203d 100644
>--- a/Source/WebCore/layout/FormattingContextGeometry.cpp
>+++ b/Source/WebCore/layout/FormattingContextGeometry.cpp
>@@ -525,6 +525,32 @@ LayoutUnit FormattingContext::Geometry::replacedWidth(LayoutContext&, const Box&
>     return computedWidthValue;
> }
> 
>+Display::Box::Edges FormattingContext::Geometry::computedBorder(LayoutContext&, const Box& layoutBox)
>+{
>+    auto& style = layoutBox.style();
>+    return {
>+        style.borderTop().width(),
>+        style.borderLeft().width(),
>+        style.borderBottom().width(),
>+        style.borderRight().width()
>+    };
>+}
>+
>+std::optional<Display::Box::Edges> FormattingContext::Geometry::computedPadding(LayoutContext& layoutContext, const Box& layoutBox)
>+{
>+    if (!layoutBox.isPaddingApplicable())
>+        return std::nullopt;
>+
>+    auto& style = layoutBox.style();
>+    auto containingBlockWidth = layoutContext.displayBoxForLayoutBox(*layoutBox.containingBlock())->width();
>+    return Display::Box::Edges(
>+        valueForLength(style.paddingTop(), containingBlockWidth),
>+        valueForLength(style.paddingLeft(), containingBlockWidth),
>+        valueForLength(style.paddingBottom(), containingBlockWidth),
>+        valueForLength(style.paddingRight(), containingBlockWidth)
>+    );
>+}
>+
> }
> }
> #endif
>diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>index e1f67e72692e36244104e3b00cd471d65acb1688..7de599f8b7cc1c6b2940b826d0fcc9392c11f5e4 100644
>--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp
>@@ -75,7 +75,8 @@ void BlockFormattingContext::layout(LayoutContext& layoutContext, FormattingStat
>             auto& displayBox = layoutPair.displayBox;
>             
>             computeWidth(layoutContext, layoutBox, displayBox);
>-            computeStaticPosition(layoutContext, layoutBox, layoutPair.displayBox);
>+            computeBorderAndPadding(layoutContext, layoutBox, displayBox);
>+            computeStaticPosition(layoutContext, layoutBox, displayBox);
>             if (layoutBox.establishesFormattingContext()) {
>                 auto formattingContext = layoutContext.formattingContext(layoutBox);
>                 formattingContext->layout(layoutContext, layoutContext.establishedFormattingState(layoutBox, *formattingContext));
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.cpp b/Source/WebCore/layout/displaytree/DisplayBox.cpp
>index f8bfdb4146176ee196d41ad0a886441e009cca38..8664e64cd646de0e8598dddbcd29a0f67ae3b08f 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.cpp
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.cpp
>@@ -56,10 +56,10 @@ LayoutRect Box::marginBox() const
>     ASSERT(m_hasValidMargin);
>     auto marginBox = borderBox();
> 
>-    marginBox.shiftXEdgeTo(marginBox.x() + m_marginLeft);
>-    marginBox.shiftYEdgeTo(marginBox.y() + m_marginTop);
>-    marginBox.shiftMaxXEdgeTo(marginBox.maxX() - m_marginRight);
>-    marginBox.shiftMaxYEdgeTo(marginBox.maxY() - m_marginBottom);
>+    marginBox.shiftXEdgeTo(marginBox.x() + m_margin.left);
>+    marginBox.shiftYEdgeTo(marginBox.y() + m_margin.top);
>+    marginBox.shiftMaxXEdgeTo(marginBox.maxX() - m_margin.right);
>+    marginBox.shiftMaxYEdgeTo(marginBox.maxY() - m_margin.bottom);
> 
>     return marginBox;
> }
>@@ -82,10 +82,10 @@ LayoutRect Box::paddingBox() const
>     ASSERT(m_hasValidBorder);
>     auto paddingBox = borderBox();
> 
>-    paddingBox.shiftXEdgeTo(paddingBox.x() + m_borderLeft);
>-    paddingBox.shiftYEdgeTo(paddingBox.y() + m_borderTop);
>-    paddingBox.shiftMaxXEdgeTo(paddingBox.maxX() - m_borderRight);
>-    paddingBox.shiftMaxYEdgeTo(paddingBox.maxY() - m_borderBottom);
>+    paddingBox.shiftXEdgeTo(paddingBox.x() + m_border.left);
>+    paddingBox.shiftYEdgeTo(paddingBox.y() + m_border.top);
>+    paddingBox.shiftMaxXEdgeTo(paddingBox.maxX() - m_border.right);
>+    paddingBox.shiftMaxYEdgeTo(paddingBox.maxY() - m_border.bottom);
> 
>     return paddingBox;
> }
>@@ -99,10 +99,10 @@ LayoutRect Box::contentBox() const
>     ASSERT(m_hasValidPadding);
>     auto contentBox = paddingBox();
> 
>-    contentBox.shiftXEdgeTo(contentBox.x() + m_paddingLeft);
>-    contentBox.shiftYEdgeTo(contentBox.y() + m_paddingTop);
>-    contentBox.shiftMaxXEdgeTo(contentBox.maxX() - m_paddingRight);
>-    contentBox.shiftMaxYEdgeTo(contentBox.maxY() - m_paddingBottom);
>+    contentBox.shiftXEdgeTo(contentBox.x() + m_padding.left);
>+    contentBox.shiftYEdgeTo(contentBox.y() + m_padding.top);
>+    contentBox.shiftMaxXEdgeTo(contentBox.maxX() - m_padding.right);
>+    contentBox.shiftMaxYEdgeTo(contentBox.maxY() - m_padding.bottom);
> 
>     return contentBox;
> }
>diff --git a/Source/WebCore/layout/displaytree/DisplayBox.h b/Source/WebCore/layout/displaytree/DisplayBox.h
>index afedfb5271303ef870ce5bcb8938fc6a7d580f6d..e56ec73fc16654580a68cf8566eecb851e2bd67a 100644
>--- a/Source/WebCore/layout/displaytree/DisplayBox.h
>+++ b/Source/WebCore/layout/displaytree/DisplayBox.h
>@@ -97,17 +97,29 @@ private:
>         BoxSizing boxSizing { BoxSizing::ContentBox };
>     };
> 
>-    void setRect(const LayoutRect&);
>     void setTopLeft(const LayoutPoint&);
>     void setTop(LayoutUnit);
>     void setLeft(LayoutUnit);
>-    void setSize(const LayoutSize&);
>     void setWidth(LayoutUnit);
>     void setHeight(LayoutUnit);
> 
>-    void setMargin(LayoutUnit marginTop, LayoutUnit marginLeft, LayoutUnit marginRight, LayoutUnit marginBottom);
>-    void setBorder(LayoutUnit borderTop, LayoutUnit borderLeft, LayoutUnit borderRight, LayoutUnit borderBottom);
>-    void setPadding(LayoutUnit paddingTop, LayoutUnit paddingLeft, LayoutUnit paddingRight, LayoutUnit paddingBottom);
>+    struct Edges {
>+        Edges() = default;
>+        Edges(LayoutUnit top, LayoutUnit left, LayoutUnit bottom, LayoutUnit right)
>+            : top(top)
>+            , left(left)
>+            , bottom(bottom)
>+            , right(right)
>+            { }
>+
>+        LayoutUnit top;
>+        LayoutUnit left;
>+        LayoutUnit bottom;
>+        LayoutUnit right;
>+    };
>+    void setMargin(Edges);
>+    void setBorder(Edges);
>+    void setPadding(Edges);
> 
> #if !ASSERT_DISABLED
>     void invalidateTop() { m_hasValidTop = false; }
>@@ -115,7 +127,6 @@ private:
>     void invalidateWidth() { m_hasValidWidth = false; }
>     void invalidateHeight() { m_hasValidHeight = false; }
>     void invalidatePosition();
>-    void invalidateSize();
>     void invalidateMargin() { m_hasValidMargin = false; }
>     void invalidateBorder() { m_hasValidBorder = false; }
>     void invalidatePadding() { m_hasValidPadding = false; }
>@@ -125,8 +136,6 @@ private:
>     bool hasValidGeometry() const { return hasValidPosition() && hasValidSize(); }
>     
>     void setHasValidPosition();
>-    void setHasValidSize();
>-    void setHasValidGeometry();
>     
>     void setHasValidMargin();
>     void setHasValidBorder();
>@@ -137,20 +146,9 @@ private:
> 
>     LayoutRect m_rect;
> 
>-    LayoutUnit m_marginTop;
>-    LayoutUnit m_marginLeft;
>-    LayoutUnit m_marginBottom;
>-    LayoutUnit m_marginRight;
>-
>-    LayoutUnit m_borderTop;
>-    LayoutUnit m_borderLeft;
>-    LayoutUnit m_borderBottom;
>-    LayoutUnit m_borderRight;
>-
>-    LayoutUnit m_paddingTop;
>-    LayoutUnit m_paddingLeft;
>-    LayoutUnit m_paddingBottom;
>-    LayoutUnit m_paddingRight;
>+    Edges m_margin;
>+    Edges m_border;
>+    Edges m_padding;
> 
> #if !ASSERT_DISABLED
>     bool m_hasValidTop { false };
>@@ -170,29 +168,11 @@ inline void Box::invalidatePosition()
>     invalidateLeft();
> }
> 
>-inline void Box::invalidateSize()
>-{
>-    invalidateWidth();
>-    invalidateHeight();
>-}
>-
> inline void Box::setHasValidPosition()
> {
>     m_hasValidTop = true;
>     m_hasValidLeft = true;
> }
>-
>-inline void Box::setHasValidSize()
>-{
>-    m_hasValidWidth = true;
>-    m_hasValidHeight = true;
>-}
>-
>-inline void Box::setHasValidGeometry()
>-{
>-    setHasValidPosition();
>-    setHasValidSize();
>-}
> #endif
> 
> inline LayoutRect Box::rect() const
>@@ -255,14 +235,6 @@ inline LayoutUnit Box::height() const
>     return m_rect.height();
> }
> 
>-inline void Box::setRect(const LayoutRect& rect)
>-{
>-#if !ASSERT_DISABLED
>-    setHasValidGeometry();
>-#endif
>-    m_rect = rect;
>-}
>-
> inline void Box::setTopLeft(const LayoutPoint& topLeft)
> {
> #if !ASSERT_DISABLED
>@@ -287,19 +259,12 @@ inline void Box::setLeft(LayoutUnit left)
>     m_rect.setX(left);
> }
> 
>-inline void Box::setSize(const LayoutSize& size)
>-{
>-#if !ASSERT_DISABLED
>-    setHasValidSize();
>-#endif
>-    m_rect.setSize(size);
>-}
>-
> inline void Box::setWidth(LayoutUnit width)
> {
> #if !ASSERT_DISABLED
>     m_hasValidWidth = true;
> #endif
>+    ASSERT(m_hasValidLeft);
>     m_rect.setWidth(width);
> }
> 
>@@ -308,112 +273,104 @@ inline void Box::setHeight(LayoutUnit height)
> #if !ASSERT_DISABLED
>     m_hasValidHeight = true;
> #endif
>+    ASSERT(m_hasValidTop);
>     m_rect.setHeight(height);
> }
> 
>-inline void Box::setMargin(LayoutUnit marginTop, LayoutUnit marginLeft, LayoutUnit marginRight, LayoutUnit marginBottom)
>+inline void Box::setMargin(Edges margin)
> {
> #if !ASSERT_DISABLED
>     void setHasValidMargin();
> #endif
>-    m_marginTop = marginTop;
>-    m_marginLeft = marginLeft;
>-    m_marginBottom = marginBottom;
>-    m_marginRight = marginRight;
>+    m_margin = margin;
> }
> 
>-inline void Box::setBorder(LayoutUnit borderTop, LayoutUnit borderLeft, LayoutUnit borderRight, LayoutUnit borderBottom)
>+inline void Box::setBorder(Edges border)
> {
> #if !ASSERT_DISABLED
>     void setHasValidBorder();
> #endif
>-    m_borderTop = borderTop;
>-    m_borderLeft = borderLeft;
>-    m_borderBottom = borderBottom;
>-    m_borderRight = borderRight;
>+    m_border = border;
> }
> 
>-inline void Box::setPadding(LayoutUnit paddingTop, LayoutUnit paddingLeft, LayoutUnit paddingRight, LayoutUnit paddingBottom)
>+inline void Box::setPadding(Edges padding)
> {
> #if !ASSERT_DISABLED
>     void setHasValidPadding();
> #endif
>-    m_paddingTop = paddingTop;
>-    m_paddingLeft = paddingLeft;
>-    m_paddingBottom = paddingBottom;
>-    m_paddingRight = paddingRight;
>+    m_padding = padding;
> }
> 
> inline LayoutUnit Box::marginTop() const
> {
>     ASSERT(m_hasValidMargin);
>-    return m_marginTop;
>+    return m_margin.top;
> }
> 
> inline LayoutUnit Box::marginLeft() const
> {
>     ASSERT(m_hasValidMargin);
>-    return m_marginLeft;
>+    return m_margin.left;
> }
> 
> inline LayoutUnit Box::marginBottom() const
> {
>     ASSERT(m_hasValidMargin);
>-    return m_marginBottom;
>+    return m_margin.bottom;
> }
> 
> inline LayoutUnit Box::marginRight() const
> {
>     ASSERT(m_hasValidMargin);
>-    return m_marginRight;
>+    return m_margin.right;
> }
> 
> inline LayoutUnit Box::paddingTop() const
> {
>     ASSERT(m_hasValidPadding);
>-    return m_paddingTop;
>+    return m_padding.top;
> }
> 
> inline LayoutUnit Box::paddingLeft() const
> {
>     ASSERT(m_hasValidPadding);
>-    return m_paddingLeft;
>+    return m_padding.left;
> }
> 
> inline LayoutUnit Box::paddingBottom() const
> {
>     ASSERT(m_hasValidPadding);
>-    return m_paddingBottom;
>+    return m_padding.bottom;
> }
> 
> inline LayoutUnit Box::paddingRight() const
> {
>     ASSERT(m_hasValidPadding);
>-    return m_paddingRight;
>+    return m_padding.right;
> }
> 
> inline LayoutUnit Box::borderTop() const
> {
>     ASSERT(m_hasValidBorder);
>-    return m_borderTop;
>+    return m_border.top;
> }
> 
> inline LayoutUnit Box::borderLeft() const
> {
>     ASSERT(m_hasValidBorder);
>-    return m_borderLeft;
>+    return m_border.left;
> }
> 
> inline LayoutUnit Box::borderBottom() const
> {
>     ASSERT(m_hasValidBorder);
>-    return m_borderBottom;
>+    return m_border.bottom;
> }
> 
> inline LayoutUnit Box::borderRight() const
> {
>     ASSERT(m_hasValidBorder);
>-    return m_borderRight;
>+    return m_border.right;
> }
> 
> }
>diff --git a/Source/WebCore/layout/layouttree/LayoutBox.cpp b/Source/WebCore/layout/layouttree/LayoutBox.cpp
>index dd503610dc00ceace70f3db50325d24948900708..91512c25b771ec9e93a2d096f5018f4457ba4ec5 100644
>--- a/Source/WebCore/layout/layouttree/LayoutBox.cpp
>+++ b/Source/WebCore/layout/layouttree/LayoutBox.cpp
>@@ -226,6 +226,13 @@ bool Box::isOverflowVisible() const
>     return m_style.overflowX() == Overflow::Visible || m_style.overflowY() == Overflow::Visible;
> }
> 
>+bool Box::isPaddingApplicable() const
>+{
>+    // 8.4 Padding properties:
>+    // Applies to: all elements except table-row-group, table-header-group, table-footer-group, table-row, table-column-group and table-column
>+    return true;
>+}
>+
> }
> }
> 
>diff --git a/Source/WebCore/layout/layouttree/LayoutBox.h b/Source/WebCore/layout/layouttree/LayoutBox.h
>index 10f60abffe0ea31ea71a410b4e2ff9f250493288..1e0b85ffde91368b6982ce116c82c13a1c29d21e 100644
>--- a/Source/WebCore/layout/layouttree/LayoutBox.h
>+++ b/Source/WebCore/layout/layouttree/LayoutBox.h
>@@ -88,6 +88,8 @@ public:
>     bool isInlineBox() const { return m_baseTypeFlags & InlineBoxFlag; }
>     bool isInlineContainer() const { return m_baseTypeFlags & InlineContainerFlag; }
> 
>+    bool isPaddingApplicable() const;
>+
>     const RenderStyle& style() const { return m_style; }
>     auto& weakPtrFactory() const { return m_weakFactory; }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 185972: 341250 | 341260