Title: [234481] trunk/Source/WebCore
Revision
234481
Author
za...@apple.com
Date
2018-08-01 14:46:52 -0700 (Wed, 01 Aug 2018)

Log Message

[LFC][Floating] Revert back to only one list for the all the floatings.
https://bugs.webkit.org/show_bug.cgi?id=188232

Reviewed by Antti Koivisto.

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):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (234480 => 234481)


--- trunk/Source/WebCore/ChangeLog	2018-08-01 21:36:48 UTC (rev 234480)
+++ trunk/Source/WebCore/ChangeLog	2018-08-01 21:46:52 UTC (rev 234481)
@@ -1,3 +1,32 @@
+2018-08-01  Zalan Bujtas  <za...@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 Antti Koivisto.
+
+        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-08-01  Basuke Suzuki  <basuke.suz...@sony.com>
 
         [Curl] Change synchronous request logic using MessageQueue to match with Mac port.

Modified: trunk/Source/WebCore/layout/FloatingContext.cpp (234480 => 234481)


--- trunk/Source/WebCore/layout/FloatingContext.cpp	2018-08-01 21:36:48 UTC (rev 234480)
+++ trunk/Source/WebCore/layout/FloatingContext.cpp	2018-08-01 21:46:52 UTC (rev 234481)
@@ -59,8 +59,6 @@
 
 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;
@@ -71,8 +69,10 @@
 
 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;
@@ -81,7 +81,7 @@
 
 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++();
@@ -92,7 +92,7 @@
     void set(LayoutUnit verticalPosition);
 
     const LayoutContext& m_layoutContext;
-    const FloatingState& m_floatingState;
+    const FloatingState::FloatingList& m_floatings;
     FloatingPair m_current;
 };
 
@@ -99,12 +99,12 @@
 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)
@@ -221,15 +221,9 @@
     return rightAlignedBoxLeft;
 }
 
-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)
 {
 }
 
@@ -238,7 +232,8 @@
     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
@@ -246,7 +241,8 @@
     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
@@ -295,15 +291,28 @@
     return *rightBottom;
 }
 
-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()) {
@@ -311,19 +320,24 @@
         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)
+            index = previousFloatingIndex(floatingType, m_floatings, *index);
+            if (!index)
+                return { };
+
+            if (m_layoutContext.displayBoxForLayoutBox(*m_floatings[*index])->bottom() > currentBottom)
                 return index;
-            if (!index--)
-                return { };
         }
 
         ASSERT_NOT_REACHED();
@@ -345,13 +359,13 @@
     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;
@@ -366,7 +380,7 @@
 
     m_current.m_verticalPosition = verticalPosition;
     // No floatings at all?
-    if (m_floatingState.isEmpty()) {
+    if (m_floatings.isEmpty()) {
         ASSERT_NOT_REACHED();
 
         m_current.m_leftIndex = { };
@@ -374,21 +388,32 @@
         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

Modified: trunk/Source/WebCore/layout/FloatingState.cpp (234480 => 234481)


--- trunk/Source/WebCore/layout/FloatingState.cpp	2018-08-01 21:36:48 UTC (rev 234480)
+++ trunk/Source/WebCore/layout/FloatingState.cpp	2018-08-01 21:46:52 UTC (rev 234481)
@@ -58,6 +58,17 @@
 }
 #endif
 
+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)
 {
     ASSERT(is<Container>(*m_formattingContextRoot));
@@ -65,19 +76,7 @@
 
     // 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)));
 }
 
 }

Modified: trunk/Source/WebCore/layout/FloatingState.h (234480 => 234481)


--- trunk/Source/WebCore/layout/FloatingState.h	2018-08-01 21:36:48 UTC (rev 234480)
+++ trunk/Source/WebCore/layout/FloatingState.h	2018-08-01 21:46:52 UTC (rev 234481)
@@ -46,16 +46,13 @@
     static Ref<FloatingState> create(LayoutContext& layoutContext, const Box& formattingContextRoot) { return adoptRef(*new FloatingState(layoutContext, formattingContextRoot)); }
 
     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;
@@ -65,10 +62,7 @@
 
     LayoutContext& m_layoutContext;
     WeakPtr<Box> m_formattingContextRoot;
-
-    FloatingList m_leftFloatings;
-    FloatingList m_rightFloatings;
-    WeakPtr<Box> m_last;
+    FloatingList m_floatings;
 };
 
 }
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to