Bug 81602 – Fix more WTF header include paths in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Fix more WTF header include paths in WebCore

[Eric Seidel (no email)]

Created attachment 132734 [details]
Patch

[Martin Robinson]

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.

[Eric Seidel (no email)]

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. :)

[WebKit Review Bot]

Comment on attachment 132734 [details]
Patch

Clearing flags on attachment: 132734

Committed r111354: <http://trac.webkit.org/changeset/111354>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(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."

[Eric Seidel (no email)]

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.  :)

[Martin Robinson]

(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.