Make FloatingObjects own it's FloatingObject instances
authorbjonesbe@adobe.com <bjonesbe@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Sep 2013 22:01:38 +0000 (22:01 +0000)
committerbjonesbe@adobe.com <bjonesbe@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Sep 2013 22:01:38 +0000 (22:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=121323

Reviewed by Alexandru Chiculita.

As part of decoupling FloatingObjects from RenderBlock, change
FloatingObjects to properly manage the FloatingObject instances it
contains.

No new tests, no behavior change.

* rendering/FloatingObjects.cpp:
(WebCore::FloatingObject::FloatingObject): Make the constructors
private so that FloatingObjects can only be created with an OwnPtr.
Also make a RenderBox required to create a FloatingObject.
(WebCore::FloatingObject::create): Factory method to create a vanilla
FloatingObject.
(WebCore::FloatingObject::copyToNewContainer): Factory method to copy
an existing FloatingObject in the case it is overhanging or intruding
and needs to be copied to the block that it overhangs or intrudes
into.
(WebCore::FloatingObject::unsafeClone): Rename this method so it is
more obvious that it really shouldn't be used, and to make it more
obvious that one should use the copyToNewContainer method for all
normal FloatingObject copies.
(WebCore::FloatingObjects::clear): Delete all the FloatingObjects in
the set before clearing it.
(WebCore::FloatingObjects::moveAllToFloatInfoMap): Move all of the
FloatingObjects in the set to a RendererToFloatInfoMap. This is used
in RenderBlockFlow::clearFloats to when it is readding floats after
clearing the set.
(WebCore::FloatingObjects::add): Take an OwnPtr.
(WebCore::FloatingObjects::remove): Delete the removed FloatingObject.
* rendering/FloatingObjects.h: Remove FloatingObject::setRenderer(),
since the RenderBox must be set in the constructor.
* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::moveAllChildrenIncludingFloatsTo): Rename clone
to unsafeClone.
(WebCore::RenderBlock::removeFloatingObjects): Don't delete anymore,
since clear does it.
(WebCore::RenderBlock::insertFloatingObject): Handle OwnPtr properly.
(WebCore::RenderBlock::removeFloatingObject): Don't delete anymore,
since remove does it.
(WebCore::RenderBlock::removeFloatingObjectsBelow): Ditto.
(WebCore::RenderBlock::addOverhangingFloats): Use copyToNewContainer
and OwnPtr.
(WebCore::RenderBlock::addIntrudingFloats): Ditto.
* rendering/RenderBlockFlow.cpp:
(WebCore::RenderBlockFlow::clearFloats): Use exportToFloatInfoMap.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@155906 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/FloatingObjects.cpp
Source/WebCore/rendering/FloatingObjects.h
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderBlockFlow.cpp

