<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[175505] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/175505">175505</a></dd>
<dt>Author</dt> <dd>akling@apple.com</dd>
<dt>Date</dt> <dd>2014-11-03 18:35:45 -0800 (Mon, 03 Nov 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorerenderingRenderElementcpp">trunk/Source/WebCore/rendering/RenderElement.cpp</a></li>
<li><a href="#trunkSourceWebCorerenderingRenderListItemcpp">trunk/Source/WebCore/rendering/RenderListItem.cpp</a></li>
<li><a href="#trunkSourceWebCorerenderingRenderListItemh">trunk/Source/WebCore/rendering/RenderListItem.h</a></li>
<li><a href="#trunkSourceWebCorerenderingRenderListMarkercpp">trunk/Source/WebCore/rendering/RenderListMarker.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (175504 => 175505)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,48 @@
</span><ins>+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.
+
</ins><span class="cx"> 2014-11-03 Gyuyoung Kim <gyuyoung.kim@samsung.com>
</span><span class="cx">
</span><span class="cx"> Move WebCore/bridge to std::unique_ptr
</span></span></pre></div>
<a id="trunkSourceWebCorerenderingRenderElementcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/rendering/RenderElement.cpp (175504 => 175505)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -521,8 +521,8 @@
</span><span class="cx"> void RenderElement::destroyLeftoverChildren()
</span><span class="cx"> {
</span><span class="cx"> while (m_firstChild) {
</span><del>- 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.
</del><ins>+ if (m_firstChild->style().styleType() == FIRST_LETTER && !m_firstChild->isText()) {
+ m_firstChild->removeFromParent(); // :first-letter fragment renderers are destroyed by their remaining text fragment.
</ins><span class="cx"> } else {
</span><span class="cx"> // 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.
</span><span class="cx"> if (m_firstChild->node())
</span></span></pre></div>
<a id="trunkSourceWebCorerenderingRenderListItemcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/rendering/RenderListItem.cpp (175504 => 175505)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -45,6 +45,7 @@
</span><span class="cx">
</span><span class="cx"> RenderListItem::RenderListItem(Element& element, PassRef<RenderStyle> style)
</span><span class="cx"> : RenderBlockFlow(element, WTF::move(style))
</span><ins>+ , m_marker(nullptr)
</ins><span class="cx"> , m_hasExplicitValue(false)
</span><span class="cx"> , m_isValueUpToDate(false)
</span><span class="cx"> , m_notInList(false)
</span><span class="lines">@@ -52,12 +53,24 @@
</span><span class="cx"> setInline(false);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+RenderListItem::~RenderListItem()
+{
+ ASSERT(!m_marker || !m_marker->parent());
+ if (m_marker) {
+ m_marker->destroy();
+ ASSERT(!m_marker);
+ }
+}
+
</ins><span class="cx"> void RenderListItem::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
</span><span class="cx"> {
</span><span class="cx"> RenderBlockFlow::styleDidChange(diff, oldStyle);
</span><span class="cx">
</span><span class="cx"> if (style().listStyleType() == NoneListStyle && (!style().listStyleImage() || style().listStyleImage()->errorOccurred())) {
</span><del>- m_marker = nullptr;
</del><ins>+ if (m_marker) {
+ m_marker->destroy();
+ ASSERT(!m_marker);
+ }
</ins><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -66,18 +79,12 @@
</span><span class="cx"> // up (e.g., in some deeply nested line box). See CSS3 spec.
</span><span class="cx"> newStyle.get().inheritFrom(&style());
</span><span class="cx"> if (!m_marker) {
</span><del>- m_marker = createRenderer<RenderListMarker>(*this, WTF::move(newStyle));
</del><ins>+ m_marker = createRenderer<RenderListMarker>(*this, WTF::move(newStyle)).leakPtr();
</ins><span class="cx"> m_marker->initializeStyle();
</span><span class="cx"> } else
</span><span class="cx"> m_marker->setStyle(WTF::move(newStyle));
</span><span class="cx"> }
</span><span class="cx">
</span><del>-void RenderListItem::willBeDestroyed()
-{
- m_marker = nullptr;
- RenderBlockFlow::willBeDestroyed();
-}
-
</del><span class="cx"> void RenderListItem::insertedIntoTree()
</span><span class="cx"> {
</span><span class="cx"> RenderBlockFlow::insertedIntoTree();
</span><span class="lines">@@ -273,7 +280,7 @@
</span><span class="cx"> return;
</span><span class="cx">
</span><span class="cx"> RenderElement* currentParent = m_marker->parent();
</span><del>- RenderBlock* newParent = getParentOfFirstLineBox(this, m_marker.get());
</del><ins>+ RenderBlock* newParent = getParentOfFirstLineBox(this, m_marker);
</ins><span class="cx"> if (!newParent) {
</span><span class="cx"> // If the marker is currently contained inside an anonymous box,
</span><span class="cx"> // then we are the only item in that anonymous box (since no line box
</span><span class="lines">@@ -292,7 +299,7 @@
</span><span class="cx"> // containers other than ourselves, so we need to disable LayoutState.
</span><span class="cx"> LayoutStateDisabler layoutStateDisabler(&view());
</span><span class="cx"> m_marker->removeFromParent();
</span><del>- newParent->addChild(m_marker.get(), firstNonMarkerChild(newParent));
</del><ins>+ newParent->addChild(m_marker, firstNonMarkerChild(newParent));
</ins><span class="cx"> m_marker->updateMarginsAndContent();
</span><span class="cx"> // If current parent is an anonymous block that has lost all its children, destroy it.
</span><span class="cx"> if (currentParent && currentParent->isAnonymousBlock() && !currentParent->firstChild() && !downcast<RenderBlock>(*currentParent).continuation())
</span><span class="lines">@@ -400,7 +407,7 @@
</span><span class="cx"> LayoutRect markerRect(markerLogicalLeft + lineOffset, blockOffset, m_marker->width(), m_marker->height());
</span><span class="cx"> if (!style().isHorizontalWritingMode())
</span><span class="cx"> markerRect = markerRect.transposedRect();
</span><del>- RenderBox* o = m_marker.get();
</del><ins>+ RenderBox* o = m_marker;
</ins><span class="cx"> bool propagateVisualOverflow = true;
</span><span class="cx"> bool propagateLayoutOverflow = true;
</span><span class="cx"> do {
</span></span></pre></div>
<a id="trunkSourceWebCorerenderingRenderListItemh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/rendering/RenderListItem.h (175504 => 175505)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -34,6 +34,7 @@
</span><span class="cx"> class RenderListItem final : public RenderBlockFlow {
</span><span class="cx"> public:
</span><span class="cx"> RenderListItem(Element&, PassRef<RenderStyle>);
</span><ins>+ virtual ~RenderListItem();
</ins><span class="cx"> Element& element() const { return downcast<Element>(nodeForNonAnonymous()); }
</span><span class="cx">
</span><span class="cx"> int value() const { if (!m_isValueUpToDate) updateValueNow(); return m_value; }
</span><span class="lines">@@ -55,13 +56,13 @@
</span><span class="cx"> static void updateItemValuesForOrderedList(const HTMLOListElement*);
</span><span class="cx"> static unsigned itemCountForOrderedList(const HTMLOListElement*);
</span><span class="cx">
</span><ins>+ void didDestroyListMarker() { m_marker = nullptr; }
+
</ins><span class="cx"> private:
</span><span class="cx"> virtual const char* renderName() const override { return "RenderListItem"; }
</span><span class="cx">
</span><span class="cx"> virtual bool isListItem() const override { return true; }
</span><span class="cx">
</span><del>- virtual void willBeDestroyed() override;
-
</del><span class="cx"> virtual void insertedIntoTree() override;
</span><span class="cx"> virtual void willBeRemovedFromTree() override;
</span><span class="cx">
</span><span class="lines">@@ -85,7 +86,7 @@
</span><span class="cx"> void explicitValueChanged();
</span><span class="cx">
</span><span class="cx"> int m_explicitValue;
</span><del>- RenderPtr<RenderListMarker> m_marker;
</del><ins>+ RenderListMarker* m_marker;
</ins><span class="cx"> mutable int m_value;
</span><span class="cx">
</span><span class="cx"> bool m_hasExplicitValue : 1;
</span></span></pre></div>
<a id="trunkSourceWebCorerenderingRenderListMarkercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/rendering/RenderListMarker.cpp (175504 => 175505)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1128,6 +1128,7 @@
</span><span class="cx">
</span><span class="cx"> RenderListMarker::~RenderListMarker()
</span><span class="cx"> {
</span><ins>+ m_listItem.didDestroyListMarker();
</ins><span class="cx"> if (m_image)
</span><span class="cx"> m_image->removeClient(this);
</span><span class="cx"> }
</span></span></pre>
</div>
</div>
</body>
</html>