Enable check-webkit-style on Qt files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ademar Reis]

As discussed in the mailing list[1] and documented in the wiki[2], we should enable check-webkit-style on Qt files.

1. https://lists.webkit.org/pipermail/webkit-qt/2010-October/000908.html
2. http://trac.webkit.org/wiki/QtWebKitContrib

Patch is on the way.

[Ademar Reis]

Created attachment 71876 [details]
patch

[Ademar Reis]

Qt code implements a few operator+= and operator-=, so marking this bug as dependant of bug 48258 (there's a patch there)

[David Levin]

Comment on attachment 71876 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71876&action=review

Just a few minor comments.

> WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2517
> +                and filename.find('/qt/') < 0

All the hardcoded checks here are for items that there isn't a generic mechanism for.

There is a generic mechanism for excluding directories. You'll have to list each directory but there aren't that many.

See _PATH_RULES_SPECIFIER in WebKitTools/Scripts/webkitpy/style/checker.py

> WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py:3722
> +        self.assert_lint('QWhatever * d_ptr;', '', 'WebKit/qt/whatever.cpp')

I'm mildly confused. Why is there a space between * and QWhatever? I thought this was  testing that the name d_ptr was let through (not that spacing around * is allowed).

[Ademar Reis]

Comment on attachment 71876 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71876&action=review

Thanks for the review!

>> WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2517
>> +                and filename.find('/qt/') < 0
> 
> All the hardcoded checks here are for items that there isn't a generic mechanism for.
> 
> There is a generic mechanism for excluding directories. You'll have to list each directory but there aren't that many.
> 
> See _PATH_RULES_SPECIFIER in WebKitTools/Scripts/webkitpy/style/checker.py

Got it. Fixing.

I still would like to have these files checked against the "don't use the single letter 'l' as an identifier" rule, which is part of readability/naming... maybe split the rule in two? something for a future patch anyway.

>> WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py:3722
>> +        self.assert_lint('QWhatever * d_ptr;', '', 'WebKit/qt/whatever.cpp')
> 
> I'm mildly confused. Why is there a space between * and QWhatever? I thought this was  testing that the name d_ptr was let through (not that spacing around * is allowed).

Ouch, that was a typo. Anyway, these changes in checkers/cpp* are not needed anymore since the directories are now properly skipped.

[Ademar Reis]

Created attachment 72033 [details]
patch #2 with review fixes

[David Levin]

*** Bug 35143 has been marked as a duplicate of this bug. ***

[Shinichiro Hamaji]

Comment on attachment 72033 [details]
patch #2 with review fixes

Looks good.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 72033 [details]:

animations/suspend-resume-animation.html
http/tests/appcache/remove-cache.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 72033 [details]
patch #2 with review fixes

Clearing flags on attachment: 72033

Committed r71894: <http://trac.webkit.org/changeset/71894>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71894 might have broken Qt Linux Release
The following tests are not passing:
svg/dom/SVGScriptElement/script-clone-rerun-self.svg
svg/dom/SVGScriptElement/script-clone-rerun.svg