155445 2016-03-14 10:54:46 -0700 check-webkit-style: should warn about blank lines after #include "config.h" in TestWebKitAPI 2016-03-16 15:06:31 -0700 1 1 1 Unclassified WebKit Tools / Tests Other Unspecified Unspecified RESOLVED FIXED P2 Normal --- 155394 1 ddkilzer ddkilzer ap bfulgham commit-queue darin dbates glenn lforschler oldest_to_newest 1174533 0 ddkilzer 2016-03-14 10:54:46 -0700 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. 1174535 1 ddkilzer 2016-03-14 10:56:58 -0700 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 1174552 2 ddkilzer 2016-03-14 11:24:04 -0700 (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> 1174557 3 ddkilzer 2016-03-14 11:25:35 -0700 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. 1174574 4 ddkilzer 2016-03-14 11:43:01 -0700 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 1174633 5 ddkilzer 2016-03-14 12:58:38 -0700 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. 1174636 6 274007 ddkilzer 2016-03-14 13:01:48 -0700 Created attachment 274007 Patch v1 1175522 7 274007 commit-queue 2016-03-16 15:06:27 -0700 Comment on attachment 274007 Patch v1 Clearing flags on attachment: 274007 Committed r198304: <http://trac.webkit.org/changeset/198304> 1175523 8 commit-queue 2016-03-16 15:06:31 -0700 All reviewed patches have been landed. Closing bug. 274007 2016-03-14 13:01:48 -0700 2016-03-16 15:06:27 -0700 Patch v1 bug-155445-20160314130137.patch text/plain 3603 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMTk4MTQyCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggNDFjODdlZDczMGYyMjMzODhlZGZlOWQ5MjQ3M2EzNmVh YjUyMzMxZi4uNDNjZGY4YjQ1MzU5Yjk4N2JjYzRlMDljNzA5YWY3YWM3NzBlNWMxYSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIy IEBACiAyMDE2LTAzLTE0ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CiAKKyAg ICAgICAgY2hlY2std2Via2l0LXN0eWxlOiBzaG91bGQgd2FybiBhYm91dCBibGFuayBsaW5lcyBh ZnRlciAjaW5jbHVkZSAiY29uZmlnLmgiIGluIFRlc3RXZWJLaXRBUEkKKyAgICAgICAgPGh0dHA6 Ly93ZWJraXQub3JnL2IvMTU1NDQ1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2VyLnB5OgorICAg ICAgICAoX1BBVEhfUlVMRVNfU1BFQ0lGSUVSKTogRG8gbm90IGlnbm9yZSAiYnVpbGQvaW5jbHVk ZSoiIGNoZWNrcworICAgICAgICBvbiBUZXN0V2ViS2l0QVBJIHNpbmNlIHRoaXMgcHJvamVjdCB1 c2VzIGEgY29uZmlnLmggaGVhZGVyIGFzIG9mCisgICAgICAgIHI5NTE4OC4gIEFsc28gcmVtb3Zl IHJlZmVyZW5jZXMgdG8gV2ViS2l0QVBJVGVzdCwgd2hpY2ggY2Vhc2VkCisgICAgICAgIHRvIGV4 aXN0IGluIHI5NTk0NC4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tl cl91bml0dGVzdC5weToKKyAgICAgICAgKEdsb2JhbFZhcmlhYmxlc1Rlc3QudGVzdF9wYXRoX3J1 bGVzX3NwZWNpZmllcik6IEFkZCB0ZXN0cy4KKyAgICAgICAgUmVtb3ZlIHJlZmVyZW5jZSB0byBX ZWJLaXRBUElUZXN0IHByb2plY3QuCisKKzIwMTYtMDMtMTQgIERhdmlkIEtpbHplciAgPGRka2ls emVyQGFwcGxlLmNvbT4KKwogICAgICAgICBSZW1vdmUgYmxhbmsgbGluZXMgYWZ0ZXIgI2luY2x1 ZGUgImNvbmZpZy5oIgogCiAgICAgICAgIEZvbGxvdy11cCBmaXggZnJvbSByZXZpZXcgY29tbWVu dHMgb24gQnVnIDE1NTM5NC4KZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5 bGUvY2hlY2tlci5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlci5weQpp bmRleCA4ZGRiMTQ1Y2YzN2ZlOTY5M2IyN2U2MWZlNzFkNGMxMzQwOTEzODc4Li5mOWYyMDE1ZDVk NjAyOThiYjZiODA0NWYxMDE0N2QzZjM5MWM4Y2Q5IDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRz L3dlYmtpdHB5L3N0eWxlL2NoZWNrZXIucHkKKysrIGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9z dHlsZS9jaGVja2VyLnB5CkBAIC0xMzYsMTUgKzEzNiwxMyBAQCBfUEFUSF9SVUxFU19TUEVDSUZJ RVIgPSBbCiAKICAgICAoWyMgVGVzdE5ldHNjYXBlUGx1Z0luIGhhcyBubyBjb25maWcuaCBhbmQg dXNlcyBmdW5ueSBuYW1lcyBsaWtlCiAgICAgICAjIE5QUF9TZXRXaW5kb3cuCi0gICAgICBvcy5w YXRoLmpvaW4oJ1Rvb2xzJywgJ0R1bXBSZW5kZXJUcmVlJywgJ1Rlc3ROZXRzY2FwZVBsdWdJbicp LAotICAgICAgIyBUaGUgQVBJIHRlc3QgaGFybmVzc2VzIGhhdmUgbm8gY29uZmlnLmggYW5kIHVz ZSBmdW5ueSBtYWNyb3MgbGlrZQotICAgICAgIyBURVNUX0NMQVNTX05BTUUuCi0gICAgICBvcy5w YXRoLmpvaW4oJ1Rvb2xzJywgJ1dlYktpdEFQSVRlc3QnKSwKLSAgICAgIG9zLnBhdGguam9pbign VG9vbHMnLCAnVGVzdFdlYktpdEFQSScpXSwKKyAgICAgIG9zLnBhdGguam9pbignVG9vbHMnLCAn RHVtcFJlbmRlclRyZWUnLCAnVGVzdE5ldHNjYXBlUGx1Z0luJyldLAogICAgICBbIi1idWlsZC9p bmNsdWRlIiwKICAgICAgICItcmVhZGFiaWxpdHkvbmFtaW5nIl0pLAogICAgIChbIyBUaGVyZSBp cyBubyBjbGVhbiB3YXkgdG8gYXZvaWQgInl5XyoiIG5hbWVzIHVzZWQgYnkgZmxleC4KLSAgICAg IG9zLnBhdGguam9pbignU291cmNlJywgJ1dlYkNvcmUnLCAnY3NzJywgJ0NTU1BhcnNlci5jcHAn KV0sCisgICAgICBvcy5wYXRoLmpvaW4oJ1NvdXJjZScsICdXZWJDb3JlJywgJ2NzcycsICdDU1NQ YXJzZXIuY3BwJyksCisgICAgICAjIFRlc3RXZWJLaXRBUEkgdXNlcyBmdW5ueSBtYWNyb3MgbGlr ZSBFWFBFQ1RfV0tfU1RSRVEuCisgICAgICBvcy5wYXRoLmpvaW4oJ1Rvb2xzJywgJ1Rlc3RXZWJL aXRBUEknKV0sCiAgICAgIFsiLXJlYWRhYmlsaXR5L25hbWluZyJdKSwKIAogICAgIChbIyBUaGUg R1RLKyBBUElzIHVzZSBHVEsrIG5hbWluZyBzdHlsZSwgd2hpY2ggaW5jbHVkZXMKZGlmZiAtLWdp dCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcl91bml0dGVzdC5weSBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcl91bml0dGVzdC5weQppbmRleCA2OGI4 N2Y1MTUzNmI0ZTY3MTc2Y2Y2MTU3N2RhNjFmYjRiMjdiODRiLi44MGVhMDg1OThiOWNjYjkyZGNi YmJmOWM3NTJmYjlkNjA0Y2ZmODEyIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L3N0eWxlL2NoZWNrZXJfdW5pdHRlc3QucHkKKysrIGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9z dHlsZS9jaGVja2VyX3VuaXR0ZXN0LnB5CkBAIC0yMTMsOCArMjEzLDE0IEBAIGNsYXNzIEdsb2Jh bFZhcmlhYmxlc1Rlc3QodW5pdHRlc3QuVGVzdENhc2UpOgogCiAgICAgICAgIGFzc2VydENoZWNr KCJyYW5kb21fcGF0aC5jcHAiLAogICAgICAgICAgICAgICAgICAgICAiYnVpbGQvaW5jbHVkZSIp Ci0gICAgICAgIGFzc2VydE5vQ2hlY2sob3MucGF0aC5qb2luKCdUb29scycsICdXZWJLaXRBUElU ZXN0JywgJ21haW4uY3BwJyksCisgICAgICAgIGFzc2VydE5vQ2hlY2sob3MucGF0aC5qb2luKCdU b29scycsICdEdW1wUmVuZGVyVHJlZScsICdUZXN0TmV0c2NhcGVQbHVnSW4nLCAnbWFpbi5jcHAn KSwKICAgICAgICAgICAgICAgICAgICAgICAiYnVpbGQvaW5jbHVkZSIpCisgICAgICAgIGFzc2Vy dE5vQ2hlY2sob3MucGF0aC5qb2luKCdUb29scycsICdEdW1wUmVuZGVyVHJlZScsICdUZXN0TmV0 c2NhcGVQbHVnSW4nLCAnbWFpbi5jcHAnKSwKKyAgICAgICAgICAgICAgICAgICAgICAicmVhZGFi aWxpdHkvbmFtaW5nIikKKyAgICAgICAgYXNzZXJ0Q2hlY2sob3MucGF0aC5qb2luKCdUb29scycs ICdUZXN0V2ViS2l0QVBJJywgJ1Rlc3RzJywgJ1dURicsICdSZWZMb2dnZXIuY3BwJyksCisgICAg ICAgICAgICAgICAgICAgICAgImJ1aWxkL2luY2x1ZGUiKQorICAgICAgICBhc3NlcnROb0NoZWNr KG9zLnBhdGguam9pbignVG9vbHMnLCAnVGVzdFdlYktpdEFQSScsICdUZXN0cycsICdtYWMnLCAn V2lsbFNlbmRTdWJtaXRFdmVudC5tbScpLAorICAgICAgICAgICAgICAgICAgICAgICJyZWFkYWJp bGl0eS9uYW1pbmciKQogICAgICAgICBhc3NlcnRDaGVjaygicmFuZG9tX3BhdGguY3BwIiwKICAg ICAgICAgICAgICAgICAgICAgInJlYWRhYmlsaXR5L25hbWluZyIpCiAgICAgICAgIGFzc2VydE5v Q2hlY2sob3MucGF0aC5qb2luKCdTb3VyY2UnLCAnV2ViS2l0JywgJ2d0aycsICd3ZWJraXQnLCAn d2Via2l0LmgnKSwK