Bug 48322

Summary: Enable check-webkit-style on Qt files
Product: WebKit Reporter: Ademar Reis <ademar>
Component: Tools / TestsAssignee: Ademar Reis <ademar>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, jedrzej.nowacki, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 48258    
Bug Blocks:    
Attachments:
Description Flags
patch
levin: review-, levin: commit-queue-
patch #2 with review fixes none

Ademar Reis
Reported 2010-10-26 07:04:00 PDT
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.
Attachments
patch (6.53 KB, patch)
2010-10-26 07:12 PDT, Ademar Reis 		levin: review-
levin: commit-queue-
DetailsFormatted DiffDiff
patch #2 with review fixes (5.37 KB, patch)
2010-10-27 07:39 PDT, Ademar Reis 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ademar Reis
Comment 1 2010-10-26 07:12:33 PDT
Created attachment 71876 [details] patch
Ademar Reis
Comment 2 2010-10-26 07:14:42 PDT
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 3 2010-10-26 21:29:11 PDT
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 4 2010-10-27 07:38:49 PDT
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
Comment 5 2010-10-27 07:39:54 PDT
Created attachment 72033 [details] patch #2 with review fixes
David Levin
Comment 6 2010-11-02 13:53:53 PDT
*** Bug 35143 has been marked as a duplicate of this bug. ***
Shinichiro Hamaji
Comment 7 2010-11-11 22:20:49 PST
Comment on attachment 72033 [details] patch #2 with review fixes Looks good.
WebKit Commit Bot
Comment 8 2010-11-11 22:51:32 PST
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.
WebKit Commit Bot
Comment 9 2010-11-11 23:21:33 PST
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
Comment 10 2010-11-11 23:21:38 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 11 2010-11-11 23:51:45 PST
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
Note You need to log in before you can comment on or make changes to this bug.