Bug 115567 - check-webkit-style should complain about a layering violation if platform-specific guards are used in WebCore outside of the platform directory
Summary: check-webkit-style should complain about a layering violation if platform-spe...
Status: CLOSED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jessie Berlin
URL:
Keywords:
Depends on: 116016
Blocks:
  Show dependency tree / graph
 
Reported: 2013-05-03 11:48 PDT by Jessie Berlin
Modified: 2013-05-21 11:15 PDT (History)
8 users (show)

See Also:

Attachments
Patch (5.12 KB, patch)
2013-05-03 12:25 PDT, Jessie Berlin 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2013-05-03 11:48:40 PDT 
This should help in discouraging folks from adding more layering violations between WebCore and WebCore/platform.
Comment 1 Jessie Berlin 2013-05-03 12:25:31 PDT 
Created attachment 200459 [details]
Patch
Comment 2 Benjamin Poulain 2013-05-03 16:10:57 PDT 
Comment on attachment 200459 [details]
Patch

Awesome idea!

I would have a couple more tests for:
-various spacing
-ifdef in a comment.
Comment 3 Jessie Berlin 2013-05-04 10:48:33 PDT 
(In reply to comment #2)
> (From update of attachment 200459 [details])
> Awesome idea!
> 
> I would have a couple more tests for:
> -various spacing

I added

        error_collector = ErrorCollector(self.assertTrue)
        self.process_file_data('Source/WebCore/loader/NavigationAction.cpp', 'cpp', ['#if PLATFORM ( MAC )', '#endif'], error_collector)
        self.assertEqual(1, error_collector.result_list().count(errmsg))

> -ifdef in a comment.

Not 100% sure what you mean by this. Do you mean

// #if PLATFORM(MAC)

?

And would you expect that to cause the style error to be thrown? I don’t think I would.
Comment 4 Jessie Berlin 2013-05-06 13:21:28 PDT 
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 200459 [details] [details])
> > Awesome idea!
> > 
> > I would have a couple more tests for:
> > -various spacing
> 
> I added
> 
>         error_collector = ErrorCollector(self.assertTrue)
>         self.process_file_data('Source/WebCore/loader/NavigationAction.cpp', 'cpp', ['#if PLATFORM ( MAC )', '#endif'], error_collector)
>         self.assertEqual(1, error_collector.result_list().count(errmsg))
> 
> > -ifdef in a comment.
> 
> Not 100% sure what you mean by this. Do you mean
> 
> // #if PLATFORM(MAC)
> 
> ?
> 
> And would you expect that to cause the style error to be thrown? I don’t think I would.

I double checked with Ben and I will add this test with the expectation that the style error will not be thrown in this case.
Comment 5 Jessie Berlin 2013-05-06 13:37:22 PDT 
Comment on attachment 200459 [details]
Patch

Committed in http://trac.webkit.org/changeset/149635
Comment 6 Csaba Osztrogonác 2013-05-13 01:58:30 PDT 
(In reply to comment #5)
> (From update of attachment 200459 [details])
> Committed in http://trac.webkit.org/changeset/149635

The new webkitpy test fails on the bots, fix is coming soon 
in https://bugs.webkit.org/show_bug.cgi?id=116016
Comment 7 Jessie Berlin 2013-05-21 11:15:55 PDT 
Rolled out in http://trac.webkit.org/changeset/150457, it doesn’t make sense to do this at this time.