[WebUI] Add some presubmit checks

chrome/browser/resources/PRESUBMIT.py

+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Presubmit script for Chromium WebUI resources.
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details about the presubmit API built into gcl/git cl, and see
+http://www.chromium.org/developers/web-development-style-guide for the rules
+we're checking against here.
+"""
+
+import os

[M-A Ruel, 2012/02/06 20:40:51]

Please use input_api.os_path and input_api.re instead.

[Dan Beam, 2012/02/08 11:42:48]

Done.

+import re
+
+def CheckChangeOnUpload(input_api, output_api):
+  results = []
+  results.extend(_CommonChecks(input_api, output_api))
+  return results
+

[M-A Ruel, 2012/02/06 20:40:51]

Please use 2 vertical spaces between file level symbols.

[Dan Beam, 2012/02/08 11:42:48]

Done.

+def CheckChangeOnCommit(input_api, output_api):
+  results = []
+  results.extend(_CommonChecks(input_api, output_api))
+  return results
+
+def _CommonChecks(input_api, output_api):
+  """Checks common to both upload and commit."""
+  ntp4_or_options2 = lambda f: input_api.FilterSourceFile(f,
+      white_list=os.path.join(os.getcwd(), '(?:ntp4|options2).*\.(?:cs|j)s$'))
+  return _CheckWebDevStyleGuide(
+      input_api,
+      output_api,
+      source_file_filter=ntp4_or_options2)
+
+def _CheckWebDevStyleGuide(input_api, output_api, source_file_filter=None):
+  def _collapseable_hex(s):
+    return (len(s) == 6 and s[0] == s[1] and s[2] == s[3] and s[4] == s[5])
+
+  def _is_gray(s):
+    return s[0] == s[1] == s[2] if len(s) == 3 else s[0:2] == s[2:4] == s[4:6]
+
+  def _remove_ats(s):
+    return re.sub(re.compile(r'@\w+.*?{(.*{.*?})+.*?}', re.DOTALL), '\\1', s)
+
+  def _remove_comments(s):
+    return re.sub(re.compile(r'/\*.*?\*/', re.DOTALL), '', s)
+
+  def _rgb_from_hex(s):
+    if len(s) == 3:
+      r, g, b = s[0] + s[0], s[1] + s[1], s[2] + s[2]
+    else:
+      r, g, b = s[0:2], s[2:4], s[4:6]
+    return 'rgb(%d, %d, %d)' % (int(r, base=16),
+                                int(g, base=16),
+                                int(b, base=16))
+
+  # Shared between hex_could_be_shorter and rgb_if_not_gray.
+  hex_reg = r'#([a-fA-F0-9]{3}|[a-fA-F0-9]{6})(?=[^_a-zA-Z0-9-]|$)(?!.+[{,]$)'
+
+  def alphabetize_props(f):
+    errors = []
+    lines = _remove_ats(_remove_comments(input_api.ReadFile(f[0], 'rb')))
+    for block in re.finditer(r'{(.*?)}', lines, re.DOTALL):
+      rules = filter(lambda l: l.find(': ') >= 0,
+                     map(lambda l: l.strip(), block.group(1).split(';'))[0:-1])
+      props = map(lambda l: l[0:l.find(':')], rules)
+      if props != sorted(props):
+        errors.append('%s:\n  %s' % (f[1], '\n  '.join(rules)))
+    return errors
+
+  def braces_and_colons_have_space_after(f):
+    errors = []
+    lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines()

[M-A Ruel, 2012/02/06 20:40:51]

This pattern is really slow. You are reading the same file 8 times! Use
NewContents().

[Dan Beam, 2012/02/08 11:42:48]

Cool, I was waiting until this was slow to optimize but if this already exists
all the better, :)

+    for l in range(0, len(lines)):
+      if re.search(r'(^\s*{)|(?:{|:(?!\/\/|.*[{,]))\S', lines[l]):
+        errors.append('  %s:~%d:%s' % (f[1], l, lines[l]))
+    return errors
+
+  def classes_use_dashes(f):
+    errors = []
+    lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines()
+    for l in range(0, len(lines)):
+      # Intentionally dumbed down version of CSS2.1 grammar for class without
+      # non-ASCII, escape chars, or whitespace.
+      m = re.search(r'(?:^| )\.(-?[_a-zA-Z0-9-]+)', lines[l])
+      if m:
+        c = m.group(1)
+        if (c.lower() != c or c.find('_') >= 0):
+          errors.append('  %s:~%d:%s' % (f[1], l, lines[l]))
+    return errors
+
+  def favor_single_quotes(f):
+    errors = []
+    lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines()
+    for l in range(0, len(lines)):
+      if lines[l].find('"') >= 0:
+        errors.append('  %s:~%d:%s' % (f[1], l, lines[l]))
+    return errors
+
+  def hex_could_be_shorter(f):
+    errors = []
+    lines = input_api.ReadFile(f[0], 'rb').splitlines()
+    for l in range(0, len(lines)):
+      m = re.search(hex_reg, lines[l])
+      if (m and _collapseable_hex(m.group(1))):
+        errors.append('  %s:%d:%s' % (f[1], l, lines[l]))
+    return errors
+
+  def milliseconds_for_small_times(f):
+    errors = []
+    lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines()
+    small_seconds = r'(?:^|[^_a-zA-Z0-9-])(0?\.[0-9]+)s(?!-?[_a-zA-Z0-9-])'
+    for l in range(0, len(lines)):
+      if re.search(small_seconds, lines[l]):
+        errors.append('  %s:~%d:%s' % (f[1], l, lines[l]))
+
+  def one_selector_per_line(f):
+    errors = []
+    lines = _remove_ats(_remove_comments(input_api.ReadFile(f[0], 'rb'))
+        ).splitlines()
+    for l in range(0, len(lines)):
+      if re.search(r'[^,]+,[^,]+{', lines[l]):
+        errors.append('  %s:~~%d:%s' % (f[1], l, lines[l]))
+    return errors
+
+  def rgb_if_not_gray(f):
+    errors = []
+    lines = _remove_comments(input_api.ReadFile(f[0], 'rb')).splitlines()
+    for l in range(0, len(lines)):
+      m = re.search(hex_reg, lines[l])
+      if (m and not _is_gray(m.group(1))):
+        rgb = _rgb_from_hex(m.group(1))
+        errors.append('  %s:~%d:%s /* = %s */ ' % (f[1], l, lines[l], rgb))
+    return errors
+
+  def zero_length_values(f):
+    errors = []
+    zeros = ''.join((r'[^0-9]0(?:\.0?)?',
+                     r'(?:px|em|%|in|cm|mm|pc|pt|ex|deg|g?rad|m?s|k?hz)',
+                     r'(?!\s*\{)'))
+    hsl = r'hsl\([^\)]*(?:[, ]|(?<=\())(?:0?\.?)?0%'
+    lines = input_api.ReadFile(f[0], 'rb').splitlines()
+    for l in range(0, len(lines)):
+      if (re.search(zeros, lines[l]) and not re.search(hsl, lines[l])):
+        errors.append('  %s:%d:%s' % (f[1], l, lines[l]))
+    return errors
+
+  added_or_modified_files_checks = [
+    { 'desc': 'Alphabetize properties, but list vendor specific (i.e. -webkit) '
+              'above standard.',
+      'test': alphabetize_props
+    },
+    { 'desc': 'Braces ({} and colons (:) should have a space after them and no '
+              'newlines before them.',
+      'test': braces_and_colons_have_space_after,
+    },
+    { 'desc': 'Classes use .dash-form.',
+      'test': classes_use_dashes,
+    },
+    { 'desc': 'Use single quotes (\') instead of double quotes (") in strings.',
+      'test': favor_single_quotes,
+    },
+    { 'desc': 'Favor short hex form (#rgb) when #rrggbb.',
+      'test': hex_could_be_shorter,
+    },
+    { 'desc': 'Use milliseconds for time measurements under 1 second.',
+      'test': milliseconds_for_small_times,
+    },
+    { 'desc': 'One selector per line (what not to do: a, b {}).',
+      'test': one_selector_per_line,
+    },
+    { 'desc': 'Use rgb() over #hex when not a shade of gray (like #333).',
+      'test': rgb_if_not_gray,
+    },
+    { 'desc': 'Make all zero length terms (i.e. 0px) 0 unless inside of hsl()'
+              'or @keyframe.',
+      'test': zero_length_values,
+    },
+  ]
+
+  errors = []
+  files = [(f.AbsoluteLocalPath(), f.LocalPath()) for f in
+           input_api.AffectedFiles(include_deletes=False,
+                                   file_filter=source_file_filter)]
+
+  # Only look at CSS files for now.
+  for f in filter(lambda l: l[1].endswith('.css'), files):

[M-A Ruel, 2012/02/06 20:40:51]

Why not do instead ar:

checks = [
  list of lambda to check a single line
]

for f in filter(lambda l: l[1].endswith('.css'), files):
  for line in _remove_comments(f.NewContents()).splitlines():
    for check in checks:
      check(f, line)

I'd probably be several times faster.

[Dan Beam, 2012/02/08 11:42:48]

Ended up doing something like that.

+    for check in added_or_modified_files_checks:
+      errs = check['test'](f)
+      if errs:
+        msg = '%s\n%s' % (check['desc'], '\n'.join(errs))
+        errors.append(output_api.PresubmitNotifyResult(msg))
+
+  return errors
+
+def GetPreferredTrySlaves(project, change):
+  slaves = PRESUBMIT.GetPreferredTrySlaves(project, change)
+  cwd = os.getcwd()
+  options2, ntp4 = (os.path.join(cwd, 'options2'), os.path.join(cwd, 'ntp4'))
+  files = change.AffectedFiles()
+  if any(options2 in f.LocalPath() for f in files):
+    slaves.append('linux_chromeos')
+  elif any(ntp4 in f.LocalPath() for f in files):
+    slaves.append('win_aura', 'linux_aura')
+  return slaves
+
+#TODO(dbeam): Where does this go?
+def testCheckWebDevStyleGuide(self):
+  fake_file = 'fake.css'
+  fake_contents = """
+/* Comment */
+.badClass, #anotherSelector {
+  color:hsl(0, 5, 0%) #777777 .2s;
+  transition: blah 0.s;
+  background: #369 0px 10px 2;  /* comment */ }
+
+@media print    /*
+ Comment.
+    */
+{
+  @-webkit-keyframe {
+    0% {
+      blah: blah;
+    }
+    0%{blar: blug;}
+  }
+} /** comment **/
+
+.app-contents:active:not(.suppress-active),
+.app:not(.click-focus):focus .app-contents:not(.suppress-active),
+html[dir="rtl"] .drag-representation:not(.placing) .app-contents {
+  border-radius: 10px;
+  -webkit-border-radius: 5px;
+}"""
+
+  change = presubmit.Change('fu', 'bar', self.fake_root_dir, None, 0, 0, None)
+
+  input_api = self.MockInputApi(change, False)
+  input_api.AffectedSourceFiles().MultipleTimes().AndReturn([fake_contents])
+  input_api.ReadFile(fake_file).MultipleTimes().AndReturn(fake_contents)
+
+  self.mox.ReplayAll()
+
+  output = _CheckWebDevStyleGuide(input_api, presubmit.OutputApi, None)
+
+  print "%s" % output