[webkit-reviews] review granted: [Bug 125366] Add missing ENABLE(DRAG_SUPPORT) guards for DragData and its clients : [Attachment 218705] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brian Burg <burg at cs.washington.edu>'s
request for review:
Bug 125366: Add missing ENABLE(DRAG_SUPPORT) guards for DragData and its
clients
https://bugs.webkit.org/show_bug.cgi?id=125366

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218705&action=review


> Source/WebCore/html/FileInputType.cpp:45
> +#if ENABLE(DRAG_SUPPORT)
> +#include "DragData.h"
> +#endif

I don’t think we need this change. Is the additional compile speed really worth
it?

> Source/WebCore/html/FileInputType.h:46
> +#if ENABLE(DRAG_SUPPORT)
> +class DragData;
> +#endif

I don’t think we need this change. See my recent comment for the rationale.

> Source/WebCore/html/HTMLInputElement.h:48
> +#if ENABLE(DRAG_SUPPORT)
> +class DragData;
> +#endif

No need for this change.

> Source/WebCore/html/InputType.h:67
> +#if ENABLE(DRAG_SUPPORT)
> +class DragData;
> +#endif

No need for this change.

> Source/WebCore/html/InputType.h:277
> +    // Should return true if the given DragData has more than one dropped
files.

Would be nice to fix the grammar here: “more than one dropped files” is not
correct English. Should be “more that one dropped file”.

> Source/WebCore/platform/DragData.cpp:29
> +#if ENABLE(DRAG_SUPPORT)

Change seems OK, but not very high value. Just a tiny compile time performance
boost for platforms that compile this file but don’t support drag.

> Source/WebCore/platform/blackberry/DragDataBlackBerry.cpp:22
> +#if ENABLE(DRAG_SUPPORT)

This is only needed if the BlackBerry platform wants to support compiling
without drag support. I’m not sure the platform wants that, so I’m not sure
this additional complexity should be added.

> Source/WebCore/platform/efl/DragDataEfl.cpp:24
> +#if ENABLE(DRAG_SUPPORT)

This is only needed if the EFL platform supports wants to support compiling
without drag support. I’m not sure the platform wants that, so I’m not sure
this additional complexity should be added.

> Source/WebCore/platform/gtk/DragDataGtk.cpp:20
> +#if ENABLE(DRAG_SUPPORT)

This is only needed if the GTK platform supports wants to support compiling
without drag support. I’m not sure the platform wants that, so I’m not sure
this additional complexity should be added.

> Source/WebCore/platform/nix/DragDataNix.cpp:29
> +#if ENABLE(DRAG_SUPPORT)

This is only needed if the NIX platform supports wants to support compiling
without drag support. I’m not sure the platform wants that, so I’m not sure
this additional complexity should be added.

> Source/WebCore/platform/win/DragDataWin.cpp:30
> +#if ENABLE(DRAG_SUPPORT)

This should not be added. We don’t need to support compiling the WIN platform
without drag support.