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.
Created attachment 71876 [details] patch
Qt code implements a few operator+= and operator-=, so marking this bug as dependant of bug 48258 (there's a patch there)
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).
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.
Created attachment 72033 [details] patch #2 with review fixes
*** Bug 35143 has been marked as a duplicate of this bug. ***
Comment on attachment 72033 [details] patch #2 with review fixes Looks good.
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 ap@webkit.org and cmarrin@apple.com. The commit-queue is continuing to process your patch.
Comment on attachment 72033 [details] patch #2 with review fixes Clearing flags on attachment: 72033 Committed r71894: <http://trac.webkit.org/changeset/71894>
All reviewed patches have been landed. Closing bug.
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