index 04cc41e..def7428 100644 (file)
@@ -1,3 +1,55 @@
+2013-09-16  Bem Jones-Bey  <bjonesbe@adobe.com>
+
+        Make FloatingObjects own it's FloatingObject instances
+        https://bugs.webkit.org/show_bug.cgi?id=121323
+
+        Reviewed by Alexandru Chiculita.
+
+        As part of decoupling FloatingObjects from RenderBlock, change
+        FloatingObjects to properly manage the FloatingObject instances it
+        contains.
+
+        No new tests, no behavior change.
+
+        * rendering/FloatingObjects.cpp:
+        (WebCore::FloatingObject::FloatingObject): Make the constructors
+        private so that FloatingObjects can only be created with an OwnPtr.
+        Also make a RenderBox required to create a FloatingObject.
+        (WebCore::FloatingObject::create): Factory method to create a vanilla
+        FloatingObject.
+        (WebCore::FloatingObject::copyToNewContainer): Factory method to copy
+        an existing FloatingObject in the case it is overhanging or intruding
+        and needs to be copied to the block that it overhangs or intrudes
+        into.
+        (WebCore::FloatingObject::unsafeClone): Rename this method so it is
+        more obvious that it really shouldn't be used, and to make it more
+        obvious that one should use the copyToNewContainer method for all
+        normal FloatingObject copies.
+        (WebCore::FloatingObjects::clear): Delete all the FloatingObjects in
+        the set before clearing it.
+        (WebCore::FloatingObjects::moveAllToFloatInfoMap): Move all of the
+        FloatingObjects in the set to a RendererToFloatInfoMap. This is used
+        in RenderBlockFlow::clearFloats to when it is readding floats after
+        clearing the set.
+        (WebCore::FloatingObjects::add): Take an OwnPtr.
+        (WebCore::FloatingObjects::remove): Delete the removed FloatingObject.
+        * rendering/FloatingObjects.h: Remove FloatingObject::setRenderer(),
+        since the RenderBox must be set in the constructor.
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::moveAllChildrenIncludingFloatsTo): Rename clone
+        to unsafeClone.
+        (WebCore::RenderBlock::removeFloatingObjects): Don't delete anymore,
+        since clear does it.
+        (WebCore::RenderBlock::insertFloatingObject): Handle OwnPtr properly.
+        (WebCore::RenderBlock::removeFloatingObject): Don't delete anymore,
+        since remove does it.
+        (WebCore::RenderBlock::removeFloatingObjectsBelow): Ditto.
+        (WebCore::RenderBlock::addOverhangingFloats): Use copyToNewContainer
+        and OwnPtr.
+        (WebCore::RenderBlock::addIntrudingFloats): Ditto.
+        * rendering/RenderBlockFlow.cpp:
+        (WebCore::RenderBlockFlow::clearFloats): Use exportToFloatInfoMap.
+
 2013-09-16  Hugo Parente Lima  <hugo.lima@openbossa.org>
 
         Fix creation of embedded JS and CSS files on cmake based ports.
