[webkit-reviews] review denied: [Bug 125367] Add missing ENABLE(DRAG_SUPPORT) guards for DragClient and clients : [Attachment 218712] 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 125367: Add missing ENABLE(DRAG_SUPPORT) guards for DragClient and clients
https://bugs.webkit.org/show_bug.cgi?id=125367

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

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