Bug 25884 – WebKit needs a style linting tool

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

I spend a ridiculous amount of my review time picking on people's coding style. ;)  I'm sure other reviewers do to.

Google gets around this (somewhat) by using a custom linting tool.  Google's code style is different  (but not crazy different) from WebKit's.

Perhaps we could hack their linting tool into one which works for WK:
http://code.google.com/p/rietveld/source/browse/branches/chromium/codereview/cpplint.py

Or we could write our own.

CCing folks who tend to get interested in this kind of thing.

[David Kilzer (:ddkilzer)]

Or you could fix WebKitTools/Scripts/wkstyle from Bug 9671 and Bug 18397.

http://trac.webkit.org/changeset/17575
http://trac.webkit.org/changeset/31791

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/wkstyle

[Eric Seidel (no email)]

Last night I was pointed to:
http://uncrustify.sourceforge.net/
as another possible linting solution for WebKit.

[Shinichiro Hamaji]

Created attachment 32365 [details]
Patch for cpplint

[Shinichiro Hamaji]

Created attachment 32366 [details]
Results of lint for WebCore/*

[Shinichiro Hamaji]

I tried to modify cpplint.py so that it could be applied for WebKit code. Though I'm not sure if WebKit people like this, I attached a patch for http://google-styleguide.googlecode.com/svn/trunk/cpplint/ and the results of the lint tool for WebCore/* as an HTML to show what kind of style errors can be detected.

If WebKit is willing to accept my patch, I'll modify the patch as a patch for WebKitTools/Scripts and upload it. In this case, I'd like you to examine if you like the results of the lint tool. I guess there are several errors which WebKit developers don't care about (for example, I don't know if WebKit code should not have trailing white spaces). If you tell me this kind of errors, I'll remove checks for these errors.

If you want to check what kind of WebKit styles are checked, please see testWebKitRules function in cpplint_unittest.py in my patch. Though there are several FIXMEs, I think about half of WebKit style is covered.

As Eric mentioned, the original style isn't far different from the WebKit style. So my change isn't so big. Here is the list of changes in my patch:

- Disable check for the order of #include (TODO: we need to support this again using WebKit's rules)
- Allow "new (RenderArena)"
- Remove check for Google style TODO comments
- Disallow "while (something()) ;"
- Change the position of braces: for function definions brace needs its own line
- Allow "else {" and "} else"
- Disallow 2-space indentations
- Remove check for indentations of labels (TODO: we need to support this in WebKit's way)
- Remove check for the number of characters in a line
- Remove rules around C-style casts and sizeof
- Allow "short", "long", and "long long"
- Remove some Google specific stuff
- Remove check for variable length array
- Remove check for the style of ifndef guards
- Add unittests based on http://webkit.org/coding/coding-style.html

[Eric Seidel (no email)]

Does cpp_lint have an option to "fix" the errors it sees?  It would be interesting to start with a smaller set of files and drive the error to 0 (ideally using the tool to fix the files as we went).  As is there is too much noise in that output for it to be very useful.

[Mark Rowe (bdash)]

There are also a large number of false-positive warnings generated, and warnings for issues that are not part of the WebKit style.  Changing WebKit is clearly not the correct way to address the false-positives, but even for cases where the warnings are legitimate I don't think that generating warnings about code that is not at all related to a patch is desirable functionality.

[Eric Seidel (no email)]

Being able to apply the linter to a diff, instead of a whole file would be an awesome feature (maybe cpplint already supports this)?  I still think a linter which only knows how to lint against whole files would still be useful though.

We need to get rid of the false positives.  False positives are a show-stopper in my mind.

We need a straightforward path from our current source, to one which passes the linter 100%.  Whether that's by using tricks like a linter which knows how to restrict its errors to just a diff, or an automated script which can fix up the style of existing code (that we could run across sets of files at a time), I don't think it matters too much.

Thanks so much for taking a look at this Albert!

[Eric Seidel (no email)]

Sorry!  I forgot who I was talking to.   Shinichiro, not Albert!

[Albert J. Wong]

From the peanut gallery...

You'll never get rid of all false positives, especially since cpplint is basically based off a list of regexes.  However, tilting it to be overly permissive would be a good starting point.

As for making the linter understand just the diff, I suppose if you tone down what it can check enough, you'd be able to do this.  However, it's likely to become much more flakey.  (eg., how does it know what indentation level to expect if the context for the diff starts mid-block?)

If the main behavior you want is to be able to filter out already existing lint issues, that can be done as a post-process step for the lint output.  Write a wrapper that takes the base file, the diff, produces the output file to lint, and then only returns lint errors for changed lines.

[Shinichiro Hamaji]

Created attachment 32439 [details]
Add cpplint

---
 3 files changed, 6109 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

Created attachment 32440 [details]
Lint results for WebCore/rendering/*

[Shinichiro Hamaji]

Created attachment 32441 [details]
Fixes for the lint errors

[Shinichiro Hamaji]

I disabled some kinds of lint errors which isn't included in WebKit's style guide explicitly. I also modifies code so that it produces less false positives. Now, the results become much smaller. I ran the updated script on WebCore/rendering/* (as Eric suggested, I decreased the number of targets). It found 69 lint errors and 3 were false positive. I think it may be acceptable ratio as the start point. I attached a fix for the 66 errors for your information.

The 3 false positives were related to #endif. For example, cpplint complained the following line:

http://trac.webkit.org/browser/trunk/WebCore/rendering/bidi.cpp#L1263

For cpplint, this looks unnecessary semicolon, but it's necessary. Maybe we can add some fixes for this kind of cases.

I think some of rules I disabled are useful though the WebKit style guide is not saying about them explicitly. For example, I believe rules for virtual destructor is useful. The linter warns when it finds a non virtual destructor in a class which has virtual methods. Anyway, we can easily re-enable them by modifying filters if we want. See UseWebKitStyles() function in cpplint.py for the list of rules I disabled for now.

Regarding patches, now I added the support for patches. Now we can do something like

git diff | ./cpplint.py --patch -

Most code around the patch mode has been incorporated from rietveld. As Albert said, this patch mode may produce some flaky warnings. But I guess it would be not so bad because cpplint doesn't see contexts so much.

As for "fix" mode, cpplint doesn't support it. And, I guess this kind of auto-fix would introduce more troubles than benefits.

[David Kilzer (:ddkilzer)]

Comment on attachment 32441 [details]
Fixes for the lint errors

This is great!  It's too bad the tool can't do the corrections itself--making all these changes by hand would be very time-consuming.

> diff --git a/WebCore/rendering/AutoTableLayout.cpp b/WebCore/rendering/AutoTableLayout.cpp
> [...]
> -                    int newWidth = max(int (m_layoutStruct[i].effMinWidth), w - reduction);
> +                    int newWidth = max(int(m_layoutStruct[i].effMinWidth), w - reduction);

This should actually be flagged as a warning for use of a C-style cast operator.  WebKit prefers C++-style cast operators:

    int newWidth = max(static_cast<int>(m_layoutStruct[i].effMinWidth), w - reduction);

Other than this issue, this patch looks good to go!  (Are you sure you don't want it reviewed?)

[Shinichiro Hamaji]

Created attachment 32444 [details]
Fix for lint errors v2

Ah, thanks for the comment! Yeah, of course, it is better to check this patch in. I didn't flag '?' as I didn't confirm it could be compiled with Mac. Now I confirmed it can be compiled and I fixed the style of cast. Could you land this patch? I think it's good if you can check this in soon. Otherwise, you may see a lot of conflicts as this patch changes many code.

By the way, the warning for C-style casts is another example of warnings I disabled. It would be nice if we can discuss which rule is necessary for WebKit. As I'm a newbie of WebKit, I know few undocumented customs.

[David Kilzer (:ddkilzer)]

Comment on attachment 32444 [details]
Fix for lint errors v2

r=me!  Yes, I can land this as well.

[David Kilzer (:ddkilzer)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/rendering/AutoTableLayout.cpp
	M	WebCore/rendering/InlineFlowBox.cpp
	M	WebCore/rendering/InlineTextBox.cpp
	M	WebCore/rendering/MediaControlElements.cpp
	M	WebCore/rendering/MediaControlElements.h
	M	WebCore/rendering/RenderArena.cpp
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderFieldset.cpp
	M	WebCore/rendering/RenderFlexibleBox.cpp
	M	WebCore/rendering/RenderFrameSet.cpp
	M	WebCore/rendering/RenderFrameSet.h
	M	WebCore/rendering/RenderImage.cpp
	M	WebCore/rendering/RenderLayer.cpp
	M	WebCore/rendering/RenderListBox.cpp
	M	WebCore/rendering/RenderMarquee.cpp
	M	WebCore/rendering/RenderMedia.cpp
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderSVGImage.cpp
	M	WebCore/rendering/RenderSlider.h
	M	WebCore/rendering/RenderTable.cpp
	M	WebCore/rendering/RenderTableCol.h
	M	WebCore/rendering/RenderTextControlSingleLine.cpp
	M	WebCore/rendering/RenderThemeChromiumSkia.cpp
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
	M	WebCore/rendering/SVGCharacterLayoutInfo.cpp
	M	WebCore/rendering/SVGCharacterLayoutInfo.h
	M	WebCore/rendering/TextControlInnerElements.h
	M	WebCore/rendering/bidi.cpp
Committed r45624
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderFlexibleBox.cpp
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderArena.cpp
	M	WebCore/rendering/RenderFrameSet.cpp
	M	WebCore/rendering/RenderLayer.cpp
	M	WebCore/rendering/RenderThemeChromiumSkia.cpp
	M	WebCore/rendering/RenderFrameSet.h
	M	WebCore/rendering/MediaControlElements.cpp
	M	WebCore/rendering/InlineFlowBox.cpp
	M	WebCore/rendering/RenderImage.cpp
	M	WebCore/rendering/RenderSlider.h
	M	WebCore/rendering/InlineTextBox.cpp
	M	WebCore/rendering/RenderTextControlSingleLine.cpp
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/bidi.cpp
	M	WebCore/rendering/AutoTableLayout.cpp
	M	WebCore/rendering/RenderFieldset.cpp
	M	WebCore/rendering/MediaControlElements.h
	M	WebCore/rendering/RenderListBox.cpp
	M	WebCore/rendering/RenderTable.cpp
	M	WebCore/rendering/RenderSVGImage.cpp
	M	WebCore/rendering/RenderMedia.cpp
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
	M	WebCore/rendering/SVGCharacterLayoutInfo.cpp
	M	WebCore/rendering/RenderTableCol.h
	M	WebCore/rendering/RenderMarquee.cpp
	M	WebCore/rendering/TextControlInnerElements.h
	M	WebCore/rendering/SVGCharacterLayoutInfo.h
r45624 = dfdc3fd5b7fc0d40e7373201a8d5ed58ca63cd43 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/45624

[David Kilzer (:ddkilzer)]

Oops...bugzilla-tool closed the bug when I landed Attachment #32444 [details].  Reopening.

[David Kilzer (:ddkilzer)]

Comment on attachment 32444 [details]
Fix for lint errors v2

Clearing my r+ flag now that this patch has landed.

[Shinichiro Hamaji]

Created attachment 32509 [details]
Add cpplint w/ some autofix

As most common kinds of lint errors seem to be trivial ones, I implemented autofix feature for most common 10 kinds of warnings. I applied the tool for some components in WebCore (css, dom, editing, rendering, history, storage, html, svg, wml, inspector, workers, xml, and page) and it seems to be working good. It found 298 lint errors and fixed 247 of them automatically. There were no false-positives in the 247 error (if my investigation on the git-diff was correct).

[Shinichiro Hamaji]

Created attachment 32510 [details]
Autofix for lint errors

[Mark Rowe (bdash)]

Comment on attachment 32510 [details]
Autofix for lint errors

> diff --git a/WebCore/html/HTMLMarqueeElement.cpp b/WebCore/html/HTMLMarqueeElement.cpp
> index d62eaab..713b57d 100644
> --- a/WebCore/html/HTMLMarqueeElement.cpp
> +++ b/WebCore/html/HTMLMarqueeElement.cpp
> @@ -34,7 +34,7 @@ namespace WebCore {
>  
>  using namespace HTMLNames;
>  
> - // WinIE uses 60ms as the minimum delay by default.
> +    // WinIE uses 60ms as the minimum delay by default.
>  const int defaultMinimumDelay = 60;

This change is wrong.

> diff --git a/WebCore/page/mac/WebCoreViewFactory.h b/WebCore/page/mac/WebCoreViewFactory.h
> index d4dc821..0a32124 100644
> --- a/WebCore/page/mac/WebCoreViewFactory.h
> +++ b/WebCore/page/mac/WebCoreViewFactory.h
> @@ -150,7 +150,7 @@
>  + (WebCoreViewFactory *)sharedFactory;
>  @end
>  
> -@interface WebCoreViewFactory (SubclassResponsibility) <WebCoreViewFactory>
> +@interface WebCoreViewFactory(SubclassResponsibility) <WebCoreViewFactory>
>  @end

This doesn't match the style that we use for categories in Objective-C.

> -@interface WebDashboardRegion : NSObject <NSCopying>
> -{
> +@interface WebDashboardRegion : NSObject <NSCopying> {
>      NSRect rect;
>      NSRect clip;
>      WebDashboardRegionType type;

This doesn't match the style that we use for interface declarations in Objective-C.

[Shinichiro Hamaji]

Created attachment 32519 [details]
Autofix for lint errors v2

Thanks for the review! For now, I've modified the issues by hand. I'll fix the linter so that it won't produce this kinds of false positves.

[David Kilzer (:ddkilzer)]

Comment on attachment 32519 [details]
Autofix for lint errors v2

r=me  Thanks!

[David Kilzer (:ddkilzer)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/MediaList.cpp
	M	WebCore/css/MediaQuery.h
	M	WebCore/css/MediaQueryEvaluator.cpp
	M	WebCore/css/MediaQueryEvaluator.h
	M	WebCore/css/MediaQueryExp.h
	M	WebCore/css/WebKitCSSMatrix.h
	M	WebCore/dom/Comment.h
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/Document.h
	M	WebCore/dom/DocumentFragment.cpp
	M	WebCore/dom/DocumentFragment.h
	M	WebCore/dom/DynamicNodeList.h
	M	WebCore/dom/EditingText.h
	M	WebCore/dom/Element.cpp
	M	WebCore/dom/NamedAttrMap.cpp
	M	WebCore/dom/Node.cpp
	M	WebCore/dom/NodeRareData.h
	M	WebCore/dom/Notation.h
	M	WebCore/dom/ProcessingInstruction.h
	M	WebCore/dom/Range.cpp
	M	WebCore/dom/StyledElement.cpp
	M	WebCore/dom/XMLTokenizerLibxml2.cpp
	M	WebCore/dom/XMLTokenizerQt.cpp
	M	WebCore/editing/ApplyStyleCommand.cpp
	M	WebCore/editing/DeleteSelectionCommand.cpp
	M	WebCore/editing/Editor.cpp
	M	WebCore/editing/SelectionController.cpp
	M	WebCore/editing/SmartReplaceICU.cpp
	M	WebCore/history/HistoryItem.cpp
	M	WebCore/html/CanvasStyle.cpp
	M	WebCore/html/HTMLAnchorElement.cpp
	M	WebCore/html/HTMLAppletElement.h
	M	WebCore/html/HTMLAudioElement.h
	M	WebCore/html/HTMLBRElement.h
	M	WebCore/html/HTMLBaseElement.h
	M	WebCore/html/HTMLBaseFontElement.h
	M	WebCore/html/HTMLDListElement.h
	M	WebCore/html/HTMLDirectoryElement.h
	M	WebCore/html/HTMLFieldSetElement.cpp
	M	WebCore/html/HTMLFormElement.cpp
	M	WebCore/html/HTMLHRElement.cpp
	M	WebCore/html/HTMLHeadElement.h
	M	WebCore/html/HTMLHtmlElement.h
	M	WebCore/html/HTMLImageElement.h
	M	WebCore/html/HTMLInputElement.cpp
	M	WebCore/html/HTMLIsIndexElement.h
	M	WebCore/html/HTMLMarqueeElement.cpp
	M	WebCore/html/HTMLMediaElement.h
	M	WebCore/html/HTMLMenuElement.h
	M	WebCore/html/HTMLMetaElement.h
	M	WebCore/html/HTMLModElement.h
	M	WebCore/html/HTMLOListElement.h
	M	WebCore/html/HTMLOptionElement.cpp
	M	WebCore/html/HTMLParamElement.h
	M	WebCore/html/HTMLQuoteElement.h
	M	WebCore/html/HTMLStyleElement.h
	M	WebCore/html/HTMLTableCaptionElement.h
	M	WebCore/html/HTMLTableCellElement.h
	M	WebCore/html/HTMLTableColElement.h
	M	WebCore/html/HTMLTableSectionElement.cpp
	M	WebCore/html/HTMLTitleElement.h
	M	WebCore/html/HTMLTokenizer.cpp
	M	WebCore/html/HTMLUListElement.h
	M	WebCore/html/HTMLVideoElement.h
	M	WebCore/html/TimeRanges.h
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/page/Console.cpp
	M	WebCore/page/EventHandler.cpp
	M	WebCore/page/Frame.cpp
	M	WebCore/page/android/DragControllerAndroid.cpp
	M	WebCore/page/android/EventHandlerAndroid.cpp
	M	WebCore/page/animation/AnimationController.cpp
	M	WebCore/page/gtk/DragControllerGtk.cpp
	M	WebCore/page/qt/DragControllerQt.cpp
	M	WebCore/page/win/DragControllerWin.cpp
	M	WebCore/page/win/FrameWin.h
	M	WebCore/rendering/RenderSlider.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/rendering/style/RenderStyleConstants.h
	M	WebCore/rendering/style/SVGRenderStyleDefs.h
	M	WebCore/rendering/style/StyleInheritedData.h
	M	WebCore/storage/DatabaseTask.h
	M	WebCore/svg/GradientAttributes.h
	M	WebCore/svg/LinearGradientAttributes.h
	M	WebCore/svg/PatternAttributes.h
	M	WebCore/svg/RadialGradientAttributes.h
	M	WebCore/svg/SVGAnimatedPathData.h
	M	WebCore/svg/SVGAnimatedPoints.h
	M	WebCore/svg/SVGAnimationElement.h
	M	WebCore/svg/SVGClipPathElement.h
	M	WebCore/svg/SVGElementInstance.h
	M	WebCore/svg/SVGFEBlendElement.cpp
	M	WebCore/svg/SVGFEBlendElement.h
	M	WebCore/svg/SVGFEColorMatrixElement.cpp
	M	WebCore/svg/SVGFEComponentTransferElement.cpp
	M	WebCore/svg/SVGFECompositeElement.cpp
	M	WebCore/svg/SVGFEDiffuseLightingElement.cpp
	M	WebCore/svg/SVGFEDisplacementMapElement.cpp
	M	WebCore/svg/SVGFEDistantLightElement.h
	M	WebCore/svg/SVGFEFloodElement.cpp
	M	WebCore/svg/SVGFEFloodElement.h
	M	WebCore/svg/SVGFEFuncAElement.h
	M	WebCore/svg/SVGFEFuncBElement.h
	M	WebCore/svg/SVGFEFuncGElement.h
	M	WebCore/svg/SVGFEFuncRElement.h
	M	WebCore/svg/SVGFEGaussianBlurElement.cpp
	M	WebCore/svg/SVGFEImageElement.cpp
	M	WebCore/svg/SVGFEMergeElement.cpp
	M	WebCore/svg/SVGFEOffsetElement.cpp
	M	WebCore/svg/SVGFEPointLightElement.h
	M	WebCore/svg/SVGFESpecularLightingElement.cpp
	M	WebCore/svg/SVGFESpotLightElement.h
	M	WebCore/svg/SVGFETileElement.cpp
	M	WebCore/svg/SVGLineElement.cpp
	M	WebCore/svg/SVGList.h
	M	WebCore/svg/SVGListTraits.h
	M	WebCore/svg/SVGMPathElement.h
	M	WebCore/svg/SVGMetadataElement.h
	M	WebCore/svg/SVGParserUtilities.cpp
	M	WebCore/svg/SVGPathElement.h
	M	WebCore/svg/SVGPathSegClosePath.h
	M	WebCore/svg/SVGSVGElement.h
	M	WebCore/svg/SVGSetElement.h
	M	WebCore/svg/SVGSwitchElement.h
	M	WebCore/svg/SVGTextPathElement.cpp
	M	WebCore/svg/SVGTextPathElement.h
	M	WebCore/svg/SVGTitleElement.h
	M	WebCore/svg/SVGTransformable.cpp
	M	WebCore/svg/SVGViewSpec.cpp
	M	WebCore/svg/animation/SMILTime.cpp
	M	WebCore/svg/animation/SVGSMILElement.h
	M	WebCore/svg/graphics/SVGResource.cpp
	M	WebCore/wml/WMLPostfieldElement.cpp
	M	WebCore/wml/WMLSetvarElement.cpp
	M	WebCore/workers/WorkerRunLoop.cpp
	M	WebCore/xml/XMLHttpRequest.cpp
	M	WebCore/xml/XPathPath.h
Committed r45747
	M	WebCore/storage/DatabaseTask.h
	M	WebCore/dom/NamedAttrMap.cpp
	M	WebCore/dom/StyledElement.cpp
	M	WebCore/dom/ProcessingInstruction.h
	M	WebCore/dom/DocumentFragment.cpp
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/XMLTokenizerQt.cpp
	M	WebCore/dom/DocumentFragment.h
	M	WebCore/dom/Document.h
	M	WebCore/dom/XMLTokenizerLibxml2.cpp
	M	WebCore/dom/NodeRareData.h
	M	WebCore/dom/Node.cpp
	M	WebCore/dom/Range.cpp
	M	WebCore/dom/Notation.h
	M	WebCore/dom/Element.cpp
	M	WebCore/dom/Comment.h
	M	WebCore/dom/DynamicNodeList.h
	M	WebCore/dom/EditingText.h
	M	WebCore/editing/SmartReplaceICU.cpp
	M	WebCore/editing/Editor.cpp
	M	WebCore/editing/DeleteSelectionCommand.cpp
	M	WebCore/editing/SelectionController.cpp
	M	WebCore/editing/ApplyStyleCommand.cpp
	M	WebCore/ChangeLog
	M	WebCore/wml/WMLSetvarElement.cpp
	M	WebCore/wml/WMLPostfieldElement.cpp
	M	WebCore/history/HistoryItem.cpp
	M	WebCore/page/animation/AnimationController.cpp
	M	WebCore/page/android/EventHandlerAndroid.cpp
	M	WebCore/page/android/DragControllerAndroid.cpp
	M	WebCore/page/qt/DragControllerQt.cpp
	M	WebCore/page/gtk/DragControllerGtk.cpp
	M	WebCore/page/win/DragControllerWin.cpp
	M	WebCore/page/win/FrameWin.h
	M	WebCore/page/Console.cpp
	M	WebCore/page/EventHandler.cpp
	M	WebCore/page/Frame.cpp
	M	WebCore/workers/WorkerRunLoop.cpp
	M	WebCore/svg/animation/SMILTime.cpp
	M	WebCore/svg/animation/SVGSMILElement.h
	M	WebCore/svg/SVGMPathElement.h
	M	WebCore/svg/SVGSVGElement.h
	M	WebCore/svg/SVGTextPathElement.h
	M	WebCore/svg/SVGFEFuncGElement.h
	M	WebCore/svg/SVGTextPathElement.cpp
	M	WebCore/svg/SVGFEBlendElement.h
	M	WebCore/svg/SVGFEMergeElement.cpp
	M	WebCore/svg/SVGFEDisplacementMapElement.cpp
	M	WebCore/svg/SVGMetadataElement.h
	M	WebCore/svg/SVGLineElement.cpp
	M	WebCore/svg/SVGListTraits.h
	M	WebCore/svg/SVGClipPathElement.h
	M	WebCore/svg/SVGViewSpec.cpp
	M	WebCore/svg/SVGList.h
	M	WebCore/svg/SVGSwitchElement.h
	M	WebCore/svg/SVGFEFuncBElement.h
	M	WebCore/svg/SVGTransformable.cpp
	M	WebCore/svg/SVGFEFuncRElement.h
	M	WebCore/svg/SVGPathElement.h
	M	WebCore/svg/SVGElementInstance.h
	M	WebCore/svg/SVGAnimatedPoints.h
	M	WebCore/svg/SVGPathSegClosePath.h
	M	WebCore/svg/SVGFEImageElement.cpp
	M	WebCore/svg/SVGFECompositeElement.cpp
	M	WebCore/svg/SVGFEFloodElement.cpp
	M	WebCore/svg/graphics/SVGResource.cpp
	M	WebCore/svg/SVGFETileElement.cpp
	M	WebCore/svg/GradientAttributes.h
	M	WebCore/svg/SVGFEPointLightElement.h
	M	WebCore/svg/SVGFEFuncAElement.h
	M	WebCore/svg/SVGParserUtilities.cpp
	M	WebCore/svg/SVGFESpecularLightingElement.cpp
	M	WebCore/svg/SVGAnimatedPathData.h
	M	WebCore/svg/SVGFEDiffuseLightingElement.cpp
	M	WebCore/svg/SVGFEComponentTransferElement.cpp
	M	WebCore/svg/SVGAnimationElement.h
	M	WebCore/svg/SVGFEDistantLightElement.h
	M	WebCore/svg/RadialGradientAttributes.h
	M	WebCore/svg/SVGSetElement.h
	M	WebCore/svg/LinearGradientAttributes.h
	M	WebCore/svg/SVGFEColorMatrixElement.cpp
	M	WebCore/svg/SVGFEFloodElement.h
	M	WebCore/svg/SVGTitleElement.h
	M	WebCore/svg/SVGFEBlendElement.cpp
	M	WebCore/svg/SVGFEGaussianBlurElement.cpp
	M	WebCore/svg/SVGFESpotLightElement.h
	M	WebCore/svg/PatternAttributes.h
	M	WebCore/svg/SVGFEOffsetElement.cpp
	M	WebCore/html/TimeRanges.h
	M	WebCore/html/HTMLInputElement.cpp
	M	WebCore/html/HTMLOptionElement.cpp
	M	WebCore/html/HTMLDirectoryElement.h
	M	WebCore/html/HTMLMetaElement.h
	M	WebCore/html/HTMLHeadElement.h
	M	WebCore/html/HTMLBRElement.h
	M	WebCore/html/HTMLFormElement.cpp
	M	WebCore/html/HTMLAnchorElement.cpp
	M	WebCore/html/HTMLTableColElement.h
	M	WebCore/html/HTMLTableCaptionElement.h
	M	WebCore/html/HTMLTableSectionElement.cpp
	M	WebCore/html/HTMLImageElement.h
	M	WebCore/html/HTMLFieldSetElement.cpp
	M	WebCore/html/HTMLBaseFontElement.h
	M	WebCore/html/HTMLVideoElement.h
	M	WebCore/html/HTMLStyleElement.h
	M	WebCore/html/HTMLAppletElement.h
	M	WebCore/html/HTMLParamElement.h
	M	WebCore/html/HTMLTitleElement.h
	M	WebCore/html/HTMLHtmlElement.h
	M	WebCore/html/HTMLHRElement.cpp
	M	WebCore/html/HTMLMediaElement.h
	M	WebCore/html/HTMLQuoteElement.h
	M	WebCore/html/HTMLIsIndexElement.h
	M	WebCore/html/HTMLMenuElement.h
	M	WebCore/html/HTMLMarqueeElement.cpp
	M	WebCore/html/HTMLBaseElement.h
	M	WebCore/html/HTMLUListElement.h
	M	WebCore/html/HTMLAudioElement.h
	M	WebCore/html/HTMLTokenizer.cpp
	M	WebCore/html/HTMLOListElement.h
	M	WebCore/html/HTMLModElement.h
	M	WebCore/html/HTMLDListElement.h
	M	WebCore/html/CanvasStyle.cpp
	M	WebCore/html/HTMLTableCellElement.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/rendering/RenderSlider.cpp
	M	WebCore/rendering/style/SVGRenderStyleDefs.h
	M	WebCore/rendering/style/RenderStyleConstants.h
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/rendering/style/StyleInheritedData.h
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/MediaQueryExp.h
	M	WebCore/css/MediaQuery.h
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/MediaList.cpp
	M	WebCore/css/MediaQueryEvaluator.h
	M	WebCore/css/WebKitCSSMatrix.h
	M	WebCore/css/MediaQueryEvaluator.cpp
	M	WebCore/xml/XMLHttpRequest.cpp
	M	WebCore/xml/XPathPath.h
r45747 = 9c4daccae82cc0d17bba36822c4da4118e74386a (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/45747

[David Kilzer (:ddkilzer)]

Sorry...Bug 27082 needs to be reviewed and landed before I can land individual patches with bugzilla-tool without closing a bug.  Reopening.

[David Kilzer (:ddkilzer)]

Comment on attachment 32519 [details]
Autofix for lint errors v2

Clearing my r+ flag on this since it's been landed.

[Simon Fraser (smfr)]

It got this one wrong:
-    for( count=0; _tempNode; count++ )
+    for ( count=0; _tempNode; count++ )

Should be
   for (count = 0; _tempNode; count++)

[David Levin]

Comment on attachment 32509 [details]
Add cpplint w/ some autofix

General comments:

Thanks for tackling this!

First, I know it was suggested that this tool be able to automatically fix errors but this patch is huge without that support.  It would be nice to do the review without that added support and then add it.  I have two reasons for this:
   1. It is less to review in this initial pass.
   2. I would like to be able to review that code in more detail since it is pretty new and untested (unlike the rest of the code which mostly has been in use already).

Secondly, WebKit python code is following the python coding style (http://www.python.org/dev/peps/pep-0008/, which is different from Google's).  The main differences are 
   1. indent code by 4 space not 2. Fortunately, you can use http://svn.python.org/projects/python/trunk/Tools/scripts/reindent.py to automatically do this for you.
   2. Function/Method names should be lower_case_with_underscores as opposed to CamelCase

I didn't do an in depth review at this point, but I left a few comments on things I noticed during a quick look.

> +++ b/WebKitTools/Scripts/cpplint.py
> @@ -0,0 +1,3362 @@
> +#!/usr/bin/python2.4

"#!/usr/bin/python"


> +# Here are some issues that I've had people identify in my code during reviews,
> +# that I think are possible to flag automatically in a lint tool.  If these were
> +# caught by lint, it would save time both for myself and that of my reviewers.
> +# Most likely, some of these are beyond the scope of the current lint framework,
> +# but I think it is valuable to retain these wish-list items even if they cannot
> +# be immediately implemented.
> +#
> +#  Suggestions
> +#  -----------
> +#  - Check for no 'explicit' for multi-arg ctor
> +#  - Check for boolean assign RHS in parens
> +#  - Check for ctor initializer-list colon position and spacing
> +#  - Check that if there's a ctor, there should be a dtor
> +#  - Check accessors that return non-pointer member variables are
> +#    declared const
> +#  - Check accessors that return non-const pointer member vars are
> +#    *not* declared const
> +#  - Check for using public includes for testing
> +#  - Check for spaces between brackets in one-line inline method
> +#  - Check for no assert()
> +#  - Check for spaces surrounding operators
> +#  - Check for 0 in pointer context (should be NULL)
> +#  - Check for 0 in char context (should be '\0')
> +#  - Check for camel-case method name conventions for methods
> +#    that are not simple inline getters and setters
> +#  - Check that base classes have virtual destructors
> +#    put "  // namespace" after } that closes a namespace, with
> +#    namespace's name after 'namespace' if it is named.
> +#  - Do not indent namespace contents
> +#  - Avoid inlining non-trivial constructors in header files
> +#  - Check for old-school (void) cast for call-sites of functions
> +#    ignored return value
> +#  - Check for class declaration order (typedefs, consts, enums,
> +#    ctor(s?), dtor, friend declarations, methods, member vars)

Several of these are either not relevant to WebKit or just plain not correct, so I think this whole comment should just be deleted.




> +# Matches invalid increment: *count++, which moves pointer insead of

s/insead/instead/

> +
> +def CheckInvalidIncrement(filename, clean_lines, linenum, error):
> +  """Checks for invalud increment *count++.

s/invalud/invalid/

[Shinichiro Hamaji]

Created attachment 32660 [details]
first cpplint


---
 3 files changed, 5799 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

> First, I know it was suggested that this tool be able to automatically fix
> errors but this patch is huge without that support.  It would be nice to do the
> review without that added support and then add it.  I have two reasons for
> this:
>    1. It is less to review in this initial pass.
>    2. I would like to be able to review that code in more detail since it is
> pretty new and untested (unlike the rest of the code which mostly has been in
> use already).
> 
> Secondly, WebKit python code is following the python coding style
> (http://www.python.org/dev/peps/pep-0008/, which is different from Google's). 
> The main differences are 
>    1. indent code by 4 space not 2. Fortunately, you can use
> http://svn.python.org/projects/python/trunk/Tools/scripts/reindent.py to
> automatically do this for you.
>    2. Function/Method names should be lower_case_with_underscores as opposed to
> CamelCase

Agreed. I also reverted the change for supporting lint for patch as this feature is also new and needs detailed review. If you want to have one more step, I may be able to split this patch into 1. just adding cpplint with webkit's python style and 2. modifying cpplint so that it supports webkit's c++ style instead of python. Please let me know if you want this step.

> I didn't do an in depth review at this point, but I left a few comments on
> things I noticed during a quick look.

Thanks for the review! I've fixed the issues you mentioned. I also fixed the style of python. Hmm... fixing CamelCase wasn't fun :(

[David Levin]

Comment on attachment 32660 [details]
first cpplint

Ok, here's my detailed review.  After you address these issues, I think we can land this.  Thanks for your patience.


Throughout change TODO to FIXME (and get rid of any assignment after the TODO).

i.e. "TODO(levin):" becomes "FIXME:"



> diff --git a/WebKitTools/Scripts/cpplint.py b/WebKitTools/Scripts/cpplint.py
> +_ERROR_CATEGORIES = '''\
> +  build/class

These are still indented by 2 spaces.  Although it is in quotes, might as well make it 4 to match.


> +# We used to check for high-bit characters, but after much discussion we
> +# decided those were OK, as long as they were in UTF-8 and didn't represent
> +# hard-coded international strings, which belong in a seperate i18n file.

Another Google specific comment that doesn't apply to WebKit.


> +for op, replacement in [('==', 'EQ'), ('!=', 'NE'),
> +                        ('>=', 'GE'), ('>', 'GT'),
> +                        ('<=', 'LE'), ('<', 'LT')]:
> +  _CHECK_REPLACEMENT['DCHECK'][op] = 'DCHECK_%s' % replacement
> +  _CHECK_REPLACEMENT['CHECK'][op] = 'CHECK_%s' % replacement
> +  _CHECK_REPLACEMENT['EXPECT_TRUE'][op] = 'EXPECT_%s' % replacement
> +  _CHECK_REPLACEMENT['ASSERT_TRUE'][op] = 'ASSERT_%s' % replacement
> +  _CHECK_REPLACEMENT['EXPECT_TRUE_M'][op] = 'EXPECT_%s_M' % replacement
> +  _CHECK_REPLACEMENT['ASSERT_TRUE_M'][op] = 'ASSERT_%s_M' % replacement

Two space indent.

> +
> +for op, inv_replacement in [('==', 'NE'), ('!=', 'EQ'),
> +                            ('>=', 'LT'), ('>', 'LE'),
> +                            ('<=', 'GT'), ('<', 'GE')]:
> +  _CHECK_REPLACEMENT['EXPECT_FALSE'][op] = 'EXPECT_%s' % inv_replacement
> +  _CHECK_REPLACEMENT['ASSERT_FALSE'][op] = 'ASSERT_%s' % inv_replacement
> +  _CHECK_REPLACEMENT['EXPECT_FALSE_M'][op] = 'EXPECT_%s_M' % inv_replacement
> +  _CHECK_REPLACEMENT['ASSERT_FALSE_M'][op] = 'ASSERT_%s_M' % inv_replacement

Two space indent.


> +def _output_format():
> +    """Gets the module's output format."""
> +    return _cpplint_state.output_format
> +
> +

A single line to separate the functions is sufficient (though two lines before a class of course).


> +def find_next_multi_line_comment_start(lines, lineix):
> +    """Find the beginning marker for a multiline comment."""
> +    while lineix < len(lines):

s/lineix/line_index/

> +
> +
> +def close_expression(clean_lines, linenum, pos):

s/lineum/line_number/


> +    line = clean_lines.elided[linenum]
> +    startchar = line[pos]

s/startchar/start_character/

> +    if startchar not in '({[':
> +        return (line, clean_lines.num_lines(), -1)
> +    if startchar == '(': endchar = ')'

s/endchar/end_character/

> +    if startchar == '[': endchar = ']'
> +    if startchar == '{': endchar = '}'

Put the statements on separate lines
    if startchar == '(':
        endchar = ')'
etc.

> +def check_for_copyright(filename, lines, error):
> +    """Logs an error if no Copyright message appears at the top of the file."""
> +
> +    # We'll say it should occur by line 10. Don't forget there's a
> +    # dummy line at the front.
> +    for line in xrange(1, min(len(lines), 11)):
> +        if re.search(r'Copyright', lines[line], re.I): break

Put the statements on separate lines

if re.search(r'Copyright', lines[line], re.I): 
    break

> +    cppvar = get_header_guard_cpp_variable(filename)
> +
> +    ifndef = None
> +    ifndef_linenum = 0

s/ifndef_linenum/ifndef_line_number/

> +    define = None
> +    endif = None
> +    endif_linenum = 0

s/endif_linenum/endif_line_number/

> +    for linenum, line in enumerate(lines):

s/linenum/line_number/ -- throughout this file.

> +        linesplit = line.split()

s/linesplit/line_split/

> +threading_list = (

THREADING_LIST = (

Since it is a constant.

> +    for single_thread_function, multithread_safe_function in threading_list:
> +        ix = line.find(single_thread_function)

s/ix/index/

> +    if search(r'(\w+|[+-]?\d+(\.\d*)?)\s*(<|>)\?=?\s*(\w+|[+-]?\d+)(\.\d*)?',
> +              line):

I would unwrap this line.

> +    if (args and
> +        args.group(1) != 'void' and
> +        not match(r'(const\s+)?%s\s*&' % re.escape(base_classname),
> +                  args.group(1).strip())):

"and" placement. -- Let's follow standard WebKit style and put the "and/or" at the beginning of the continuation line as opposed to the end of the line.

For example,
    if (args
       and args.group(1) != 'void'
       and not match(r'(const\s+)?%s\s*&' % re.escape(base_classname),
                     args.group(1).strip())):

> +        if ((classinfo.virtual_method_linenumber is not None) and
> +            (not classinfo.has_virtual_destructor) and
> +            (not classinfo.is_derived)):  # Only warn for base classes

"and" placement.

> +    if (  # Ignore control structures.
> +        not search(r'\b(if|for|while|switch|return|new|delete)\b', fncall) and
> +        # Ignore pointers/references to functions.
> +        not search(r' \([^)]+\)\([^)]*(\)|,$)', fncall) and
> +        # Ignore pointers/references to arrays.
> +        not search(r' \([^)]+\)\[[^\]]+\]', fncall)):

"and" placement.

> +        if function_name == 'TEST' or function_name == 'TEST_F' or (
> +            not match(r'[A-Z_]+$', function_name)):

I would just unwrap this line. (WebKit doesn't stop code at 80 columns.)


> +    # Next, we complain if there's a comment too near the text
> +    commentpos = line.find('//')

s/commentpos/comment_position/


> +    if commentpos != -1:
> +        # Check if the // may be in quotes.  If so, ignore it
> +        # Comparisons made explicit for clarity -- pylint: disable-msg=C6403
> +        if (line.count('"', 0, commentpos) -
> +            line.count('\\"', 0, commentpos)) % 2 == 0:   # not in quotes

I would unwrap this.

> +            # Allow one space for new scopes, two spaces otherwise:
> +            if (not match(r'^\s*{ //', line) and
> +                ((commentpos >= 1 and
> +                  line[commentpos-1] not in string.whitespace) or
> +                 (commentpos >= 2 and
> +                  line[commentpos-2] not in string.whitespace))):

"and/or" placment

> +                matched = (search(r'[=/-]{4,}\s*$', line[commentend:]) or
> +                         search(r'^/+ ', line[commentend:]))

"or" placement

> +    matched = search(r'\b(if|for|while|switch)\s*'
> +                   r'\(([ ]*)(.).*[^ ]+([ ]*)\)\s*{\s*$',
> +                   line)

These two lines should be indented slightly.


> +    if matched:
> +        if len(matched.group(2)) != len(matched.group(4)):
> +            if not (matched.group(3) == ';' and
> +                    len(matched.group(2)) == 1 + len(matched.group(4)) or
> +                    not matched.group(2) and search(r'\bfor\s*\(.*; \)', line)):

"and/or" placement.

> +    elif (search(r'\s+;\s*$', line) and
> +          not search(r'\bfor\b', line)):

"and" placement.

> +    elif (search(r'\b(for|while)\s*\(.*\)\s*;\s*$', line) and
> +          line.count('(') == line.count(')') and
> +          # Allow do {} while();
> +          not search(r'}\s*while', line)):

"and" placement.

> +    prevlinenum = linenum - 1

s/prevlinenum/previous_line_number/

> +        prevline = get_previous_non_blank_line(clean_lines, linenum)[0]
> +        if (not search(r'[;:}{)=]\s*$|\)\s*const\s*$', prevline) and
> +            prevline.find('#') < 0):

"and" placement.

> +    if (search(r'{.*}\s*;', line) and
> +        line.count('{') == line.count('}') and
> +        not search(r'struct|class|enum|\s*=\s*{', line)):

"and" placement.

> +def get_line_width(line):
> +    if isinstance(line, unicode):
> +        width = 0
> +        for c in unicodedata.normalize('NFC', line):
> +            if unicodedata.east_asian_width(c) in ('W', 'F'):
> +                width += 2
> +            elif not unicodedata.combining(c):
> +                width += 1
> +        return width
> +    else:
> +        return len(line)

Since the "if" ends with a "return" then is no need for the else (following standard WebKit code style).


> +def check_style(filename, clean_lines, linenum, file_extension, error):
> +    """Checks rules from the 'C++ style rules' section of cppguide.html.
> +
> +    Most of these rules are hard to test (naming, comment style), but we
> +    do what we can.  In particular we check for 2-space indents, line lengths,

s/2-space indents/4-space indents/

> +    # One or three blank spaces at the beginning of the line is weird; it's
> +    # hard to reconcile that with 2-space indents.
s/2-space indents/4-space indents/

> +    # Labels should always be indented at least one space.
> +    elif not initial_spaces and line[:2] != '//' and search(r'[^:]:\s*$',
> +                                                            line):

I would unwrap the "line):" which looks like it was done to meet the 80 column limit (which WebKit doesn't have).

> +
> +    # Check if the line is a header guard.
> +    is_header_guard = False
> +    if file_extension == 'h':
> +        cppvar = get_header_guard_cpp_variable(filename)
> +        if (line.startswith('#ifndef %s' % cppvar) or
> +            line.startswith('#define %s' % cppvar) or
> +            line.startswith('#endif  // %s' % cppvar)):
> +            is_header_guard = True

is_header_guard is unused now so you can remove this bit of code.

> +
> +    if (cleansed_line.count(';') > 1 and
> +        # for loops are allowed two ;'s (and may run over two lines).
> +        cleansed_line.find('for') == -1 and
> +        (get_previous_non_blank_line(clean_lines, linenum)[0].find('for') == -1 or
> +         get_previous_non_blank_line(clean_lines, linenum)[0].find(';') != -1) and
> +        # It's ok to have many commands in a switch case that fits in 1 line
> +        not ((cleansed_line.find('case ') != -1 or
> +              cleansed_line.find('default:') != -1) and
> +             cleansed_line.find('break;') != -1)):

"and/or" placement.


> +    for suffix in ('test.cc', 'regtest.cc', 'unittest.cc',
> +                   'inl.h', 'impl.h', 'internal.h'):
> +        if (filename.endswith(suffix) and len(filename) > len(suffix) and
> +            filename[-len(suffix) - 1] in ('-', '_')):
> +            return filename[:-len(suffix) - 1]

"and" placement.

> +    if (filename.endswith('_test.cc') or
> +        filename.endswith('_unittest.cc') or
> +        filename.endswith('_regtest.cc')):

"or" placement.

> +        return True
> +    else:
> +        return False

Remove "else" since the if has a return.

> +    if is_system:
> +        if is_cpp_h:
> +            return _CPP_SYS_HEADER
> +        else:
> +            return _C_SYS_HEADER

Remove "else" since the if has a return.

> +
> +    if target_base == include_base and (
> +        include_dir == target_dir or
> +        include_dir == os.path.normpath(target_dir + '/../public')):

"and/or" placement.

> +    if (target_first_component and include_first_component and
> +        target_first_component.group(0) ==
> +        include_first_component.group(0)):

"and" placement.

Also, having to do
"a ==
 b"
is just ugly so I'd unwrap this part of it.

> +    if linenum + 1 < clean_lines.num_lines():
> +        extended_line = line + clean_lines.elided[linenum + 1]
> +    else:
> +        extended_line = line

extended_line is unused.

> +    # Make Windows paths like Unix.
> +    fullname = os.path.abspath(filename).replace('\\', '/')

fullname is unused.


> +def use_webkit_styles():
> +    global _DEFAULT_FILTERS
> +    _DEFAULT_FILTERS = [
> +        '-whitespace/end_of_line',
> +        '-whitespace/comments',

I don't think the whitespace/comments is correct for WebKit right now.  WebKit style uses 1 space before end of line comments (unliks Google style which does 2 spaces), so I'd just remove this one until we fix it.



> +def main():
> +    use_webkit_styles()

As I've looked through this code, I really want it to get landed but I'm concerned there are issues with respect to WebKit style that will have to be fixed after it is landed when multiple people can contribute and we can do incremental changes to improve it.  Given that it is a tool and not part of this shipping code, I think that it is ok to handle in this fashion.

However, it would be nice to let people know about the potential alpha state of the warnings (due to the adaption to WebKit style).

In short for now, it would be nice to print a warning banner here that this tool is still under development and may flag things incorrectly.  After it is landed and people have an opportunity to try it out we can remove the warning banner.

Something like this:

WARNING WARNING WARNING
This tool is in the process of development and may give inaccurate results at present.
Please files bugs (and/or patches) for things that you notice that it flags incorrectly.
WARNING WARNING WARNING

[Shinichiro Hamaji]

Created attachment 32701 [details]
first cpplint v2


---
 3 files changed, 5799 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

Thanks for the thorough comments on this big patch! I've fixed most of them. Some comments will be inlined.

> A single line to separate the functions is sufficient (though two lines before
> a class of course).

PEP0008 is saying that we should have two-lines for top-level functions: http://www.python.org/dev/peps/pep-0008/

> THREADING_LIST = (
> 
> Since it is a constant.

I changed this to _THREADING_LIST as this is private.

> I would unwrap this line.

I also unwrapped some lines which were wrapped around operators.

> I don't think the whitespace/comments is correct for WebKit right now.  WebKit
> style uses 1 space before end of line comments (unliks Google style which does
> 2 spaces), so I'd just remove this one until we fix it.

This is the list of features we are disabling, not enabling. So, the original code would be OK. I added some comments to clarify what we are doing here. Thanks.

> As I've looked through this code, I really want it to get landed but I'm
> concerned there are issues with respect to WebKit style that will have to be
> fixed after it is landed when multiple people can contribute and we can do
> incremental changes to improve it.  Given that it is a tool and not part of
> this shipping code, I think that it is ok to handle in this fashion.
> 
> However, it would be nice to let people know about the potential alpha state of
> the warnings (due to the adaption to WebKit style).
> 
> In short for now, it would be nice to print a warning banner here that this
> tool is still under development and may flag things incorrectly.  After it is
> landed and people have an opportunity to try it out we can remove the warning
> banner.

Agreed. I've also added some comments which mention that this python script may be inconsistent with other WebKit's script as this comes from Google's cpplint.

[Shinichiro Hamaji]

Created attachment 32703 [details]
first cpplint v3


---
 3 files changed, 5788 insertions(+), 0 deletions(-)

[David Levin]

Comment on attachment 32703 [details]
first cpplint v3

Here's a few last changes.  I'm willing to do them while landing the patch, but if you put up a new one that fixes them, that would also be nice.


> diff --git a/WebKitTools/Scripts/cpplint.py b/WebKitTools/Scripts/cpplint.py
> +# ********************* WARNING WARNING WARNING *********************
> +#
> +# This tool is in the process of development and may give inaccurate
> +# results at resent.  Please files bugs (and/or patches) for things
> +# that you notice that it flags incorrectly.

I actually meant this as something that the tool printed out to warn anyone who tried it out.

> +#
> +# Also, please note that the style of this Python script may be
> +# inconsistent with other Python scripts in WebKit.
> +#

I don't think this is necessary.  I've tried to be very careful with the style in this patch and you've done a lot of changes to make it conform.

> +        self.filters = _DEFAULT_FILTERS[:]
> +        for filt in filters.split(','):

s/filt/filter/

> +            clean_filt = filt.strip()

s/clean_filt/clean_filter/

> +        trigger = base_trigger * 2**_verbose_level()

Add spaces around **


> +_RE_PATTERN_IVALID_INCREMENT = re.compile(

s/IVLIAD/INVALID/

> +    fncall = line    # if there's no control flow construct, look at whole line

s/fncall/function_call/

> +            if search(r'(;|})', start_line):  # Declarations and trivial functions
> +                body_found = True
> +                break                              # ... ignore
> +            elif search(r'{', start_line):

This should be an "if"  (This is in the same spirit as when the previous if ends with a return.)

> +        prevbrace = previous_line.rfind('{')

s/prevbrace/previous_brace/

> +def update_include_state(filename, include_state, io=codecs):
...
> +    headerfile = None

header_file

[Shinichiro Hamaji]

Created attachment 32711 [details]
first cpplint v4


---
 3 files changed, 5788 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

(In reply to comment #37)
> (From update of attachment 32703 [details])
> Here's a few last changes.  I'm willing to do them while landing the patch, but
> if you put up a new one that fixes them, that would also be nice.

Thanks again! Fixed all.

[David Levin]

Committed as http://trac.webkit.org/changeset/45857

[David Levin]

In a future patch, it would be nice to rename it to something like "webkit-lint" (no py ending and chmod +x to make it runnable directly but given the current stage of its development, it seemed ok to do this after this first checkin).