index 479a996..a19141a 100644 (file)
@@ -42,6 +42,63 @@ struct SameSizeAsFloatingObject {
 
 COMPILE_ASSERT(sizeof(FloatingObject) == sizeof(SameSizeAsFloatingObject), FloatingObject_should_stay_small);
 
+FloatingObject::FloatingObject(RenderBox* renderer)
+    : m_renderer(renderer)
+    , m_originatingLine(0)
+    , m_paginationStrut(0)
+    , m_shouldPaint(true)
+    , m_isDescendant(false)
+    , m_isPlaced(false)
+#ifndef NDEBUG
+    , m_isInPlacedTree(false)
+#endif
+{
+    EFloat type = renderer->style()->floating();
+    ASSERT(type != NoFloat);
+    if (type == LeftFloat)
+        m_type = FloatLeft;
+    else if (type == RightFloat)
+        m_type = FloatRight;
+}
+
+FloatingObject::FloatingObject(RenderBox* renderer, Type type, const LayoutRect& frameRect, bool shouldPaint, bool isDescendant)
+    : m_renderer(renderer)
+    , m_originatingLine(0)
+    , m_frameRect(frameRect)
+    , m_paginationStrut(0)
+    , m_type(type)
+    , m_shouldPaint(shouldPaint)
+    , m_isDescendant(isDescendant)
+    , m_isPlaced(true)
+#ifndef NDEBUG
+    , m_isInPlacedTree(false)
+#endif
+{
+}
+
+PassOwnPtr<FloatingObject> FloatingObject::create(RenderBox* renderer)
+{
+    OwnPtr<FloatingObject> newObj = adoptPtr(new FloatingObject(renderer));
+    newObj->setShouldPaint(!renderer->hasSelfPaintingLayer()); // If a layer exists, the float will paint itself. Otherwise someone else will.
+    newObj->setIsDescendant(true);
+
+    return newObj.release();
+}
+
+PassOwnPtr<FloatingObject> FloatingObject::copyToNewContainer(LayoutSize offset, bool shouldPaint, bool isDescendant) const
+{
+    return adoptPtr(new FloatingObject(renderer(), type(), LayoutRect(frameRect().location() - offset, frameRect().size()), shouldPaint, isDescendant));
+}
+
+PassOwnPtr<FloatingObject> FloatingObject::unsafeClone() const
+{
+    OwnPtr<FloatingObject> cloneObject = adoptPtr(new FloatingObject(renderer(), type(), m_frameRect, m_shouldPaint, m_isDescendant));
+    cloneObject->m_originatingLine = m_originatingLine;
+    cloneObject->m_paginationStrut = m_paginationStrut;
+    cloneObject->m_isPlaced = m_isPlaced;
+    return cloneObject.release();
+}
+
 template <FloatingObject::Type FloatTypeValue>
 class ComputeFloatOffsetAdapter {
 public:
@@ -107,15 +164,26 @@ void FloatingObjects::clearLineBoxTreePointers()
 
 void FloatingObjects::clear()
 {
-    // FIXME: This should call deleteAllValues, except clearFloats
-    // like to play fast and loose with ownership of these pointers.
-    // If we move to OwnPtr that will fix this ownership oddness.
+    deleteAllValues(m_set);
     m_set.clear();
     m_placedFloatsTree.clear();
     m_leftObjectsCount = 0;
     m_rightObjectsCount = 0;
 }
 
+void FloatingObjects::moveAllToFloatInfoMap(RendererToFloatInfoMap& map)
+{
+    FloatingObjectSetIterator end = m_set.end();
+    for (FloatingObjectSetIterator it = m_set.begin(); it != end; ++it) {
+        FloatingObject* f = *it;
+        map.add(f->renderer(), f);
+    }
+    // clear set before clearing this because we don't want to delete all of
+    // the objects we have just transferred.
+    m_set.clear();
+    clear();
+}
+
 void FloatingObjects::increaseObjectsCount(FloatingObject::Type type)
 {
     if (type == FloatingObject::FloatLeft)
@@ -167,12 +235,14 @@ void FloatingObjects::removePlacedObject(FloatingObject* floatingObject)
 #endif
 }
 
-void FloatingObjects::add(FloatingObject* floatingObject)
+FloatingObject* FloatingObjects::add(PassOwnPtr<FloatingObject> floatingObject)
 {
-    increaseObjectsCount(floatingObject->type());
-    m_set.add(floatingObject);
-    if (floatingObject->isPlaced())
-        addPlacedObject(floatingObject);
+    FloatingObject* newObject = floatingObject.leakPtr();
+    increaseObjectsCount(newObject->type());
+    m_set.add(newObject);
+    if (newObject->isPlaced())
+        addPlacedObject(newObject);
+    return newObject;
 }
 
 void FloatingObjects::remove(FloatingObject* floatingObject)
@@ -182,6 +252,8 @@ void FloatingObjects::remove(FloatingObject* floatingObject)
     ASSERT(floatingObject->isPlaced() || !floatingObject->isInPlacedTree());
     if (floatingObject->isPlaced())
         removePlacedObject(floatingObject);
+    ASSERT(!floatingObject->originatingLine());
+    delete floatingObject;
 }
 
 void FloatingObjects::computePlacedFloatsTree()
index 31e63fd..296c7d6 100644 (file)
@@ -27,6 +27,7 @@
 #include "PODIntervalTree.h"
 #include "RootInlineBox.h"
 #include <wtf/ListHashSet.h>
+#include <wtf/OwnPtr.h>
 
 namespace WebCore {
 
@@ -41,50 +42,11 @@ public:
     // Note that Type uses bits so you can use FloatLeftRight as a mask to query for both left and right.
     enum Type { FloatLeft = 1, FloatRight = 2, FloatLeftRight = 3 };
 
-    explicit FloatingObject(EFloat type)
-        : m_renderer(0)
-        , m_originatingLine(0)
-        , m_paginationStrut(0)
-        , m_shouldPaint(true)
-        , m_isDescendant(false)
-        , m_isPlaced(false)
-#ifndef NDEBUG
-        , m_isInPlacedTree(false)
-#endif
-    {
-        ASSERT(type != NoFloat);
-        if (type == LeftFloat)
-            m_type = FloatLeft;
-        else if (type == RightFloat)
-            m_type = FloatRight;
-    }
+    static PassOwnPtr<FloatingObject> create(RenderBox*);
 
-    FloatingObject(Type type, const LayoutRect& frameRect)
-        : m_renderer(0)
-        , m_originatingLine(0)
-        , m_frameRect(frameRect)
-        , m_paginationStrut(0)
-        , m_type(type)
-        , m_shouldPaint(true)
-        , m_isDescendant(false)
-        , m_isPlaced(true)
-#ifndef NDEBUG
-        , m_isInPlacedTree(false)
-#endif
-    {
-    }
+    PassOwnPtr<FloatingObject> copyToNewContainer(LayoutSize, bool shouldPaint = false, bool isDescendant = false) const;
 
-    FloatingObject* clone() const
-    {
-        FloatingObject* cloneObject = new FloatingObject(type(), m_frameRect);
-        cloneObject->m_renderer = m_renderer;
-        cloneObject->m_originatingLine = m_originatingLine;
-        cloneObject->m_paginationStrut = m_paginationStrut;
-        cloneObject->m_shouldPaint = m_shouldPaint;
-        cloneObject->m_isDescendant = m_isDescendant;
-        cloneObject->m_isPlaced = m_isPlaced;
-        return cloneObject;
-    }
+    PassOwnPtr<FloatingObject> unsafeClone() const;
 
     Type type() const { return static_cast<Type>(m_type); }
     RenderBox* renderer() const { return m_renderer; }
@@ -121,7 +83,6 @@ public:
     void setIsDescendant(bool isDescendant) { m_isDescendant = isDescendant; }
 
     // FIXME: Callers of these methods are dangerous and should be whitelisted explicitly or removed.
-    void setRenderer(RenderBox* renderer) { m_renderer = renderer; }
     RootInlineBox* originatingLine() const { return m_originatingLine; }
     void setOriginatingLine(RootInlineBox* line) { m_originatingLine = line; }
 
@@ -166,6 +127,9 @@ public:
     }
 
 private:
+    explicit FloatingObject(RenderBox*);
+    FloatingObject(RenderBox*, Type, const LayoutRect&, bool shouldPaint, bool isDescendant);
+
     RenderBox* m_renderer;
     RootInlineBox* m_originatingLine;
     LayoutRect m_frameRect;
@@ -194,7 +158,7 @@ typedef FloatingObjectSet::const_iterator FloatingObjectSetIterator;
 typedef PODInterval<int, FloatingObject*> FloatingObjectInterval;
 typedef PODIntervalTree<int, FloatingObject*> FloatingObjectTree;
 typedef PODFreeListArena<PODRedBlackTree<FloatingObjectInterval>::Node> IntervalArena;
-
+typedef HashMap<RenderBox*, FloatingObject*> RendererToFloatInfoMap;
 
 class FloatingObjects {
     WTF_MAKE_NONCOPYABLE(FloatingObjects); WTF_MAKE_FAST_ALLOCATED;
@@ -203,7 +167,8 @@ public:
     ~FloatingObjects();
 
     void clear();
-    void add(FloatingObject*);
+    void moveAllToFloatInfoMap(RendererToFloatInfoMap&);
+    FloatingObject* add(PassOwnPtr<FloatingObject>);
     void remove(FloatingObject*);
     void addPlacedObject(FloatingObject*);
     void removePlacedObject(FloatingObject*);
index 0091b43..5cdf95b 100644 (file)
@@ -1148,7 +1148,7 @@ void RenderBlock::moveAllChildrenIncludingFloatsTo(RenderBlock* toBlock, bool fu
             if (toBlock->containsFloat(floatingObject->renderer()))
                 continue;
 
-            toBlock->m_floatingObjects->add(floatingObject->clone());
+            toBlock->m_floatingObjects->add(floatingObject->unsafeClone());
         }
     }
 }
