It seems for the most part, our autotests fit pretty wel in the category "consumers of the WebKit API", so they should probably undergo the appropriate style checks.
Created attachment 103494 [details] Patch
Comment on attachment 103494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103494&action=review > Tools/ChangeLog:7 > + This essentially prevents Qt tests from being style-checked > + against the same rules that apply to WebCore (e.g. config.h). I think it's better to have them style-checked myself.
(In reply to comment #2) > (From update of attachment 103494 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103494&action=review > > > Tools/ChangeLog:7 > > + This essentially prevents Qt tests from being style-checked > > + against the same rules that apply to WebCore (e.g. config.h). > > I think it's better to have them style-checked myself. Hi Robert, I'm not sure to follow you. The point of this change is to allow things like what Benjamin suggested in https://bugs.webkit.org/show_bug.cgi?id=65237 In the case of these tests (apart for the special case of mime type sniffing), we are in the position of being consumers of the API provided by QtWebKit, so it seems to me that rules that enforce including config.h don't belong here. Maybe I should rephrase the ChangeLog message.
Created attachment 103596 [details] Patch
Comment on attachment 103596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103596&action=review You must also modify the tests accordingly. And you should add a new test for Source/WebKit/qt/tests/MIMESniffing. > Tools/Scripts/webkitpy/style/checker.py:137 > + ([# The MIMESniffing test tests the code directly rather than the API, > + # let's enable the include check for this one. > + "Source/WebKit/qt/tests/MIMESniffing"], > + ["+build/include"]), This list is order sensitive. Only the first path substring match is used. -> Source/WebKit/qt/tests/MIMESniffing will never match
Created attachment 103614 [details] Patch Benjamin: oops ! looks like I didn't read the comment up there properly.
Comment on attachment 103614 [details] Patch Good idea for a patch. I am glad those warning will disappear.
Comment on attachment 103614 [details] Patch Clearing flags on attachment: 103614 Committed r92847: <http://trac.webkit.org/changeset/92847>
All reviewed patches have been landed. Closing bug.
Reverted r92847 for reason: Broke check-webkit-style Committed r92857: <http://trac.webkit.org/changeset/92857>
Sorry guys, this broke the style elf, which is kind of a big deal -- I rolled it out.
Created attachment 103646 [details] Patch Sorry again about that guys. Since Benjamin mentioned the warnings happened in other places, I figured I'd add the WebKit2 qt tests in this.
(In reply to comment #3) > Hi Robert, I'm not sure to follow you. The point of this change is to allow things like what Benjamin suggested in https://bugs.webkit.org/show_bug.cgi?id=65237 In the case of these tests (apart for the special case of mime type sniffing), we are in the position of being consumers of the API provided by QtWebKit, so it seems to me that rules that enforce including config.h don't belong here. Maybe I should rephrase the ChangeLog message. What I meant was: "The style check keeps the code clean, which is a good thing." If noise like the warnings about config.h are annoying, maybe we should target those instead.
Comment on attachment 103646 [details] Patch rs=me
Comment on attachment 103646 [details] Patch Let's just hold it for now. I still need to check if that's not messing with alphabetical sorting thingy that was brought up earlier.
Is this bug still valid?
(In reply to comment #16) > Is this bug still valid? Yes, it still is AFAICT, but it's more of a "nice to have" kind of thing though, and I never got back to it. I think I remember the concern raised by Robert turned out to be right and a rule specific to config.h should be introduced and only that should be disabled for autotests.
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.