Bug 81602

Summary: Fix more WTF header include paths in WebCore
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: 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 Flags
Patch none

Description Eric Seidel (no email) 2012-03-19 17:36:35 PDT
Fix more WTF header include paths in WebCore
Comment 1 Eric Seidel (no email) 2012-03-19 17:38:43 PDT
Created attachment 132734 [details]
Patch
Comment 2 Martin Robinson 2012-03-19 18:58:40 PDT
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 3 Eric Seidel (no email) 2012-03-19 19:18:42 PDT
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 4 WebKit Review Bot 2012-03-19 22:05:16 PDT
Comment on attachment 132734 [details]
Patch

Clearing flags on attachment: 132734

Committed r111354: <http://trac.webkit.org/changeset/111354>
Comment 5 WebKit Review Bot 2012-03-19 22:05:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Martin Robinson 2012-03-19 22:12:23 PDT
(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."
Comment 7 Eric Seidel (no email) 2012-03-19 22:19:24 PDT
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.  :)
Comment 8 Martin Robinson 2012-03-19 22:27:14 PDT
(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.