Bug 140248

Summary: Add a script to check for platform layering violations
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140288    
Bug Blocks: 21354    
Attachments:
Description Flags
Patch
darin: review+
Current output none

Description Carlos Garcia Campos 2015-01-08 08:20:46 PST
I wrote a script to get all the platform layering violations, and I think it might be useful for others. We could just add it to Tools/Scripts for now so that it can be manually run. I think there are too many layering violations at the moment to run the script in the bots or as part of the style checker.
Comment 1 Carlos Garcia Campos 2015-01-08 08:24:01 PST
Created attachment 244259 [details]
Patch
Comment 2 Darin Adler 2015-01-08 08:31:24 PST
Could you post a copy of the current output of the script?
Comment 3 Carlos Garcia Campos 2015-01-08 08:35:35 PST
(In reply to comment #2)
> Could you post a copy of the current output of the script?

Sure, I'll attach it.
Comment 4 Carlos Garcia Campos 2015-01-08 08:36:54 PST
Created attachment 244261 [details]
Current output

I think there are a few false positives, that are not layering violations, but that should be fixed anyway using <> instead of "" for example.
Comment 5 Darin Adler 2015-01-08 09:01:38 PST
(In reply to comment #4)
> I think there are a few false positives, that are not layering violations,
> but that should be fixed anyway using <> instead of "" for example.

Before we start fixing the actual violations, I think it’s important to wipe out the false positives. Could you give an example of one where you think using <> is the right thing to do?
Comment 6 Darin Adler 2015-01-08 09:04:04 PST
OK, <stdio.h> clearly falls into that category.

But I’m more worried about the "TextCodec.cpp" one, for example. Why is that showing up as a violation?

I love the idea of the script, and I think I’ll say review+, but I really want this script in a place where its output is actionable as soon as possible, maybe even before you land it?
Comment 7 Carlos Garcia Campos 2015-01-08 09:34:55 PST
(In reply to comment #6)
> OK, <stdio.h> clearly falls into that category.

Yes, exactly.

> But I’m more worried about the "TextCodec.cpp" one, for example. Why is that
> showing up as a violation?

hmm, that's a bug in the script itself, I was assuming all includes are header files, I'll fix that.

> I love the idea of the script, and I think I’ll say review+, but I really
> want this script in a place where its output is actionable as soon as
> possible, maybe even before you land it?

Sure, I'll check and fix all the false positives before, thanks!
Comment 8 Carlos Garcia Campos 2015-01-09 03:48:35 PST
Committed r178167: <http://trac.webkit.org/changeset/178167>
Comment 9 Carlos Garcia Campos 2015-01-09 03:49:56 PST
(In reply to comment #8)
> Committed r178167: <http://trac.webkit.org/changeset/178167>

False positives were fixed in r178166 and I fixed the cpp includes before landing this.