Bug 125366 - Add missing ENABLE(DRAG_SUPPORT) guards for DragData and its clients
Summary: Add missing ENABLE(DRAG_SUPPORT) guards for DragData and its clients
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:
Blocks: 125248
  Show dependency treegraph
 
Reported: 2013-12-06 14:08 PST by Brian Burg
Modified: 2015-01-17 20:39 PST (History)
1 user (show)

See Also:


Attachments
patch (21.08 KB, patch)
2013-12-06 17:51 PST, Brian Burg
burg: commit-queue-
Details | Formatted Diff | Diff
patch (14.54 KB, patch)
2013-12-06 18:02 PST, Brian Burg
no flags Details | Formatted Diff | Diff
patch (17.40 KB, patch)
2013-12-08 12:19 PST, Brian Burg
no flags Details | Formatted Diff | Diff
patch (13.29 KB, patch)
2013-12-10 18:36 PST, Brian Burg
kling: 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-06 14:08:03 PST
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.
Comment 1 Brian Burg 2013-12-06 17:51:28 PST
Created attachment 218640 [details]
patch
Comment 2 Brian Burg 2013-12-06 17:52:09 PST
Comment on attachment 218640 [details]
patch

bad patch.
Comment 3 Brian Burg 2013-12-06 18:02:02 PST
Created attachment 218641 [details]
patch
Comment 4 Darin Adler 2013-12-06 19:59:35 PST
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.
Comment 5 Brian Burg 2013-12-08 11:45:46 PST
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?
Comment 6 Brian Burg 2013-12-08 12:19:03 PST
Created attachment 218705 [details]
patch
Comment 7 Darin Adler 2013-12-09 10:41:53 PST
(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.
Comment 8 Darin Adler 2013-12-09 10:51:40 PST
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.
Comment 9 Brian Burg 2013-12-10 18:36:29 PST
Created attachment 218930 [details]
patch