[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.
+"""
+
+# TODO(dbeam): Lazy load these?
+import unittest
+from testing_support.super_mox import SuperMoxTestBase, mox

[M-A Ruel, 2012/02/09 14:09:46]

As I said before, I dislike that;

presubmit_support.py and trychange.py are loading all the relevant PRESUBMIT.py
in their process. Having super_mox loaded in an executable could have unforeseen
side effects.

Let's say in 2 years from now, once alzheimer kicks in, an unrelated change in
super_mox.py does a side on import, mocking something important. That would
break this presubmit check and this would be really hard to debug. That's my
main point here, I don't want to debug this kind of problem in 2 years from now.
I assure you, debugging mock left overs isn't fun.

So use input_api.canned_checks.RunUnitTests(
  input_api, output_api,
  [input_api.os_path.join(input_api.PresubmitLocalPath(), 'test_presubmit.py')])

and have test_presubmit.py do whatever it wants. It's fine at that point to mock
everything since it's out of process.

+
+
+def CheckChangeOnUpload(input_api, output_api):
+  return _CommonChecks(input_api, output_api)
+
+
+def CheckChangeOnCommit(input_api, output_api):
+  return _CommonChecks(input_api, output_api)
+
+
+def _RunUnitTests():
+  unittest.TextTestRunner().run(
+      unittest.TestLoader().loadTestsFromTestCase(WebDevStyleGuideTest))
+
+
+def _CommonChecks(input_api, output_api):
+  """Checks common to both upload and commit."""
+  results = []
+  resources = input_api.PresubmitLocalPath()
+
+  presubmit = input_api.os_path.join(resources, 'PRESUBMIT.py')
+  if presubmit in [f.AbsoluteLocalPath() for f in input_api.AffectedFiles()]:
+    _RunUnitTests()
+
+  # TODO(dbeam): Remove this filter eventually when ready.
+  dirs = (input_api.os_path.join(resources, 'ntp4'),
+          input_api.os_path.join(resources, 'options2'))
+  def file_filter(affected_file):
+    f = affected_file.AbsoluteLocalPath()
+    return (f.startswith(dirs) and f.endswith('.css'))
+
+  results.extend(_CheckWebDevStyleGuide(input_api, output_api,
+                                        source_file_filter=file_filter))
+  return results
+
+
+# TODO(dbeam): Real CSS parser?
+def _CheckWebDevStyleGuide(input_api, output_api, source_file_filter=None):
+  # We use this a lot, so make a nick name variable.
+  re = input_api.re
+
+  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 int(r, base=16), int(g, base=16), int(b, base=16)
+
+  def alphabetize_props(file):
+    errors = []
+    for block in re.finditer(r'{(.*?)}', file, re.DOTALL):
+      rules = filter(lambda r: r.find(': ') >= 0,
+                     map(lambda t: t.strip(), block.group(1).split(';'))[0:-1])
+      props = map(lambda r: r[0:r.find(':')], rules)
+      if props != sorted(props):
+        errors.append('    %s;\n' % (';\n    '.join(rules)))
+    return errors
+
+  def braces_have_space_before_and_nothing_after(line):
+    return re.search(r'(?:^|\S){|{\s*\S+\s*$', line)
+
+  def classes_use_dashes(line):
+    # Intentionally dumbed down version of CSS 2.1 grammar for class without
+    # non-ASCII, escape chars, or whitespace.
+    m = re.search(r'\.(-?[_a-zA-Z0-9-]+).*[,{]\s*$', line)
+    return (m and (m.group(1).lower() != m.group(1) or
+                   m.group(1).find('_') >= 0))
+
+  def close_brace_on_new_line(line):
+    return (line.find('}') >= 0 and re.search(r'[^ }]', line))
+
+  def colons_have_space_after(line):
+    return re.search(r':(?!\/\/)\S(?!.*[{,]\s*$)', line)
+
+  def favor_single_quotes(lines):
+    return line.find('"') >= 0
+
+  # 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-]|$)'
+             r'(?!.*(?:{.*|,\s*)$)')
+  def hex_could_be_shorter(line):
+    m = re.search(hex_reg, line)
+    return (m and _collapseable_hex(m.group(1)))
+
+  small_seconds = r'(?:^|[^_a-zA-Z0-9-])(0?\.[0-9]+)s(?!-?[_a-zA-Z0-9-])'
+  def milliseconds_for_small_times(line):
+    return re.search(small_seconds, line)
+
+  def one_rule_per_line(line):
+    return re.search('(.*:(?!\/\/)){2,}(?!.*[,{]\s*$)', line)
+
+  any_reg = re.compile(r':(?:-webkit-)?any\(.*?\)', re.DOTALL)
+  multi_sels = re.compile(r'(?:}[\n\s]*)?([^,]+,(?=[^{}]+?{).*[,{])\s*$',
+                          re.MULTILINE)
+  def one_selector_per_line(file):
+    errors = []
+    for b in re.finditer(multi_sels, re.sub(any_reg, '', file)):
+      errors.append('    ' + b.group(1).strip())
+    return errors
+
+  def rgb_if_not_gray(line):
+    m = re.search(hex_reg, line)
+    return (m and not _is_gray(m.group(1)))
+
+  def suggest_ms_from_s(line):
+    ms = int(float(re.search(small_seconds, line).group(1)) * 1000)
+    return ' (replace with %dms)' % ms
+
+  def suggest_rgb_from_hex(line):
+    suggestions = ['rgb(%d, %d, %d)' % _rgb_from_hex(h.group(1))
+        for h in re.finditer(hex_reg, line)]
+    return ' (replace with %s)' % ', '.join(suggestions)
+
+  def suggest_short_hex(line):
+    h = re.search(hex_reg, line).group(1)
+    return ' (replace with #%s)' % (h[0] + h[2] + h[4])
+
+  hsl = r'hsl\([^\)]*(?:[, ]|(?<=\())(?:0?\.?)?0%'
+  zeros = (r'[^0-9]0(?:\.0?)?'
+           r'(?:px|em|%|in|cm|mm|pc|pt|ex|deg|g?rad|m?s|k?hz)'
+           r'(?!\s*\{)')
+  def zero_length_values(line):
+    return (re.search(zeros, line) and not re.search(hsl, line))
+
+  added_or_modified_files_checks = [
+    { 'desc': 'Alphabetize properties and list vendor specific (i.e. -webkit) '
+              'above standard.',
+      'test': alphabetize_props,
+      'multiline': True,
+    },
+    { 'desc': 'Start braces ({) end a selector, have a space before them and '
+              'no rules after.',
+      'test': braces_have_space_before_and_nothing_after,
+    },
+    { 'desc': 'Classes use .dash-form.',
+      'test': classes_use_dashes,
+    },
+    { 'desc': 'Always put a rule closing brace (}) on a new line.',
+      'test': close_brace_on_new_line,
+    },
+    { 'desc': 'Colons (:) should have a space after them.',
+      'test': colons_have_space_after,
+    },
+    { 'desc': 'Use single quotes (\') instead of double quotes (") in strings.',
+      'test': favor_single_quotes,
+    },
+    { 'desc': 'Use abbreviated hex (#rgb) when in form #rrggbb.',
+      'test': hex_could_be_shorter,
+      'after': suggest_short_hex,
+    },
+    { 'desc': 'Use milliseconds for time measurements under 1 second.',
+      'test': milliseconds_for_small_times,
+      'after': suggest_ms_from_s,
+    },
+    { 'desc': 'One rule per line (what not to do: color: red; margin: 0;).',
+      'test': one_rule_per_line,
+    },
+    { 'desc': 'One selector per line (what not to do: a, b {}).',
+      'test': one_selector_per_line,
+      'multiline': True,
+    },
+    { 'desc': 'Use rgb() over #hex when not a shade of gray (like #333).',
+      'test': rgb_if_not_gray,
+      'after': suggest_rgb_from_hex,
+    },
+    { 'desc': 'Make all zero length terms (i.e. 0px) 0 unless inside of hsl() '
+              'or part of @keyframe.',
+      'test': zero_length_values,
+    },
+  ]
+
+  results = []
+  affected_files = input_api.AffectedFiles(include_deletes=False,
+                                           file_filter=source_file_filter)
+  files = []
+  for f in affected_files:
+    # Remove all /*comments*/ and @at-keywords as we're not using a real parser.
+    file_contents = _remove_ats(_remove_comments('\n'.join(f.NewContents())))
+    files.append((f.LocalPath(), file_contents))
+
+  # Only look at CSS files for now.
+  for f in filter(lambda f: f[0].endswith('.css'), files):
+    file_errors = []
+    for check in added_or_modified_files_checks:
+      # If the check is multiline, it receieves the whole file and gives us back
+      # a list of things wrong. If the check isn't multiline, we pass it each
+      # line and the check returns something truthy if there's an issue.
+      if ('multiline' in check and check['multiline']):
+        check_errors = check['test'](f[1])
+        if len(check_errors) > 0:
+          # There are currently no multiline checks with ['after'].
+          file_errors.append('- %s\n%s' %
+              (check['desc'], '\n'.join(check_errors).rstrip()))
+      else:
+        check_errors = []
+        lines = f[1].splitlines()
+        for lnum in range(0, len(lines)):
+          line = lines[lnum]
+          if check['test'](line):
+            error = '    ' + line.strip()
+            if 'after' in check:
+              error += check['after'](line)
+            check_errors.append(error)
+        if len(check_errors) > 0:
+          file_errors.append('- %s\n%s' %
+              (check['desc'], '\n'.join(check_errors)))
+    if len(file_errors) > 0:
+      results.append(output_api.PresubmitNotifyResult(
+          '%s:\n%s' % (f[0], '\n\n'.join(file_errors))))
+
+  return results
+
+
+class WebDevStyleGuideTest(SuperMoxTestBase):
+  def runTest(self):
+    self.__testFunc()
+
+  def setUp(self):
+    SuperMoxTestBase.setUp(self)
+
+    self.fake_file_name = 'fake.css'
+
+    self.fake_file = self.mox.CreateMockAnything()
+    self.mox.StubOutWithMock(self.fake_file, 'LocalPath')
+    self.fake_file.LocalPath().AndReturn(self.fake_file_name)
+    # Actual calls to NewContents() are defined in each test.
+    self.mox.StubOutWithMock(self.fake_file, 'NewContents')
+
+    self.input_api = self.mox.CreateMockAnything()
+    self.mox.StubOutWithMock(self.input_api, 'AffectedSourceFiles')
+    self.input_api.AffectedFiles(
+        include_deletes=False, file_filter=None).AndReturn([self.fake_file])
+
+    import re
+    self.input_api.re = re
+
+    # Actual creations of PresubmitNotifyResult are defined in each test.
+    self.output_api = self.mox.CreateMockAnything()
+    self.mox.StubOutWithMock(self.output_api, 'PresubmitNotifyResult',
+                                  use_mock_anything=True)
+
+  def VerifyContentsProducesOutput(self, contents, output):
+    self.fake_file.NewContents().AndReturn(contents.splitlines())
+    self.output_api.PresubmitNotifyResult(
+        self.fake_file_name + ':\n' + output.strip()).AndReturn(None)
+    self.mox.ReplayAll()
+    _CheckWebDevStyleGuide(self.input_api, self.output_api)
+
+  def testAlphaWithAtBlock(self):
+    self.VerifyContentsProducesOutput("""
+/* A hopefully safely ignored comment and @media statement. /**/
+@media print {
+  div {
+    display: block;
+    color: red;
+  }
+}""", """
+- Alphabetize properties and list vendor specific (i.e. -webkit) above standard.
+    display: block;
+    color: red;""")
+
+  def testAlphaWithNonStandard(self):
+    self.VerifyContentsProducesOutput("""
+div {
+  /* A hopefully safely ignored comment and @media statement. /**/
+  color: red;
+  -webkit-margin-start: 5px;
+}""", """
+- Alphabetize properties and list vendor specific (i.e. -webkit) above standard.
+    color: red;
+    -webkit-margin-start: 5px;""")
+
+  def testAlphaWithLongerDashedProps(self):
+    self.VerifyContentsProducesOutput("""
+div {
+  border-left: 5px;  /* A hopefully removed comment. */
+  border: 5px solid red;
+}""", """
+- Alphabetize properties and list vendor specific (i.e. -webkit) above standard.
+    border-left: 5px;
+    border: 5px solid red;""")
+
+  def testBracesHaveSpaceBeforeAndNothingAfter(self):
+    self.VerifyContentsProducesOutput("""
+/* Hello! */div/* Comment here*/{
+  display: block;
+}
+
+blah /* hey! */
+{
+  rule: value;
+}
+
+.this.is { /* allowed */
+  rule: value;
+}""", """
+- Start braces ({) end a selector, have a space before them and no rules after.
+    div{
+    {""")
+
+  def testClassesUseDashes(self):
+    self.VerifyContentsProducesOutput("""
+.className,
+.ClassName,
+.class-name /* We should not catch this. */,
+.class_name {
+  display: block;
+}""", """
+ - Classes use .dash-form.
+    .className,
+    .ClassName,
+    .class_name {""")
+
+  def testCloseBraceOnNewLine(self):
+    self.VerifyContentsProducesOutput("""
+@media { /* TODO(dbeam) Fix this case.
+  .rule {
+    display: block;
+  }}
+
+#rule {
+  rule: value; }""", """
+- Always put a rule closing brace (}) on a new line.
+    rule: value; }""")
+
+  def testColonsHaveSpaceAfter(self):
+    self.VerifyContentsProducesOutput("""
+div:not(.class):not([attr]) /* We should not catch this. */ {
+  display:block;
+}""", """
+- Colons (:) should have a space after them.
+    display:block;""")
+
+  def testFavorSingleQuotes(self):
+    self.VerifyContentsProducesOutput("""
+html[dir="rtl"] body,
+html[dir=ltr] body /* TODO(dbeam): Require '' around rtl in future? */ {
+  background: url("chrome://resources/BLAH");
+  font-family: "Open Sans";
+}""", """
+- Use single quotes (') instead of double quotes (") in strings.
+    html[dir="rtl"] body,
+    background: url("chrome://resources/BLAH");
+    font-family: "Open Sans";""")
+
+  def testHexCouldBeShorter(self):
+    self.VerifyContentsProducesOutput("""
+#abc,
+#abc-,
+#abc-ghij,
+#abcdef-,
+#abcdef-ghij,
+#aaaaaa,
+#bbaacc {
+  color: #999999;
+  color: #666;
+}""", """
+- Use abbreviated hex (#rgb) when in form #rrggbb.
+    color: #999999; (replace with #999)""")
+
+  def testUseMillisecondsForSmallTimes(self):
+    self.VerifyContentsProducesOutput("""
+.transition-0s /* This is gross but may happen. */ {
+  transform: one 0.2s;
+  transform: two .1s;
+  transform: tree 1s;
+  transform: four 300ms;
+}""", """
+- Use milliseconds for time measurements under 1 second.
+    transform: one 0.2s; (replace with 200ms)
+    transform: two .1s; (replace with 100ms)""")
+
+  def testOneRulePerLine(self):
+    self.VerifyContentsProducesOutput("""
+div {
+  rule: value; /* rule: value; */
+  rule: value; rule: value;
+}""", """
+- One rule per line (what not to do: color: red; margin: 0;).
+    rule: value; rule: value;""")
+
+  def testOneSelectorPerLine(self):
+    self.VerifyContentsProducesOutput("""
+a,
+div,a,
+div,/* Hello! */ span,
+#id.class([dir=rtl):not(.class):any(a, b, d) {
+  rule: value;
+}""", """
+- One selector per line (what not to do: a, b {}).
+    div,a,
+    div, span,""")
+
+
+  def testRgbIfNotGray(self):
+    self.VerifyContentsProducesOutput("""
+#abc,
+#aaa,
+#aabbcc {
+  background: -webkit-linear-gradient(left, from(#abc), to(#def));
+  color: #bad;
+  color: #bada55;
+}""", """
+- Use rgb() over #hex when not a shade of gray (like #333).
+    background: -webkit-linear-gradient(left, from(#abc), to(#def)); """
+"""(replace with rgb(170, 187, 204), rgb(221, 238, 255))
+    color: #bad; (replace with rgb(187, 170, 221))
+    color: #bada55; (replace with rgb(186, 218, 85))""")
+
+  def testZeroLengthTerms(self):
+    self.VerifyContentsProducesOutput("""
+@-webkit-keyframe anim {
+  0% { /* Ignore key frames */
+    width: 0px;
+  }
+  10% {
+    width: 10px;
+  }
+  100% {
+    width: 100px;
+  }
+}
+.animating {
+  -webkit-animation: anim 0s;
+  -webkit-animation-duration: anim 0ms;
+  -webkit-transform: scale(0%),
+                     translateX(0deg),
+                     translateY(0rad),
+                     translateZ(0grad);
+  background-position-x: 0em;
+  background-position-y: 0ex;
+  border-width: 0em;
+  color: hsl(0, 0%, 85%); /* Shouldn't trigger error. */
+}
+
+@page {
+  border-width: 0mm;
+  height: 0cm;
+  width: 0in;
+}""","""
+- Make all zero length terms (i.e. 0px) 0 unless inside of hsl() or part of"""
+""" @keyframe.
+    width: 0px;
+    -webkit-animation: anim 0s;
+    -webkit-animation-duration: anim 0ms;
+    -webkit-transform: scale(0%),
+    translateX(0deg),
+    translateY(0rad),
+    translateZ(0grad);
+    background-position-x: 0em;
+    background-position-y: 0ex;
+    border-width: 0em;
+    border-width: 0mm;
+    height: 0cm;
+    width: 0in;
+""")
+
+
+if __name__ == '__main__':
+  _RunUnitTests()