Changeset 175505 in webkit

Timestamp:

Nov 3, 2014 6:35:45 PM (9 years ago)

Author:

akling@apple.com

Message:

Clarify RenderListMarker ownership model.
<​https://webkit.org/b/138329>

Reviewed by Antti Koivisto.

A RenderListMarker is either in-tree and owned by the tree, or out-of-tree
and owned by a RenderListItem.

This patch changes RenderListItem::m_marker to be a raw pointer, and removes
the special handling of list markers in RenderElement child teardown.

We also remove the willBeDestroyed() hook. It was used to clear out the
m_marker pointer, but this is now done in the regular ~RenderListItem()
destructor with an assertion for marker sanity. m_marker is automatically
nulled out by a didDestroyListMarker() callback on RenderListItem.

• rendering/RenderElement.cpp:

(WebCore::RenderElement::destroyLeftoverChildren):

Removed special handling of list marker renderers when deleting a
RenderElement's children.

• rendering/RenderListItem.cpp:

(WebCore::RenderListItem::RenderListItem):
(WebCore::RenderListItem::~RenderListItem):
(WebCore::RenderListItem::styleDidChange):
(WebCore::RenderListItem::insertOrMoveMarkerRendererIfNeeded):
(WebCore::RenderListItem::positionListMarker):

Made m_marker a raw pointer instead of a RenderPtr since the ownership
really switches between weak and strong reference.

(WebCore::RenderListItem::willBeDestroyed):

• rendering/RenderListMarker.cpp:

(WebCore::RenderListMarker::~RenderListMarker):

Added a regular destructor to replace the willBeDestroyed() hook.

• rendering/RenderListItem.h:

(WebCore::RenderListItem::didDestroyListMarker):

Added. Called by ~RenderListMarker to null out RenderListItem::m_marker.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderElement.cpp (modified) (1 diff)
• rendering/RenderListItem.cpp (modified) (7 diffs)
• rendering/RenderListItem.h (modified) (4 diffs)
• rendering/RenderListMarker.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-11-03  Andreas Kling  <akling@apple.com>
+
+        Clarify RenderListMarker ownership model.
+        <https://webkit.org/b/138329>
+
+        Reviewed by Antti Koivisto.
+
+        A RenderListMarker is either in-tree and owned by the tree, or out-of-tree
+        and owned by a RenderListItem.
+
+        This patch changes RenderListItem::m_marker to be a raw pointer, and removes
+        the special handling of list markers in RenderElement child teardown.
+
+        We also remove the willBeDestroyed() hook. It was used to clear out the
+        m_marker pointer, but this is now done in the regular ~RenderListItem()
+        destructor with an assertion for marker sanity. m_marker is automatically
+        nulled out by a didDestroyListMarker() callback on RenderListItem.
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::destroyLeftoverChildren):
+
+            Removed special handling of list marker renderers when deleting a
+            RenderElement's children.
+
+        * rendering/RenderListItem.cpp:
+        (WebCore::RenderListItem::RenderListItem):
+        (WebCore::RenderListItem::~RenderListItem):
+        (WebCore::RenderListItem::styleDidChange):
+        (WebCore::RenderListItem::insertOrMoveMarkerRendererIfNeeded):
+        (WebCore::RenderListItem::positionListMarker):
+
+            Made m_marker a raw pointer instead of a RenderPtr since the ownership
+            really switches between weak and strong reference.
+
+        (WebCore::RenderListItem::willBeDestroyed):
+        * rendering/RenderListMarker.cpp:
+        (WebCore::RenderListMarker::~RenderListMarker):
+
+            Added a regular destructor to replace the willBeDestroyed() hook.
+
+        * rendering/RenderListItem.h:
+        (WebCore::RenderListItem::didDestroyListMarker):
+
+            Added. Called by ~RenderListMarker to null out RenderListItem::m_marker.
+
 2014-11-03  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
 

trunk/Source/WebCore/rendering/RenderElement.cpp

