Bug 125049 - Add JavaScript style checker and teach checker.py about .js files
Summary: Add JavaScript style checker and teach checker.py about .js files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 125045
  Show dependency treegraph
 
Reported: 2013-12-01 16:35 PST by Brian Burg
Modified: 2013-12-02 14:50 PST (History)
6 users (show)

See Also:


Attachments
patch (13.23 KB, patch)
2013-12-01 18:26 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-12-01 16:35:47 PST
We should check WebInspectorUI for trivial style problems using check-webkit-style. The first step is to create a JavaScript checker module and associate it with the .js file extension. As this is intended to check the inspector sources, we should defer on checking layout tests, perf tests, and other things written in JavaScript.
Comment 1 Brian Burg 2013-12-01 18:26:04 PST
Created attachment 218130 [details]
patch
Comment 2 Joseph Pecoraro 2013-12-02 13:04:49 PST
Comment on attachment 218130 [details]
patch

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

> Tools/Scripts/webkitpy/style/checker_unittest.py:484
> +        # Check checker attributes on a typical input.
> +        file_base = "foo"
> +        file_extension = "css"
> +        file_path = file_base + "." + file_extension
> +        self.assert_checker_text(file_path)
> +        checker = self.dispatch(file_path)
> +        self.assertEqual(checker.handle_style_error,
> +                          self.mock_handle_style_error)
> +

There is no mention in the ChangeLog of handling .css files as text. It also seems weird to put this test in "test_js_paths" and not a "test_css_paths". Looks fine though.
Comment 3 WebKit Commit Bot 2013-12-02 14:20:13 PST
Comment on attachment 218130 [details]
patch

Rejecting attachment 218130 [details] from commit-queue.

burg@cs.washington.edu does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 4 Brian Burg 2013-12-02 14:21:15 PST
Oops, I don't understand how attachment flags work. Can you set cq+? Also,

(In reply to comment #2)

> There is no mention in the ChangeLog of handling .css files as text. It also seems weird to put this test in "test_js_paths" and not a "test_css_paths". Looks fine though.

This is copied from existing style checkers as a negative test case. IMO, all of the checker constructor unit tests should be deleted since they are pretty useless. But then, I'd get lost deleting a lot of unit tests for lack of purpose.
Comment 5 WebKit Commit Bot 2013-12-02 14:50:29 PST
Comment on attachment 218130 [details]
patch

Clearing flags on attachment: 218130

Committed r159969: <http://trac.webkit.org/changeset/159969>
Comment 6 WebKit Commit Bot 2013-12-02 14:50:31 PST
All reviewed patches have been landed.  Closing bug.