Bug 125248 - Missing ENABLE(DRAG_SUPPORT) guards for drag-specific DragImage functions
Summary: Missing ENABLE(DRAG_SUPPORT) guards for drag-specific DragImage functions
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 125366 125367 125369
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-04 13:11 PST by Brian Burg
Modified: 2017-02-10 17:34 PST (History)
3 users (show)

See Also:


Attachments
patch (17.81 KB, patch)
2013-12-08 12:53 PST, Brian Burg
no flags Details | Formatted Diff | Diff
patch (17.96 KB, patch)
2013-12-08 13:41 PST, Brian Burg
no flags Details | Formatted Diff | Diff
patch (18.79 KB, patch)
2013-12-10 18:41 PST, Brian Burg
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-12-04 13:11:05 PST
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.
Comment 1 Darin Adler 2013-12-04 18:08:12 PST
We should put the functions needed only for dragging inside ENABLE(DRAG_SUPPORT). Why not? Doesn’t seem to require separate source files.
Comment 2 Brian Burg 2013-12-06 10:59:00 PST
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.
Comment 3 Brian Burg 2013-12-08 12:53:37 PST
Created attachment 218708 [details]
patch
Comment 4 Brian Burg 2013-12-08 13:20:34 PST
Comment on attachment 218708 [details]
patch

Forgot to remove existing DragImage.cpp guards
Comment 5 Brian Burg 2013-12-08 13:41:27 PST
Created attachment 218713 [details]
patch
Comment 6 Darin Adler 2013-12-09 10:58:42 PST
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.
Comment 7 Brian Burg 2013-12-10 18:41:12 PST
Created attachment 218932 [details]
patch
Comment 8 Darin Adler 2013-12-11 14:36:00 PST
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.
Comment 9 Brian Burg 2017-02-10 17:34:15 PST
Not needed.