<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Get rid of DataObjectGtk::forClipboard and cleanup pasteboard code"
href="https://bugs.webkit.org/show_bug.cgi?id=161907">bug 161907</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #288687 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Get rid of DataObjectGtk::forClipboard and cleanup pasteboard code"
href="https://bugs.webkit.org/show_bug.cgi?id=161907#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Get rid of DataObjectGtk::forClipboard and cleanup pasteboard code"
href="https://bugs.webkit.org/show_bug.cgi?id=161907">bug 161907</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=288687&action=diff" name="attach_288687" title="Patch">attachment 288687</a> <a href="attachment.cgi?id=288687&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=288687&action=review">https://bugs.webkit.org/attachment.cgi?id=288687&action=review</a>
<span class="quote">> Source/WebCore/platform/gtk/PasteboardGtk.cpp:99
> -DataObjectGtk* Pasteboard::dataObject() const
> +const DataObjectGtk& Pasteboard::dataObject() const
> {
> - return m_dataObject.get();
> + return *m_dataObject;
> }</span >
You should also change m_dataObject to be a (non-const) reference, since it's always initialized in the constructor initializer lists and there are also ASSERTs to ensure it's not null. Then you can get rid of the ASSERTS, and don't have to dereference it throughout the file anymore.
<span class="quote">> Source/WebCore/platform/gtk/PasteboardHelper.cpp:311
> + // When gtk_clipboard_set_with_data fails the callbacks are ignored, so we need to leak the data we were passing to clearClipboardContentsCallback.
> + data.release();</span >
This confused me because I assumed this was on the failure path, because your comment starts with "when it fails," but in fact this is the success case. I see that you need to leak it only on success, so the code is right, just the comment is confusing.
<span class="quote">> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:69
> + DragData dragData(const_cast<DataObjectGtk*>(&dataObject), clientPosition, globalPosition, dataTransfer.sourceOperation());</span >
Wouldn't it be better to provide a non-const accessor as well, to avoid the const_cast here?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>