Summary: | Add missing ENABLE(DRAG_SUPPORT) guards for DragData and its clients | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||||||
Component: | Images | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||
Severity: | Normal | CC: | darin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 125248 | ||||||||||||
Attachments: |
|
Description
Brian Burg
2013-12-06 14:08:03 PST
Created attachment 218640 [details]
patch
Comment on attachment 218640 [details]
patch
bad patch.
Created attachment 218641 [details]
patch
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. 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? Created attachment 218705 [details]
patch
(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 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. Created attachment 218930 [details]
patch
|