<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/101653/">http://git.reviewboard.kde.org/r/101653/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Thomas Friedrichsmeier.</div>
<p style="color: grey;"><i>Updated Oct. 5, 2011, 7:28 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ok, I undid the deprecation of removeStatusBarItem(). In fact, using this actually makes sense in plugin destructors, since plugins may be destroyed during the lifetime of a KPart.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I guess this one needs some explaining...
Many KParts::Plugins appear to assume that widgets that they add to a KParts::StatusBarExtension (SE) will effectively be owned by SE, and will be deleted with the extension. E.g. the konqi W3 Validator plugin has this code:
void PluginValidators::removeStatusBarIcon()
{
if (!m_icon)
return;
m_statusBarExt = KParts::StatusBarExtension::childObject(m_part);
if (!m_statusBarExt)
return;
m_statusBarExt->removeStatusBarItem(m_icon);
delete m_icon;
m_icon = 0;
}
Note the return when no SE exists (any more). Checking for the existence of the SE is important, as it may become deleted before the plugins are deleted. In fact, at least for KHTMLPart that is the case. However, the assumption that the icon will have been deleted with the SE is wrong. It is still around. In konqueror this is not visible, because the part is always sent a de-activate event before destruction, and the SE hides the icons, then. In RKWard, parts typically do not receive a de-activate event before destruction, turning the memory leak into a visible icon leak (see this screenshot: http://reaktanz.de/stuff/R/RKWard_icon_soup.png ).
Fixing this in the plugin is simple: Only SE::removeStatusBarItem() would need to be guarded. But the problem is bound to re-surface in the next plugin. Further, I think the assumption that the widget will become owned by the SE is very reasonable.
----- The part of the description below was changed for Version 2 of the patch -----
Basically that is what this patch does, by deleting the widget in the destructor. Double deletion of the widget in both the StatusBarExtension and the Plugin should not be a problem in this solution. In the StatusBarExtension, the widget is already stored as a QPointer, and it is destroyed using deleteLater(), rather than immediate deletion.
Deprecating StatusBarExtension::removeStatusBarItem() may be somewhat controversial. However, while I could think of valid use-cases for this in theory, I firmly believe that none exists in practice. Rather all uses that I have seen are in conjunction with the manual deletion of the added widget / icon. The deprecation is intended to serve as a prompt for removing such - now obsolete - code.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kparts/statusbarextension.h <span style="color: grey">(69691d7f80c55a6193c345f7f514d35af2b20b15)</span></li>
<li>kparts/statusbarextension.cpp <span style="color: grey">(11fdf40681c708bc0f031797083c7f6b6b638f60)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101653/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>