Bug 125048 - Web Inspector: Use ESLint to check Inspector JS for code style problems
Summary: Web Inspector: Use ESLint to check Inspector JS for code style problems
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 125059
Blocks: 125045
  Show dependency treegraph
 
Reported: 2013-12-01 16:23 PST by Brian Burg
Modified: 2016-12-13 15:34 PST (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-12-01 16:23:51 PST
It looks like it would be not much work to hook up something like JSHint to the check-webkit-style JS checker.

JSHint (jshint.com) is an MIT-licensed fork of JSLint and fairly configurable. We'd want to write a custom reporter that discards things we don't care about. This can be done incrementally- first ignore everything except trailing whitespace, then report more errors as desired.

Steps to minimum viable improvement (checking trailing whitespaces via JSHint):

0. Add a single-file jshint dist to the repository (somewhere?) and add a LICENSE file (license is stated here: http://jshint.com/hack/)

1. Teach the JavaScript checker to invoke JSHint CLI using a built standalone JSC. (http://www.jshint.com/docs/cli/)

2. Create a configuration file (http://www.jshint.com/docs/) that sets some options and defines Web Inspector-specific globals defined in Main.js and Utilities.js.

3. Create a custom reporter and teach the python checker how to interpret the output.

4. Scope the checker to only run for WebInspectorUI sources. Maybe folks want to check non-inspector JS (i.e., tests) as well, but that's out of scope.
Comment 1 Brian Burg 2013-12-01 22:02:25 PST
Before taking on this bug, I'll note that JSHint retains portions of JSLint's unpleasant Crockford license. (http://en.wikipedia.org/wiki/JSLint#License) So, it would be good to know if this 3rd-party library cannot be included in the project repository on those grounds.

I am not aware of any other reasonable 3rd-party JS style checkers. Closure Linter is unconfigurable and only enforces Google style.
Comment 2 Timothy Hatcher 2013-12-02 14:15:02 PST
(In reply to comment #1)
> Before taking on this bug, I'll note that JSHint retains portions of JSLint's unpleasant Crockford license. (http://en.wikipedia.org/wiki/JSLint#License) So, it would be good to know if this 3rd-party library cannot be included in the project repository on those grounds.
> 
> I am not aware of any other reasonable 3rd-party JS style checkers. Closure Linter is unconfigurable and only enforces Google style.

We have jsmin.py with the Crockford license. But we need to get approval internally at Apple to add any third-party project to WebKit.
Comment 3 Brian Burg 2013-12-02 14:30:29 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Before taking on this bug, I'll note that JSHint retains portions of JSLint's unpleasant Crockford license. (http://en.wikipedia.org/wiki/JSLint#License) So, it would be good to know if this 3rd-party library cannot be included in the project repository on those grounds.
> > 
> > I am not aware of any other reasonable 3rd-party JS style checkers. Closure Linter is unconfigurable and only enforces Google style.
> 
> We have jsmin.py with the Crockford license. But we need to get approval internally at Apple to add any third-party project to WebKit.

Ok, good to know, please update the bug if/when you get approval. I'll peck at the implementation more when I need a side-project break.

Of the steps mentioned, I have a patch for (0), and (4) already landed. I also landed a native readFile() so JSHint can run in "Rhino" mode.
Comment 4 Radar WebKit Bug Importer 2014-02-14 09:52:54 PST
<rdar://problem/16070893>
Comment 5 Timothy Hatcher 2014-02-14 11:45:32 PST
Lets exposure http://eslint.org instead, it is nicer than JSHint.
Comment 6 Brian Burg 2014-08-27 10:06:15 PDT
Is this hooked up to check-webkit-style yet?
Comment 7 Timothy Hatcher 2014-08-27 10:10:11 PDT
No, but there is a basic config in the tree now.

http://trac.webkit.org/browser/trunk/Source/WebInspectorUI/.eslintrc