Bug 118524 – [GTK] createDragImageForLink is not implemented on Gtk

• Component: WebKitGTK
• Status: UNCONFIRMED

[Sanghyun Park]

createDragImageForLink function is not implemented in Gtk port.
I will upload the patch as soon as possible

[Sanghyun Park]

Created attachment 206524 [details]
Patch

[Sanghyun Park]

Created attachment 206645 [details]
Patch

[Martin Robinson]

Hrm. Chromium doesn't use a drag image for links and Firefox uses the actual DOM node. For the moment it seems that not having a drag image for links isn't a huge problem. I'm curious what other GTK+ port reviewers think...

[Sanghyun Park]

(In reply to comment #3)
> Hrm. Chromium doesn't use a drag image for links and Firefox uses the actual DOM node. For the moment it seems that not having a drag image for links isn't a huge problem. I'm curious what other GTK+ port reviewers think...

Thanks for your kind comment.
But, in Chromium (Version 30.0.1560.0 canary), drag image for links is implemented. 
And mac port also use a drag image for links.
I agree to your opinion, drag image for links isn't a huge issue. 
Although this is not a big problem, I thinks that it's better to support the drag image for links in GTK+ port.

[Sanghyun Park]

Dear GTK+ port reviewers.
Please tell me your opinion about this patch.

[Michael Catanzaro]

Comment on attachment 206645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206645&action=review

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.

> Source/WebCore/ChangeLog:10
> +        No new tests. No change in behavior.

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.

> Source/WebCore/platform/gtk/DragImageGtk.cpp:2
>   * 2010 Igalia S.L

Please add Copyright (C) for this Igalia line as well; it's obviously a mistake that it's missing.

> Source/WebCore/platform/gtk/DragImageGtk.cpp:110
> +Font* dragLabelFont(int size, bool bold)

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.

> Source/WebCore/platform/gtk/DragImageGtk.cpp:117
> +        g_object_get(settings, "gtk-font-name", &fontName, NULL);

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.

> Source/WebCore/platform/gtk/DragImageGtk.cpp:126
> +    description.setComputedSize((float)size);

Why are these casts necessary? Surely the int would be promoted to a float?

> Source/WebCore/platform/gtk/DragImageGtk.cpp:128
> +    Font* result = new Font(description, 0, 0);

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.

> Source/WebCore/platform/gtk/DragImageGtk.cpp:133
> +void doDrawTextAtPoint(cairo_t* context, const String& text, const IntPoint& point, const Font& font, const Color& color)

static void

> 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)

static void

> Source/WebCore/platform/gtk/DragImageGtk.cpp:154
> +DragImageRef createDragImageForLink(KURL& url, const String& inLabel, FontRenderingMode)

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.

> Source/WebCore/platform/gtk/DragImageGtk.cpp:194
> +    cairo_surface_t* image = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, imageSize.width(), imageSize.height());

Use a RefPtr<cairo_surface_t>