[Webkit-unassigned] [Bug 125248] Missing ENABLE(DRAG_SUPPORT) guards for drag-specific DragImage functions

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #218713|review?                     |review-
               Flag|                            |




--- Comment #6 from Darin Adler <darin at apple.com>  2013-12-09 10:58:42 PST ---
(From update of attachment 218713)
View in context: https://bugs.webkit.org/attachment.cgi?id=218713&action=review

> Source/WebCore/platform/DragImage.cpp:186
> +#if ENABLE(DRAG_SUPPORT)
>  DragImageRef createDragImageForImage(Frame& frame, Node& node, IntRect& imageRect, IntRect& elementRect)

The #endif that balances this has a blank line before it. Normally it’s best to be symmetric with such things so there should  be a blank line here after the #if.

> Source/WebCore/platform/DragImage.h:69
> +IntSize dragImageSize(DragImageRef);

The dragImageSize function is a basic getter and should be in a separate paragraph before the rest of the functions, as it is today. While it’s OK to conditionalize it, it harms the clarity of this header to move it into an arbitrary position in the middle of the functions.

> Source/WebCore/platform/DragImage.h:70
> +void deleteDragImage(DragImageRef);

It’s bad presentation to have the deleteDragImage function, needed to handle the basic lifetime of DragImageRef, mixed in the middle of the rest of the functions. It should go somewhere else, preferably earlier in the file.

The deleteDragImage function is needed even without ENABLE(DRAG_SUPPORT); every call to createDragImage needs to be balanced by a call to deleteDragImage, except on platforms where it’s an empty function like Mac. It may be that this seems to be not needed when ENABLE(DRAG_SUPPORT) is off just because the other code using these images is Mac platform-specific code and thus doesn’t call deleteDragImage. Just a guess.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.