<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_UNCONFIRMED "
title="UNCONFIRMED - [GTK] createDragImageForLink is not implemented on Gtk"
href="https://bugs.webkit.org/show_bug.cgi?id=118524">bug 118524</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 #206645 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [GTK] createDragImageForLink is not implemented on Gtk"
href="https://bugs.webkit.org/show_bug.cgi?id=118524#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_UNCONFIRMED "
title="UNCONFIRMED - [GTK] createDragImageForLink is not implemented on Gtk"
href="https://bugs.webkit.org/show_bug.cgi?id=118524">bug 118524</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=206645&action=diff" name="attach_206645" title="Patch">attachment 206645</a> <a href="attachment.cgi?id=206645&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=206645&action=review">https://bugs.webkit.org/attachment.cgi?id=206645&action=review</a>
I agree with Sanghyun; this isn't an important feature, but it'd be nice polish to have. Thanks for working on this Sanghyun, and my apologies that it didn't get a proper review for so long.
<span class="quote">> Source/WebCore/ChangeLog:10
> + No new tests. No change in behavior.</span >
I agree you don't need to test this, since it would be quite difficult to test. But the patch obviously includes a functional behavior change. You can simply delete this line from the changelog.
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:2
> * 2010 Igalia S.L</span >
Please add Copyright (C) for this Igalia line as well; it's obviously a mistake that it's missing.
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:110
> +Font* dragLabelFont(int size, bool bold)</span >
Never pass ownership through a raw pointer; it makes it too hard to reason about memory. This should return a Ref<Font>. Also, the function should be marked static, to make it inaccessible outside this file.
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:117
> + g_object_get(settings, "gtk-font-name", &fontName, NULL);</span >
NULL -> nullptr
Anyway, this is OK, but it's not great. It would be better to use the same font or fonts as is actually used for the link. The Firefox drag image, for example, looks exactly the same as the link being dragged.
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:126
> + description.setComputedSize((float)size);</span >
Why are these casts necessary? Surely the int would be promoted to a float?
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:128
> + Font* result = new Font(description, 0, 0);</span >
Nowadays we avoid naked new/delete. If the constructor were still public, it would be correct to use std::make_unique instead of new, and return a std::unique_ptr<Font>. But the constructor is no longer public; when you rebase this, you'll find that Fonts are ref-counted now, and you'll be forced to create the Font with Font::create, which returns a Ref<Font>. That's good; you can simply return a Ref<Font> from this function.
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:133
> +void doDrawTextAtPoint(cairo_t* context, const String& text, const IntPoint& point, const Font& font, const Color& color)</span >
static void
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:146
> +void drawDoubledTextAtPoint(cairo_t* context, const String& text, const IntPoint& point, const Font& font, const Color& topColor, const Color& bottomColor)</span >
static void
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:154
> +DragImageRef createDragImageForLink(KURL& url, const String& inLabel, FontRenderingMode)</span >
I think the return value is being leaked, here and for all other the pre-existing functions that return a DragImageRef. Or at least, I don't see where the memory is being freed. Surely the type of DragImageRef should be RefPtr<cairo_surface_t>, not cairo_surface_t*? Where does this memory get freed?
Not your fault, since this is a pre-existing problem, but we should understand this before we commit this patch.
This is one of the reasons we avoid transferring ownership through raw pointers. And ought to avoid tricky typedefs like this.
<span class="quote">> Source/WebCore/platform/gtk/DragImageGtk.cpp:194
> + cairo_surface_t* image = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, imageSize.width(), imageSize.height());</span >
Use a RefPtr<cairo_surface_t></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>