@@ -522 +522 @@
 {
     while (m_firstChild) {
-        if (m_firstChild->isListMarker() || (m_firstChild->style().styleType() == FIRST_LETTER && !m_firstChild->isText())) {
-            m_firstChild->removeFromParent(); // List markers are owned by their enclosing list and so don't get destroyed by this container. Similarly, first letters are destroyed by their remaining text fragment.
+        if (m_firstChild->style().styleType() == FIRST_LETTER && !m_firstChild->isText()) {
+            m_firstChild->removeFromParent(); // :first-letter fragment renderers are destroyed by their remaining text fragment.
         } else {
             // Destroy any anonymous children remaining in the render tree, as well as implicit (shadow) DOM elements like those used in the engine-based text fields.

trunk/Source/WebCore/rendering/RenderListItem.cpp

@@ -46 +46 @@
 RenderListItem::RenderListItem(Element& element, PassRef<RenderStyle> style)
     : RenderBlockFlow(element, WTF::move(style))
+    , m_marker(nullptr)
     , m_hasExplicitValue(false)
     , m_isValueUpToDate(false)
@@ -53 +54 @@
 }
 
+RenderListItem::~RenderListItem()
+{
+    ASSERT(!m_marker || !m_marker->parent());
+    if (m_marker) {
+        m_marker->destroy();
+        ASSERT(!m_marker);
+    }
+}
+
 void RenderListItem::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
@@ -58 +68 @@
 
     if (style().listStyleType() == NoneListStyle && (!style().listStyleImage() || style().listStyleImage()->errorOccurred())) {
-        m_marker = nullptr;
+        if (m_marker) {
+            m_marker->destroy();
+            ASSERT(!m_marker);
+        }
         return;
     }
@@ -67 +80 @@
     newStyle.get().inheritFrom(&style());
     if (!m_marker) {
-        m_marker = createRenderer<RenderListMarker>(*this, WTF::move(newStyle));
+        m_marker = createRenderer<RenderListMarker>(*this, WTF::move(newStyle)).leakPtr();
         m_marker->initializeStyle();
     } else
         m_marker->setStyle(WTF::move(newStyle));
-}
-
-void RenderListItem::willBeDestroyed()
-{
-    m_marker = nullptr;
-    RenderBlockFlow::willBeDestroyed();
 }
 
@@ -274 +281 @@
 
     RenderElement* currentParent = m_marker->parent();
-    RenderBlock* newParent = getParentOfFirstLineBox(this, m_marker.get());
+    RenderBlock* newParent = getParentOfFirstLineBox(this, m_marker);
     if (!newParent) {
         // If the marker is currently contained inside an anonymous box,
@@ -293 +300 @@
         LayoutStateDisabler layoutStateDisabler(&view());
         m_marker->removeFromParent();
-        newParent->addChild(m_marker.get(), firstNonMarkerChild(newParent));
+        newParent->addChild(m_marker, firstNonMarkerChild(newParent));
         m_marker->updateMarginsAndContent();
         // If current parent is an anonymous block that has lost all its children, destroy it.
@@ -401 +408 @@
             if (!style().isHorizontalWritingMode())
                 markerRect = markerRect.transposedRect();
-            RenderBox* o = m_marker.get();
+            RenderBox* o = m_marker;
             bool propagateVisualOverflow = true;
             bool propagateLayoutOverflow = true;

trunk/Source/WebCore/rendering/RenderListItem.h

@@ -35 +35 @@
 public:
     RenderListItem(Element&, PassRef<RenderStyle>);
+    virtual ~RenderListItem();
     Element& element() const { return downcast<Element>(nodeForNonAnonymous()); }
 
@@ -56 +57 @@
     static unsigned itemCountForOrderedList(const HTMLOListElement*);
 
+    void didDestroyListMarker() { m_marker = nullptr; }
+
 private:
     virtual const char* renderName() const override { return "RenderListItem"; }
@@ -61 +64 @@
     virtual bool isListItem() const override { return true; }
     
-    virtual void willBeDestroyed() override;
-
     virtual void insertedIntoTree() override;
     virtual void willBeRemovedFromTree() override;
@@ -86 +87 @@
 
     int m_explicitValue;
-    RenderPtr<RenderListMarker> m_marker;
+    RenderListMarker* m_marker;
     mutable int m_value;
 

trunk/Source/WebCore/rendering/RenderListMarker.cpp

@@ -1129 +1129 @@
 RenderListMarker::~RenderListMarker()
 {
+    m_listItem.didDestroyListMarker();
     if (m_image)
         m_image->removeClient(this);