PRESUBMIT check for JavaScript style errors

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index aa79141a90fa5217e6f29f1b7646c0744a21ffd5..96128d571d7aa892fe415468c92677da14ffd849 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -8,10 +8,6 @@ See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into gcl.
 """
 
-
-import re
-
-
 _EXCLUDED_PATHS = (
     r"^breakpad[\\\/].*",
     r"^native_client_sdk[\\\/].*",
@@ -30,6 +26,114 @@ _TEST_ONLY_WARNING = (
     'Email joi@chromium.org if you have questions.')
 
 
+def _CheckJavaScriptStyle(input_api, output_api):
+  """Check for JavaScript style violations."""
+
+  import closure_linter.common.errorhandler
+
+  class ErrorHandlerImpl(closure_linter.common.errorhandler.ErrorHandler):
+    """Implementation of ErrorHandler that collects all errors except those
+       that don't apply for Chromium JavaScript code.
+    """
+
+    def __init__(self):
+      self._errors = []
+
+    def HandleFile(self, filename, first_token):
+      self._filename = filename
+
+    def HandleError(self, error):
+      if (self._valid(error)):
+        error.filename = self._filename
+        self._errors.append(error)
+
+    def GetErrors(self):
+      return self._errors
+
+    def HasErrors(self):
+      return bool(self._errors)
+
+    def _valid(self, error):
+      """Check whether an error is valid. Most errors are valid, with a few
+         exceptions which are listed here.
+      """
+
+      import closure_linter.errors
+
+      return error.code not in [
+          closure_linter.errors.COMMA_AT_END_OF_LITERAL,
+          closure_linter.errors.JSDOC_ILLEGAL_QUESTION_WITH_PIPE,
+          closure_linter.errors.JSDOC_TAG_DESCRIPTION_ENDS_WITH_INVALID_CHARACTER

[Tyler Breisacher (Chromium), 2012/02/06 22:00:13]

What is typically done in cases like this to avoid the 80char issue? 'import
closure_linter.errors as cl_errors' or something like that?

+      ]
+
+  import closure_linter.checker
+  import os.path
+
+  # Only check the following folders. OWNERS of folders containing JavaScript
+  # code can opt-in to this check by adding the folder here.
+  checked_folders = [
+      os.path.join('chrome', 'browser', 'resources', 'ntp4'),
+      os.path.join('chrome', 'browser', 'resources', 'options2'),
+  ]
+
+  def inCheckedFolder(affected_file):
+    return any(affected_file.LocalPath().startswith(cf)
+        for cf in checked_folders)
+
+  def jsOrHtml(affected_file):
+    return input_api.re.search('\.(js|html?)$', affected_file.LocalPath())
+
+  def fileFilter(affected_file):
+    return jsOrHtml(affected_file) and inCheckedFolder(affected_file)
+
+  results = []
+
+  for f in input_api.change.AffectedFiles(file_filter=fileFilter):
+    errorLines = []
+
+    # check for getElementById()
+    for i, line in enumerate(f.NewContents(), start=1):
+      if 'getElementById' in line:
+        errorLines.append('  line %d: %s\n%s' % (
+            i,
+            'Use $() instead of document.getElementById()',
+            line))
+
+      const_re = input_api.re.compile(r'\bconst\b')
+      if const_re.search(line):
+        errorLines.append('  line %d: %s\n%s' % (
+            i,
+            'Use |var| instead of |const|. See http://crbug.com/80149',
+            line))
+
+    # Use closure_linter to check for several different errors
+    error_handler = ErrorHandlerImpl()
+    checker = closure_linter.checker.JavaScriptStyleChecker(error_handler)
+    checker.Check(f.LocalPath())
+
+    for error in error_handler.GetErrors():
+      errorMsg = '  line %d: E%04d: %s\n%s' % (
+          error.token.line_number,
+          error.code,
+          error.message,
+          error.token.line)
+      errorLines.append(errorMsg)
+
+    if errorLines:
+      errorLines = [
+          'Found JavaScript style violations in %s:' %
+          f.LocalPath()] + errorLines
+      results.append(output_api.PresubmitError('\n'.join(errorLines)))
+
+  if results:
+    results.append(output_api.PresubmitNotifyResult(
+        'See the JavaScript style guide at '
+        'http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml'
+        ' and contact tbreisacher@chromium.org for feedback on this'
+        ' PRESUBMIT check.'))
+
+  return results
+
 
 def _CheckNoInterfacesInBase(input_api, output_api):
   """Checks to make sure no files in libbase.a have |@interface|."""
@@ -214,6 +318,7 @@ def _CheckNoNewOldCallback(input_api, output_api):
 def _CommonChecks(input_api, output_api):
   """Checks common to both upload and commit."""
   results = []
+  results.extend(_CheckJavaScriptStyle(input_api, output_api))
   results.extend(input_api.canned_checks.PanProjectChecks(
       input_api, output_api, excluded_paths=_EXCLUDED_PATHS))
   results.extend(_CheckNoInterfacesInBase(input_api, output_api))
@@ -340,6 +445,7 @@ def GetPreferredTrySlaves(project, change):
     return ['mac_rel']
   preferred = ['win_rel', 'linux_rel', 'mac_rel']
   aura_re = '_aura[^/]*[.][^/]*'
-  if any(re.search(aura_re, f.LocalPath()) for f in change.AffectedFiles()):
+  if any(input_api.re.search(aura_re, f.LocalPath())
+      for f in change.AffectedFiles()):
     preferred.append('linux_chromeos')
   return preferred