- Revision
- 175505
- Author
- akl...@apple.com
- Date
- 2014-11-03 18:35:45 -0800 (Mon, 03 Nov 2014)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (175504 => 175505)
--- trunk/Source/WebCore/ChangeLog 2014-11-04 02:23:50 UTC (rev 175504)
+++ trunk/Source/WebCore/ChangeLog 2014-11-04 02:35:45 UTC (rev 175505)
@@ -1,3 +1,48 @@
+2014-11-03 Andreas Kling <akl...@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....@samsung.com>
Move WebCore/bridge to std::unique_ptr
Modified: trunk/Source/WebCore/rendering/RenderElement.cpp (175504 => 175505)
--- trunk/Source/WebCore/rendering/RenderElement.cpp 2014-11-04 02:23:50 UTC (rev 175504)
+++ trunk/Source/WebCore/rendering/RenderElement.cpp 2014-11-04 02:35:45 UTC (rev 175505)
@@ -521,8 +521,8 @@
void RenderElement::destroyLeftoverChildren()
{
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.
if (m_firstChild->node())
Modified: trunk/Source/WebCore/rendering/RenderListItem.cpp (175504 => 175505)
--- trunk/Source/WebCore/rendering/RenderListItem.cpp 2014-11-04 02:23:50 UTC (rev 175504)
+++ trunk/Source/WebCore/rendering/RenderListItem.cpp 2014-11-04 02:35:45 UTC (rev 175505)
@@ -45,6 +45,7 @@
RenderListItem::RenderListItem(Element& element, PassRef<RenderStyle> style)
: RenderBlockFlow(element, WTF::move(style))
+ , m_marker(nullptr)
, m_hasExplicitValue(false)
, m_isValueUpToDate(false)
, m_notInList(false)
@@ -52,12 +53,24 @@
setInline(false);
}
+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)
{
RenderBlockFlow::styleDidChange(diff, oldStyle);
if (style().listStyleType() == NoneListStyle && (!style().listStyleImage() || style().listStyleImage()->errorOccurred())) {
- m_marker = nullptr;
+ if (m_marker) {
+ m_marker->destroy();
+ ASSERT(!m_marker);
+ }
return;
}
@@ -66,18 +79,12 @@
// up (e.g., in some deeply nested line box). See CSS3 spec.
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();
-}
-
void RenderListItem::insertedIntoTree()
{
RenderBlockFlow::insertedIntoTree();
@@ -273,7 +280,7 @@
return;
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,
// then we are the only item in that anonymous box (since no line box
@@ -292,7 +299,7 @@
// containers other than ourselves, so we need to disable LayoutState.
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.
if (currentParent && currentParent->isAnonymousBlock() && !currentParent->firstChild() && !downcast<RenderBlock>(*currentParent).continuation())
@@ -400,7 +407,7 @@
LayoutRect markerRect(markerLogicalLeft + lineOffset, blockOffset, m_marker->width(), m_marker->height());
if (!style().isHorizontalWritingMode())
markerRect = markerRect.transposedRect();
- RenderBox* o = m_marker.get();
+ RenderBox* o = m_marker;
bool propagateVisualOverflow = true;
bool propagateLayoutOverflow = true;
do {
Modified: trunk/Source/WebCore/rendering/RenderListItem.h (175504 => 175505)
--- trunk/Source/WebCore/rendering/RenderListItem.h 2014-11-04 02:23:50 UTC (rev 175504)
+++ trunk/Source/WebCore/rendering/RenderListItem.h 2014-11-04 02:35:45 UTC (rev 175505)
@@ -34,6 +34,7 @@
class RenderListItem final : public RenderBlockFlow {
public:
RenderListItem(Element&, PassRef<RenderStyle>);
+ virtual ~RenderListItem();
Element& element() const { return downcast<Element>(nodeForNonAnonymous()); }
int value() const { if (!m_isValueUpToDate) updateValueNow(); return m_value; }
@@ -55,13 +56,13 @@
static void updateItemValuesForOrderedList(const HTMLOListElement*);
static unsigned itemCountForOrderedList(const HTMLOListElement*);
+ void didDestroyListMarker() { m_marker = nullptr; }
+
private:
virtual const char* renderName() const override { return "RenderListItem"; }
virtual bool isListItem() const override { return true; }
- virtual void willBeDestroyed() override;
-
virtual void insertedIntoTree() override;
virtual void willBeRemovedFromTree() override;
@@ -85,7 +86,7 @@
void explicitValueChanged();
int m_explicitValue;
- RenderPtr<RenderListMarker> m_marker;
+ RenderListMarker* m_marker;
mutable int m_value;
bool m_hasExplicitValue : 1;
Modified: trunk/Source/WebCore/rendering/RenderListMarker.cpp (175504 => 175505)
--- trunk/Source/WebCore/rendering/RenderListMarker.cpp 2014-11-04 02:23:50 UTC (rev 175504)
+++ trunk/Source/WebCore/rendering/RenderListMarker.cpp 2014-11-04 02:35:45 UTC (rev 175505)
@@ -1128,6 +1128,7 @@
RenderListMarker::~RenderListMarker()
{
+ m_listItem.didDestroyListMarker();
if (m_image)
m_image->removeClient(this);
}