@@ -3260,7 +3260,6 @@ void RenderBlock::removeFloatingObjects()
     if (!m_floatingObjects)
         return;
 
-    deleteAllValues(m_floatingObjects->set());
     m_floatingObjects->clear();
 }
 
@@ -3281,7 +3280,7 @@ FloatingObject* RenderBlock::insertFloatingObject(RenderBox* o)
 
     // Create the special object entry & append it to the list
 
-    FloatingObject* newObj = new FloatingObject(o->style()->floating());
+    OwnPtr<FloatingObject> newObj = FloatingObject::create(o);
     
     // Our location is irrelevant if we're unsplittable or no pagination is in effect.
     // Just go ahead and lay out the float.
@@ -3304,13 +3303,7 @@ FloatingObject* RenderBlock::insertFloatingObject(RenderBox* o)
         shapeOutside->setShapeSize(logicalWidthForChild(o), logicalHeightForChild(o));
 #endif
 
-    newObj->setShouldPaint(!o->hasSelfPaintingLayer()); // If a layer exists, the float will paint itself. Otherwise someone else will.
-    newObj->setIsDescendant(true);
-    newObj->setRenderer(o);
-
-    m_floatingObjects->add(newObj);
-    
-    return newObj;
+    return m_floatingObjects->add(newObj.release());
 }
 
 void RenderBlock::removeFloatingObject(RenderBox* o)
