Bug 125367 – Add missing ENABLE(DRAG_SUPPORT) guards for DragClient and clients

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

[Brian Burg]

Continuing on our theme... These guards are done differently in every port for no reason. They should be done the same way everywhere so it's blindingly obvious what drag support implies.

[Brian Burg]

Created attachment 218707 [details]
patch

[EFL EWS Bot]

Comment on attachment 218707 [details]
patch

Attachment 218707 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/46838079

[Brian Burg]

Created attachment 218712 [details]
patch

[Darin Adler]

Comment on attachment 218712 [details]
patch

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

The cross-platform changes here are good; it’s a small refinement that costs very little to cut out even more of the drag code for platforms that have it turned off.

But the platform-specific part of this patch is overkill. It’s a mistake to add ENABLE(DRAG_SUPPORT) conditionals for every platform. Most platforms compile only with drag support enabled or only with drag support disabled. We don’t need each platform to support both conditions. Unfortunately I don’t know the status of the not-Apple-driven ports such as GTK, EFL, and WinCE.

> Source/WebCore/page/Page.cpp:1563
>      , dragClient(0)

Should be nullptr instead of 0.

> Source/WebKit/win/WebCoreSupport/WebDragClient.h:31
> +#if ENABLE(DRAG_SUPPORT)

No need to do this. Windows should always be compiled with DRAG_SUPPORT enabled.

> Source/WebKit/win/WebView.cpp:2770
> +#if ENABLE(DRAG_SUPPORT)

No need to do this. Windows should always be compiled with DRAG_SUPPORT enabled.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:191
> +#if ENABLE(DRAG_SUPPORT)
> +#include "WebDragClient.h"
> +#include <WebCore/DragController.h>
> +#include <WebCore/DragData.h>
> +#include <WebCore/DragSession.h>
> +#endif

This change isn’t needed. These headers themselves are already properly conditionalized and there is no harm in including them unconditionally.

[Brian Burg]

(In reply to comment #4)
> (From update of attachment 218712 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218712&action=review
> 
> The cross-platform changes here are good; it’s a small refinement that costs very little to cut out even more of the drag code for platforms that have it turned off.
> 
> But the platform-specific part of this patch is overkill.

OK, noted. I'll fix the other patches to heed this too.

[Brian Burg]

Created attachment 218931 [details]
patch

[Blaze Burg]

Not needed.