Attachment 346293 Details for Bug 188232 – Patch

[patch] Patch

bug-188232-20180801133023.patch (text/plain), 13.57 KB, created by zalan on 2018-08-01 13:30:23 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2018-08-01 13:30:23 PDT

Size: 13.57 KB

patch

obsolete

>Subversion Revision: 234426
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 74e5096c249e3945d35870424c43470424521202..85f1869f468614e6ec8677f4172f5f531899fe61 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2018-08-01  Zalan Bujtas  <zalan@apple.com>
>+
>+        [LFC][Floating] Revert back to only one list for the all the floatings.
>+        https://bugs.webkit.org/show_bug.cgi?id=188232
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If the combined floating list turns out to be a performance bottleneck, we can still split it into left and right. However at this point
>+        having 2 dedicated lists just makes the implementation more complicated.
>+
>+        * layout/FloatingContext.cpp:
>+        (WebCore::Layout::begin):
>+        (WebCore::Layout::end):
>+        (WebCore::Layout::FloatingPair::FloatingPair):
>+        (WebCore::Layout::FloatingPair::left const):
>+        (WebCore::Layout::FloatingPair::right const):
>+        (WebCore::Layout::Iterator::Iterator):
>+        (WebCore::Layout::previousFloatingIndex):
>+        (WebCore::Layout::Iterator::operator++):
>+        (WebCore::Layout::Iterator::set):
>+        (WebCore::Layout::floatingDisplayBox): Deleted.
>+        * layout/FloatingState.cpp:
>+        (WebCore::Layout::FloatingState::remove):
>+        (WebCore::Layout::FloatingState::append):
>+        * layout/FloatingState.h:
>+        (WebCore::Layout::FloatingState::isEmpty const):
>+        (WebCore::Layout::FloatingState::floatings const):
>+        (WebCore::Layout::FloatingState::last const):
>+
> 2018-07-31  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][Floating] Add basic left/right floating positioning.
>diff --git a/Source/WebCore/layout/FloatingContext.cpp b/Source/WebCore/layout/FloatingContext.cpp
>index 1859d92395f44cf2bef2a4a071b7802a5a891268..5127b005b27fec8385e387e43756fc12ec111668 100644
>--- a/Source/WebCore/layout/FloatingContext.cpp
>+++ b/Source/WebCore/layout/FloatingContext.cpp
>@@ -59,8 +59,6 @@ class Iterator;
> 
> class FloatingPair {
> public:
>-    FloatingPair(const LayoutContext&, const FloatingState&);
>-
>     bool isEmpty() const { return !m_leftIndex && !m_rightIndex; }
>     const Display::Box* left() const;
>     const Display::Box* right() const;
>@@ -70,8 +68,10 @@ public:
> 
> private:
>     friend class Iterator;
>+    FloatingPair(const LayoutContext&, const FloatingState::FloatingList&);
>+
>     const LayoutContext& m_layoutContext;
>-    const FloatingState& m_floatingState;
>+    const FloatingState::FloatingList& m_floatings;
> 
>     std::optional<unsigned> m_leftIndex;
>     std::optional<unsigned> m_rightIndex;
>@@ -80,7 +80,7 @@ private:
> 
> class Iterator {
> public:
>-    Iterator(const LayoutContext&, const FloatingState&, std::optional<LayoutUnit> verticalPosition);
>+    Iterator(const LayoutContext&, const FloatingState::FloatingList&, std::optional<LayoutUnit> verticalPosition);
> 
>     const FloatingPair& operator*() const { return m_current; }
>     Iterator& operator++();
>@@ -91,19 +91,19 @@ private:
>     void set(LayoutUnit verticalPosition);
> 
>     const LayoutContext& m_layoutContext;
>-    const FloatingState& m_floatingState;
>+    const FloatingState::FloatingList& m_floatings;
>     FloatingPair m_current;
> };
> 
> static Iterator begin(const LayoutContext& layoutContext, const FloatingState& floatingState, LayoutUnit initialVerticalPosition)
> {
>     // Start with the inner-most floating pair for the initial vertical position.
>-    return Iterator(layoutContext, floatingState, initialVerticalPosition);
>+    return Iterator(layoutContext, floatingState.floatings(), initialVerticalPosition);
> }
> 
> static Iterator end(const LayoutContext& layoutContext, const FloatingState& floatingState)
> {
>-    return Iterator(layoutContext, floatingState, std::nullopt);
>+    return Iterator(layoutContext, floatingState.floatings(), std::nullopt);
> }
> 
> FloatingContext::FloatingContext(const Container& formattingContextRoot, FloatingState& floatingState)
>@@ -208,15 +208,9 @@ LayoutUnit FloatingContext::alignWithFloatings(const FloatingPair& floatingPair,
>     return containingBlockContentRight - marginBoxWidth;
> }
> 
>-static const Display::Box* floatingDisplayBox(unsigned index, const FloatingState::FloatingList& floatings, const LayoutContext& layoutContext)
>-{
>-    RELEASE_ASSERT(index < floatings.size());
>-    return layoutContext.displayBoxForLayoutBox(*floatings[index]);
>-}
>-
>-FloatingPair::FloatingPair(const LayoutContext& layoutContext, const FloatingState& floatingState)
>+FloatingPair::FloatingPair(const LayoutContext& layoutContext, const FloatingState::FloatingList& floatings)
>     : m_layoutContext(layoutContext)
>-    , m_floatingState(floatingState)
>+    , m_floatings(floatings)
> {
> }
> 
>@@ -224,8 +218,9 @@ const Display::Box* FloatingPair::left() const
> {
>     if (!m_leftIndex)
>         return nullptr;
>-
>-    return floatingDisplayBox(*m_leftIndex, m_floatingState.floatings().left, m_layoutContext);
>+    
>+    ASSERT(m_floatings[*m_leftIndex]->isLeftFloatingPositioned());
>+    return m_layoutContext.displayBoxForLayoutBox(*m_floatings[*m_leftIndex]);
> }
> 
> const Display::Box* FloatingPair::right() const
>@@ -233,7 +228,8 @@ const Display::Box* FloatingPair::right() const
>     if (!m_rightIndex)
>         return nullptr;
> 
>-    return floatingDisplayBox(*m_rightIndex, m_floatingState.floatings().right, m_layoutContext);
>+    ASSERT(m_floatings[*m_rightIndex]->isRightFloatingPositioned());
>+    return m_layoutContext.displayBoxForLayoutBox(*m_floatings[*m_rightIndex]);
> }
> 
> bool FloatingPair::intersects(const Display::Box::Rect& rect) const
>@@ -264,15 +260,28 @@ bool FloatingPair::operator ==(const FloatingPair& other) const
>     return m_leftIndex == other.m_leftIndex && m_rightIndex == other.m_rightIndex;
> }
> 
>-Iterator::Iterator(const LayoutContext& layoutContext, const FloatingState& floatingState, std::optional<LayoutUnit> verticalPosition)
>+Iterator::Iterator(const LayoutContext& layoutContext, const FloatingState::FloatingList& floatings, std::optional<LayoutUnit> verticalPosition)
>     : m_layoutContext(layoutContext)
>-    , m_floatingState(floatingState)
>-    , m_current(layoutContext, floatingState)
>+    , m_floatings(floatings)
>+    , m_current(layoutContext, floatings)
> {
>     if (verticalPosition)
>         set(*verticalPosition);
> }
> 
>+inline static std::optional<unsigned> previousFloatingIndex(Float floatingType, const FloatingState::FloatingList& floatings, unsigned currentIndex)
>+{
>+    RELEASE_ASSERT(currentIndex <= floatings.size());
>+    
>+    while (currentIndex) {
>+        auto* floating = floatings[--currentIndex].get();
>+        if ((floatingType == Float::Left && floating->isLeftFloatingPositioned()) || (floatingType == Float::Right && floating->isRightFloatingPositioned()))
>+            return currentIndex;
>+    }
>+
>+    return { };
>+}
>+
> Iterator& Iterator::operator++()
> {
>     if (m_current.isEmpty()) {
>@@ -280,19 +289,24 @@ Iterator& Iterator::operator++()
>         return *this;
>     }
> 
>-    auto previousFloatingIndex = [&](const FloatingState::FloatingList& floatings, unsigned index) -> std::optional<unsigned> {
>+    auto findPreviousFloatingWithLowerBottom = [&](Float floatingType, unsigned currentIndex) -> std::optional<unsigned> {
> 
>-        RELEASE_ASSERT(index < floatings.size());
>+        RELEASE_ASSERT(currentIndex < m_floatings.size());
> 
>-        if (!index)
>+        // Last floating? There's certainly no previous floating at this point.
>+        if (!currentIndex)
>             return { };
> 
>-        auto currentBottom = floatingDisplayBox(index--, floatings, m_layoutContext)->bottom();
>+        auto currentBottom = m_layoutContext.displayBoxForLayoutBox(*m_floatings[currentIndex])->bottom();
>+
>+        std::optional<unsigned> index = currentIndex;
>         while (true) {
>-            if (floatingDisplayBox(index, floatings, m_layoutContext)->bottom() > currentBottom)
>-                return index;
>-            if (!index--)
>+            index = previousFloatingIndex(floatingType, m_floatings, *index);
>+            if (!index)
>                 return { };
>+
>+            if (m_layoutContext.displayBoxForLayoutBox(*m_floatings[*index])->bottom() > currentBottom)
>+                return index;
>         }
> 
>         ASSERT_NOT_REACHED();
>@@ -314,13 +328,13 @@ Iterator& Iterator::operator++()
>     if (updateLeft) {
>         ASSERT(m_current.m_leftIndex);
>         m_current.m_verticalPosition = *leftBottom;
>-        m_current.m_leftIndex = previousFloatingIndex(m_floatingState.floatings().left, *m_current.m_leftIndex);
>+        m_current.m_leftIndex = findPreviousFloatingWithLowerBottom(Float::Left, *m_current.m_leftIndex);
>     }
>     
>     if (updateRight) {
>         ASSERT(m_current.m_rightIndex);
>         m_current.m_verticalPosition = *rightBottom;
>-        m_current.m_rightIndex = previousFloatingIndex(m_floatingState.floatings().right, *m_current.m_rightIndex);
>+        m_current.m_rightIndex = findPreviousFloatingWithLowerBottom(Float::Right, *m_current.m_rightIndex);
>     }
> 
>     return *this;
>@@ -335,7 +349,7 @@ void Iterator::set(LayoutUnit verticalPosition)
> 
>     m_current.m_verticalPosition = verticalPosition;
>     // No floatings at all?
>-    if (m_floatingState.isEmpty()) {
>+    if (m_floatings.isEmpty()) {
>         ASSERT_NOT_REACHED();
> 
>         m_current.m_leftIndex = { };
>@@ -343,21 +357,32 @@ void Iterator::set(LayoutUnit verticalPosition)
>         return;
>     }
> 
>-    auto findFloatingBelow = [&](const FloatingState::FloatingList& list) -> std::optional<unsigned> {
>-        
>-        auto index = list.size(); 
>-        while (index) {
>-            auto bottom = floatingDisplayBox(--index, list, m_layoutContext)->bottom();
>+    auto findFloatingBelow = [&](Float floatingType) -> std::optional<unsigned> {
>+
>+        ASSERT(!m_floatings.isEmpty());
>+
>+        auto index = floatingType == Float::Left ? m_current.m_leftIndex : m_current.m_rightIndex;
>+        // Start from the end if we don't have current yet.
>+        index = index.value_or(m_floatings.size());
>+        while (true) {
>+            index = previousFloatingIndex(floatingType, m_floatings, *index);
>+            if (!index)
>+                return { };
>+
>+            auto bottom = m_layoutContext.displayBoxForLayoutBox(*m_floatings[*index])->bottom();
>             // Is this floating intrusive on this position?
>             if (bottom > verticalPosition)
>                 return index;
>         }
>+
>         return { };
>     };
> 
>-    auto& floatings = m_floatingState.floatings();
>-    m_current.m_leftIndex = findFloatingBelow(floatings.left);
>-    m_current.m_rightIndex = findFloatingBelow(floatings.right);
>+    m_current.m_leftIndex = findFloatingBelow(Float::Left);
>+    m_current.m_rightIndex = findFloatingBelow(Float::Right);
>+    
>+    ASSERT(!m_current.m_leftIndex || (*m_current.m_leftIndex < m_floatings.size() && m_floatings[*m_current.m_leftIndex]->isLeftFloatingPositioned()));
>+    ASSERT(!m_current.m_rightIndex || (*m_current.m_rightIndex < m_floatings.size() && m_floatings[*m_current.m_rightIndex]->isRightFloatingPositioned()));
> }
> 
> bool Iterator::operator==(const Iterator& other) const
>diff --git a/Source/WebCore/layout/FloatingState.cpp b/Source/WebCore/layout/FloatingState.cpp
>index 39725907a43d3d5d5afff55c1e1447d03f005223..8a76f5da3b1fed1daa6e3883012dddf3af58b5d1 100644
>--- a/Source/WebCore/layout/FloatingState.cpp
>+++ b/Source/WebCore/layout/FloatingState.cpp
>@@ -42,23 +42,22 @@ FloatingState::FloatingState(LayoutContext& layoutContext)
> {
> }
> 
>+void FloatingState::remove(const Box& layoutBox)
>+{
>+    for (size_t index = 0; index < m_floatings.size(); ++index) {
>+        if (m_floatings[index].get() == &layoutBox) {
>+            m_floatings.remove(index);
>+            return;
>+        }
>+    }
>+    ASSERT_NOT_REACHED();
>+}
>+
> void FloatingState::append(const Box& layoutBox)
> {
>     // Floating state should hold boxes with computed position/size.
>     ASSERT(m_layoutContext.displayBoxForLayoutBox(layoutBox));
>-    m_last = makeWeakPtr(const_cast<Box&>(layoutBox));
>-
>-    if (layoutBox.isLeftFloatingPositioned()) {
>-        m_leftFloatings.append(m_last);
>-        return;
>-    }
>-
>-    if (layoutBox.isRightFloatingPositioned()) {
>-        m_rightFloatings.append(m_last);
>-        return;
>-    }
>-
>-    ASSERT_NOT_REACHED();
>+    m_floatings.append(makeWeakPtr(const_cast<Box&>(layoutBox))); 
> }
> 
> }
>diff --git a/Source/WebCore/layout/FloatingState.h b/Source/WebCore/layout/FloatingState.h
>index 96f8873ef9d126ad13648568027cebff8c9b41f0..107b75c5103ea5a431a1bbf3b09e6b95f9b208d4 100644
>--- a/Source/WebCore/layout/FloatingState.h
>+++ b/Source/WebCore/layout/FloatingState.h
>@@ -46,16 +46,13 @@ public:
>     static Ref<FloatingState> create(LayoutContext& layoutContext) { return adoptRef(*new FloatingState(layoutContext)); }
> 
>     void append(const Box& layoutBox);
>+    void remove(const Box& layoutBox);
> 
>-    bool isEmpty() const { return m_leftFloatings.isEmpty() && m_rightFloatings.isEmpty(); }
>+    bool isEmpty() const { return m_floatings.isEmpty(); }
> 
>     using FloatingList = Vector<WeakPtr<Box>>;
>-    struct Floatings {
>-        const FloatingList& left;
>-        const FloatingList& right; 
>-    };
>-    const Floatings floatings() const { return { m_leftFloatings, m_rightFloatings }; }
>-    const Box* last() const { return m_last.get(); }
>+    const FloatingList& floatings() const { return m_floatings; }
>+    const Box* last() const { return isEmpty() ? nullptr : m_floatings.last().get(); }
> 
> private:
>     friend class FloatingContext;
>@@ -64,10 +61,7 @@ private:
>     LayoutContext& layoutContext() const { return m_layoutContext; }
> 
>     LayoutContext& m_layoutContext;
>-
>-    FloatingList m_leftFloatings;
>-    FloatingList m_rightFloatings;
>-    WeakPtr<Box> m_last;
>+    FloatingList m_floatings;
> };
> 
> }

Flags:

koivisto: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188232: 346289 | 346293