The DragAndDropHelper returns a new allocated DragData object in several methods, while the only thing we need is the platformData (DataObjectGtk). So we could just return a pointer and create the DragData object in the stack
Created attachment 145013 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 145013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145013&action=review Hrm. I'm not sure if I prefer the new approach. It does avoid an allocation on the heap, but replaces it with an allocation on the stack which is arguably riskier and with the result of creating more code. Additionally this moves from two places in the code having to create DragData to six. I worry also that the use of position as an in and out parameter could be confusing and would prefer to simply have a getter on DragAndDropHelper. > Source/WebCore/platform/gtk/GtkDragAndDropHelper.cpp:161 > - return adoptPtr(static_cast<DragData*>(0)); > + return 0; > I'm not sure what I was thinking when I wrote this code. This should be return nullptr here.
Comment on attachment 145013 [details] Patch An alternative approach would be to cache the DragData in DragData in DragAndDropHelper.
Look at how other ports use it, all of them create a DragData in the stack when needed, because I think it's thought as a temporary object.
(In reply to comment #3) > (From update of attachment 145013 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145013&action=review > > Hrm. I'm not sure if I prefer the new approach. It does avoid an allocation on the heap, but replaces it with an allocation on the stack which is arguably riskier and with the result of creating more code. Additionally this moves from two places in the code having to create DragData to six. > > I worry also that the use of position as an in and out parameter could be confusing and would prefer to simply have a getter on DragAndDropHelper. > That's a workaround to use the last motion position instead of the coords passed to drag-data-received, the caller has everything required to build a DragData.
DragData has a small size so I don't think there's any risk in allocate it on the stack, and it's what all other ports do after all.
(In reply to comment #7) > DragData has a small size so I don't think there's any risk in allocate it on the stack, and it's what all other ports do after all. It still seems like this is less clean than the old approach. An allocation on the heap isn't such a terrible thing, but even so it can be eliminated by simply caching the DragData in the DragAndDropHelper.
(In reply to comment #8) > (In reply to comment #7) > > DragData has a small size so I don't think there's any risk in allocate it on the stack, and it's what all other ports do after all. > > It still seems like this is less clean than the old approach. An allocation on the heap isn't such a terrible thing, but even so it can be eliminated by simply caching the DragData in the DragAndDropHelper. Well, I don't see this is less clean, and DragData looks like a temp object to me, so I prefer not to cache it at all.
Created attachment 177257 [details] Updated patch to apply on current git master Let's give this another try, I still think the code is simpler, more efficient and consistent with *all* other ports in both WebKit1 and WebKit2.
Committed r136494: <http://trac.webkit.org/changeset/136494>