[Webkit-unassigned] [Bug 118524] [GTK] createDragImageForLink is not implemented on Gtk

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=118524

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #206645|review?                     |review-
              Flags|                            |

--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 206645
  --> https://bugs.webkit.org/attachment.cgi?id=206645
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>

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160107/c3e8b228/attachment-0001.html>