Summary: | Fix more WTF header include paths in WebCore | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | gustavo, menard, mrobinson, ossy, pnormand, rakuco, webkit.review.bot, xan.lopez | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 75673 | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2012-03-19 17:36:35 PDT
Created attachment 132734 [details]
Patch
Comment on attachment 132734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132734&action=review Looks good, though I think there are some style issues with the order of includes, unless I'm mistaken about the whole thing. Perhaps you could fix them all before landing? > Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:26 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> Shouldn't this go at the bottom of the list now? > Source/WebCore/accessibility/gtk/WebKitAccessibleInterfaceText.cpp:37 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> Ditto. > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:27 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> Ditto. > Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp:28 > -#include "GOwnPtr.h" > + > +#include <wtf/gobject/GOwnPtr.h> I've been telling people not to put a newline between WebCore headers and system headers, but if it's not a problem I'll stop. :) > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:38 > #include <gst/pbutils/missing-plugins.h> > + > +#include <wtf/Noncopyable.h> Extra newline here > Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:25 > +#include <wtf/gobject/GOwnPtr.h> To the bottom. I'm not sure why the style bot isn't complaining about this... > Source/WebCore/platform/graphics/pango/FontPango.cpp:38 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> Ditto. > Source/WebCore/platform/gtk/ContextMenuGtk.cpp:23 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> Ditto. > Source/WebCore/platform/gtk/FileSystemGtk.cpp:26 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> Ditto. > Source/WebCore/platform/gtk/GtkPopupMenu.cpp:30 > -#include "GOwnPtr.h" > +#include <wtf/gobject/GOwnPtr.h> I guess all of these need to be fixed? > Source/WebCore/platform/text/gtk/TextCodecGtk.h:33 > #include <glib.h> Looks like this one was out of order to begin with. Might be a good idea to fix it now. Comment on attachment 132734 [details]
Patch
Honestly, my belief is that if it's not a rule which the style-bot catches, than it's not a rule we should bother ourselves with. :) Trying to make every reviewer guess the exact include formatting seems silly. I don't know if there should be a newline between system and local includes, or where specifically <wtf> includes go in the ordering. But if the style-bot isn't complaingin how we'd like it to, we need to file bugs and fix them. Otherwise we're just wasting our breath in these bugs. :)
Comment on attachment 132734 [details] Patch Clearing flags on attachment: 132734 Committed r111354: <http://trac.webkit.org/changeset/111354> All reviewed patches have been landed. Closing bug. (In reply to comment #3) > (From update of attachment 132734 [details]) > Honestly, my belief is that if it's not a rule which the style-bot catches, than it's not a rule we should bother ourselves with. :) Trying to make every reviewer guess the exact include formatting seems silly. I don't know if there should be a newline between system and local includes, or where specifically <wtf> includes go in the ordering. But if the style-bot isn't complaingin how we'd like it to, we need to file bugs and fix them. Otherwise we're just wasting our breath in these bugs. :) I definitely hear what you are saying and I didn't mean to be a wet rag on an otherwise great patch over a dumb style issue. It's a pity that the style bot didn't complain though, because there is some pretty specific language at http://www.webkit.org/coding/coding-style.html about the order of includes. Personally I don't think it's that important, but perhaps we should either remove the rules from the page or follow them everywhere. Specifically I'm referring to: "Other #include statements should be in sorted order (case sensitive, as done by the command-line sort tool or the Xcode sort selection command). Don't bother to organize them in a logical order." and "Includes of system headers must come after includes of other headers." I agree. But we could also quibble about what qualifies as a "system" header, and (do all "system" <> style includes consistute "system headers") and what the exact sort order is (capitals before lower case, etc.). As we both agree, check-webkit-style should enforce this sort of thing. :) (In reply to comment #7) > I agree. But we could also quibble about what qualifies as a "system" header, and (do all "system" <> style includes consistute "system headers") and what the exact sort order is (capitals before lower case, etc.). As we both agree, check-webkit-style should enforce this sort of thing. :) It seems the style guide isn't clear enough. |