Bug 125366 – Add missing ENABLE(DRAG_SUPPORT) guards for DragData and its clients

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

[Brian Burg]

The mac port implementations for DragData are guarded by ENABLE(DRAG_SUPPORT), but the header and clients of DragData are not. This is simple to fix.

[Brian Burg]

Created attachment 218640 [details]
patch

[Brian Burg]

Comment on attachment 218640 [details]
patch

bad patch.

[Brian Burg]

Created attachment 218641 [details]
patch

[Darin Adler]

Comment on attachment 218641 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        consumer of all of these methods is DragControler, which is guarded.

Typo: DragControler with one l

> Source/WebCore/html/FileInputType.h:70
> +#if ENABLE(DRAG_SUPPORT)
>      virtual bool receiveDroppedFiles(const DragData&) OVERRIDE;
> +#endif

Would be nice to have this paragraphed separately instead of in the middle of unconditional functions.

> Source/WebCore/html/InputType.h:249
> +#if ENABLE(DRAG_SUPPORT)
>      // Should return true if the given DragData has more than one dropped files.
>      virtual bool receiveDroppedFiles(const DragData&);
> +#endif

Would be nice to have this paragraphed separately instead of in the middle of unconditional functions.

> Source/WebCore/platform/win/DragDataWin.cpp:255
> +

Extra blank line at the end of this file.

[Brian Burg]

Thanks for the look-over. Will upload style-fixed patch.

(In reply to comment #4)
> (From update of attachment 218641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218641&action=review
> 
> > Source/WebCore/platform/win/DragDataWin.cpp:255
> > +
> 
> Extra blank line at the end of this file.

Filed with check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=125424

BTW, are there guidelines for when to add guards to forward declarations of classes? I've seen some of the larger files like Frame, Page, and WebPage do that, but not many other files. Is this coming into vogue, going out of style, or just not widely applied?

[Brian Burg]

Created attachment 218705 [details]
patch

[Darin Adler]

(In reply to comment #5)
> BTW, are there guidelines for when to add guards to forward declarations of classes? I've seen some of the larger files like Frame, Page, and WebPage do that, but not many other files. Is this coming into vogue, going out of style, or just not widely applied?

We don’t have a consistent rule. I think I prefer not to guard forward declarations at all. The benefits of additional compile time errors don’t seem to outweigh the inelegance of the additional conditionals.

[Darin Adler]

Comment on attachment 218705 [details]
patch

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.

[Brian Burg]

Created attachment 218930 [details]
patch