@@ -3345,8 +3338,6 @@ void RenderBlock::removeFloatingObject(RenderBox* o)
                 markLinesDirtyInBlockRange(0, logicalBottom);
             }
             m_floatingObjects->remove(r);
-            ASSERT(!r->originatingLine());
-            delete r;
         }
     }
 }
@@ -3360,8 +3351,6 @@ void RenderBlock::removeFloatingObjectsBelow(FloatingObject* lastFloat, int logi
     FloatingObject* curr = floatingObjectSet.last();
     while (curr != lastFloat && (!curr->isPlaced() || curr->logicalTop(isHorizontalWritingMode()) >= logicalOffset)) {
         m_floatingObjects->remove(curr);
-        ASSERT(!curr->originatingLine());
-        delete curr;
         if (floatingObjectSet.isEmpty())
             break;
         curr = floatingObjectSet.last();
@@ -3829,24 +3818,21 @@ LayoutUnit RenderBlock::addOverhangingFloats(RenderBlock* child, bool makeChildP
             // If the object is not in the list, we add it now.
             if (!containsFloat(r->renderer())) {
                 LayoutSize offset = isHorizontalWritingMode() ? LayoutSize(-childLogicalLeft, -childLogicalTop) : LayoutSize(-childLogicalTop, -childLogicalLeft);
-                FloatingObject* floatingObj = new FloatingObject(r->type(), LayoutRect(r->frameRect().location() - offset, r->frameRect().size()));
-                floatingObj->setRenderer(r->renderer());
+                bool shouldPaint = false;
 
                 // The nearest enclosing layer always paints the float (so that zindex and stacking
                 // behaves properly).  We always want to propagate the desire to paint the float as
                 // far out as we can, to the outermost block that overlaps the float, stopping only
                 // if we hit a self-painting layer boundary.
-                if (r->renderer()->enclosingFloatPaintingLayer() == enclosingFloatPaintingLayer())
+                if (r->renderer()->enclosingFloatPaintingLayer() == enclosingFloatPaintingLayer()) {
                     r->setShouldPaint(false);
-                else
-                    floatingObj->setShouldPaint(false);
-
-                floatingObj->setIsDescendant(true);
-
+                    shouldPaint = true;
+                }
                 // We create the floating object list lazily.
                 if (!m_floatingObjects)
                     createFloatingObjects();
-                m_floatingObjects->add(floatingObj);
+
+                m_floatingObjects->add(r->copyToNewContainer(offset, shouldPaint, true));
             }
         } else {
             if (makeChildPaintOtherFloats && !r->shouldPaint() && !r->renderer()->hasSelfPaintingLayer()
@@ -3897,28 +3883,20 @@ void RenderBlock::addIntrudingFloats(RenderBlock* prev, LayoutUnit logicalLeftOf
         FloatingObject* r = *prevIt;
         if (r->logicalBottom(isHorizontalWritingMode()) > logicalTopOffset) {
             if (!m_floatingObjects || !m_floatingObjects->set().contains(r)) {
-                LayoutSize offset = isHorizontalWritingMode() ? LayoutSize(logicalLeftOffset, logicalTopOffset) : LayoutSize(logicalTopOffset, logicalLeftOffset);
-                FloatingObject* floatingObj = new FloatingObject(r->type(), LayoutRect(r->frameRect().location() - offset, r->frameRect().size()));
+                // We create the floating object list lazily.
+                if (!m_floatingObjects)
+                    createFloatingObjects();
 
                 // Applying the child's margin makes no sense in the case where the child was passed in.
                 // since this margin was added already through the modification of the |logicalLeftOffset| variable
                 // above.  |logicalLeftOffset| will equal the margin in this case, so it's already been taken
                 // into account.  Only apply this code if prev is the parent, since otherwise the left margin
                 // will get applied twice.
-                if (prev != parent()) {
-                    if (isHorizontalWritingMode())
-                        floatingObj->setX(floatingObj->x() + prev->marginLeft());
-                    else
-                        floatingObj->setY(floatingObj->y() + prev->marginTop());
-                }
-               
-                floatingObj->setShouldPaint(false); // We are not in the direct inheritance chain for this float. We will never paint it.
-                floatingObj->setRenderer(r->renderer());
-                
-                // We create the floating object list lazily.
-                if (!m_floatingObjects)
-                    createFloatingObjects();
-                m_floatingObjects->add(floatingObj);
+                LayoutSize offset = isHorizontalWritingMode()
+                    ? LayoutSize(logicalLeftOffset + (prev != parent() ? prev->marginLeft() : LayoutUnit()), logicalTopOffset)
+                    : LayoutSize(logicalTopOffset, logicalLeftOffset + (prev != parent() ? prev->marginTop() : LayoutUnit()));
+
+                m_floatingObjects->add(r->copyToNewContainer(offset));
             }
         }
     }
index 1d75c91..ef58708 100644 (file)
@@ -103,7 +103,6 @@ void RenderBlockFlow::clearFloats()
     // Inline blocks are covered by the isReplaced() check in the avoidFloats method.
     if (avoidsFloats() || isRoot() || isRenderView() || isFloatingOrOutOfFlowPositioned() || isTableCell()) {
         if (m_floatingObjects) {
-            deleteAllValues(m_floatingObjects->set());
             m_floatingObjects->clear();
         }
         if (!oldIntrudingFloatSet.isEmpty())
@@ -111,20 +110,13 @@ void RenderBlockFlow::clearFloats()
         return;
     }
 
-    typedef HashMap<RenderObject*, FloatingObject*> RendererToFloatInfoMap;
     RendererToFloatInfoMap floatMap;
 
     if (m_floatingObjects) {
-        const FloatingObjectSet& floatingObjectSet = m_floatingObjects->set();
-        if (childrenInline()) {
-            FloatingObjectSetIterator end = floatingObjectSet.end();
-            for (FloatingObjectSetIterator it = floatingObjectSet.begin(); it != end; ++it) {
-                FloatingObject* f = *it;
-                floatMap.add(f->renderer(), f);
-            }
-        } else
-            deleteAllValues(floatingObjectSet);
-        m_floatingObjects->clear();
+        if (childrenInline())
+            m_floatingObjects->moveAllToFloatInfoMap(floatMap);
+        else
+            m_floatingObjects->clear();
     }
 
     // We should not process floats if the parent node is not a RenderBlock. Otherwise, we will add