Bug 155445 – check-webkit-style: should warn about blank lines after #include "config.h" in TestWebKitAPI

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

In Bug 155394, I created a patch (that was auto-committed) that contained a blank line between these two lines in RefLogger.cpp:

#include "config.h"

#include "RefLogger.h"

However, this is against conventional WebKit style, so let's try to flag this in check-webkit-style so it can be spotted earlier and often-er.

[David Kilzer (:ddkilzer)]

For funsies, I wrote a command-line to find all the "violators" in Source:

$ find Source -type f -exec perl -e 'use File::Basename; undef $/; my $file = $ARGV[0]; my $base = basename($file); $base =~ s/\.[a-z]+$//; my $content = <>; print "$file\n" if ($content =~ m/#include "config.h"\n\n#include "${base}.h"/mg);' {} \;
Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp
Source/JavaScriptCore/runtime/CodeCache.cpp
Source/JavaScriptCore/runtime/PropertyDescriptor.cpp
Source/WTF/wtf/dtoa/bignum.cc
Source/WTF/wtf/dtoa/diy-fp.cc
Source/WTF/wtf/dtoa/fast-dtoa.cc
Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp
Source/WebCore/css/CSSBasicShapes.cpp
Source/WebCore/css/CSSGroupingRule.cpp
Source/WebCore/css/WebKitCSSRegionRule.cpp
Source/WebCore/dom/ChildListMutationScope.cpp
Source/WebCore/dom/MutationObserver.cpp
Source/WebCore/dom/MutationObserverInterestGroup.cpp
Source/WebCore/dom/MutationObserverRegistration.cpp
Source/WebCore/fileapi/BlobURL.cpp
Source/WebCore/fileapi/FileReader.cpp
Source/WebCore/fileapi/FileReaderLoader.cpp
Source/WebCore/fileapi/FileReaderSync.cpp
Source/WebCore/html/TimeRanges.cpp
Source/WebCore/html/canvas/CanvasContextAttributes.cpp
Source/WebCore/page/scrolling/ScrollingCoordinator.cpp
Source/WebCore/platform/CrossThreadCopier.cpp
Source/WebCore/platform/FileStream.cpp
Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp
Source/WebCore/platform/graphics/GraphicsLayer.cpp
Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
Source/WebCore/platform/graphics/ca/LayerFlushScheduler.cpp
Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp
Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp
Source/WebCore/platform/graphics/gpu/TilingData.cpp
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp
Source/WebCore/platform/graphics/win/MediaPlayerPrivateTaskTimer.cpp
Source/WebCore/platform/network/BlobResourceHandle.cpp
Source/WebCore/platform/network/FormData.cpp
Source/WebCore/platform/win/StructuredExceptionHandlerSuppressor.cpp
Source/WebCore/rendering/FilterEffectRenderer.cpp
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderLayerCompositor.cpp
Source/WebCore/rendering/RenderMarquee.cpp
Source/WebCore/rendering/RenderRuby.cpp
Source/WebCore/rendering/RenderRubyBase.cpp
Source/WebCore/rendering/RenderRubyRun.cpp
Source/WebCore/rendering/RenderRubyText.cpp
Source/WebCore/rendering/style/BasicShapes.cpp
Source/WebCore/svg/SVGZoomEvent.cpp
Source/WebCore/workers/DedicatedWorkerThread.cpp
Source/WebCore/workers/Worker.cpp
Source/WebCore/workers/WorkerScriptLoader.cpp
Source/WebCore/workers/WorkerThread.cpp
Source/WebKit2/NetworkProcess/NetworkLoad.cpp
Source/WebKit2/Shared/WebPopupItem.cpp

And in Tools:

$ find Tools -type f -exec perl -e 'use File::Basename; undef $/; my $file = $ARGV[0]; my $base = basename($file); $base =~ s/\.[a-z]+$//; my $content = <>; print "$file\n" if ($content =~ m/#include "config.h"\n\n#include "${base}.h"/mg);' {} \;
Tools/TestWebKitAPI/PlatformUtilities.cpp
Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> And in Tools:
> 
> $ find Tools -type f -exec perl -e 'use File::Basename; undef $/; my $file =
> $ARGV[0]; my $base = basename($file); $base =~ s/\.[a-z]+$//; my $content =
> <>; print "$file\n" if ($content =~ m/#include "config.h"\n\n#include
> "${base}.h"/mg);' {} \;
> Tools/TestWebKitAPI/PlatformUtilities.cpp
> Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp

Fixed in:

Committed r198141:  <http://trac.webkit.org/r198141>

[David Kilzer (:ddkilzer)]

So we do already have a check for this:

You should not add a blank line before implementation file's own header.  [build/include_order] [4]

Seems like the patch from Bug 155394 somehow missed the check, though.

[David Kilzer (:ddkilzer)]

Strangely, this only appears to fail in RefLogger.cpp:

Steps to reproduce:
1. git checkout bb9b34395effabdaeec2b92e42cee9df70aad271^
2. ./Tools/Scripts/check-webkit-style ./Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp

Expected results:
ERROR: Tools/TestWebKitAPI/Tests/WTF/RefLogger.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 1 files

Actual results:
Total errors found: 0 in 1 files

[David Kilzer (:ddkilzer)]

So it turns out that Tools/TestWebKitAPI was configured (in Tools/Scripts/webkitpy/style/checker.py, of course) to skip all warnings that start with 'build/include' because at the time this was apparently added, TestWebKitAPI didn't have its own config.h header.

As of r95188 in September 2011, though, Tools/TestWebKitAPI/config.h came into existence, but checker.py was never updated.

[David Kilzer (:ddkilzer)]

Created attachment 274007 [details]
Patch v1

[WebKit Commit Bot]

Comment on attachment 274007 [details]
Patch v1

Clearing flags on attachment: 274007

Committed r198304: <http://trac.webkit.org/changeset/198304>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.