[webkit-reviews] review denied: [Bug 125248] Missing ENABLE(DRAG_SUPPORT) guards for drag-specific DragImage functions : [Attachment 218713] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brian Burg <burg at cs.washington.edu>'s
request for review:
Bug 125248: Missing ENABLE(DRAG_SUPPORT) guards for drag-specific DragImage
functions
https://bugs.webkit.org/show_bug.cgi?id=125248

Attachment 218713: patch
https://bugs.webkit.org/attachment.cgi?id=218713&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.