Bug 125248 – Missing ENABLE(DRAG_SUPPORT) guards for drag-specific DragImage functions

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Images

[Brian Burg]

The platform-independent + PLATFORM(MAC) implementations are guarded by DRAG_SUPPORT but the header and other platforms are not. After or as part of renaming DragImage, https://bugs.webkit.org/show_bug.cgi?id=125200, it would be good to split the drag-related functions into a separate file that is guarded by DRAG_SUPPORT, or just remove the guard entirely.

[Darin Adler]

We should put the functions needed only for dragging inside ENABLE(DRAG_SUPPORT). Why not? Doesn’t seem to require separate source files.

[Brian Burg]

From looking at uses of functions, it seems that these are the functions not specific to drag functionality (i.e., used by DragController or guarded drag functionality in Clipboard).

DragImageRef createDragImageFromImage(Image*, ImageOrientationDescription);
DragImageRef createDragImageForNode(Frame&, Node&);
DragImageRef createDragImageForSelection(Frame&, bool forceBlackText = false);
DragImageRef createDragImageForRange(Frame&, Range&, bool forceBlackText = false);

So all other functions will be guarded with ENABLE(DRAG_SUPPORT). These tend to coincide with the platform-specific implementations, so the entire DragImageFoo.cpp file can have the guard.

[Brian Burg]

Created attachment 218708 [details]
patch

[Brian Burg]

Comment on attachment 218708 [details]
patch

Forgot to remove existing DragImage.cpp guards

[Brian Burg]

Created attachment 218713 [details]
patch

[Darin Adler]

Comment on attachment 218713 [details]
patch

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.

[Brian Burg]

Created attachment 218932 [details]
patch

[Darin Adler]

Comment on attachment 218932 [details]
patch

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

Change seems OK, but also seems that for platforms where DRAG_SUPPORT isn’t supported, we should leave out the functions for that entirely instead of having (#if DRAG_SUPPORT, return 0, #endif) placeholders that don’t do anything useful.

> Source/WebCore/platform/DragImage.h:66
>  // These functions should be memory neutral, eg. if they return a newly allocated image,
>  // they should release the input image. As a corollary these methods don't guarantee
>  // the input image ref will still be valid after they have been called.

This comment is only about the three functions that both take and return a DragImageRef, all of which are inside the #if. I think it’s peculiar to have it up here where it seems to apply to many other functions.

[Blaze Burg]

Not needed.