Add web_dev_style presubmit to ensure <if>/<include> live in comments

Before:

<include src="...">
<if expr="chromeos">
    chromeOsOnlyCode();
</if>

After:

// <includes src="...">
// <if expr="chromeos">
    chromeOsOnlyCode();
// </if>

This is to unblock running clang-format on JavaScript and allow other
tools to run on more expected input inside of .js files (i.e. closure
compiler).

BUG=678778
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2624503002
Cr-Commit-Position: refs/heads/master@{#443069}

chrome/browser/resources/settings/people_page/people_page.js

@@ -247,10 +247,10 @@ Polymer({
     if (!this.syncStatus && syncStatus && !syncStatus.signedIn)
       chrome.metricsPrivate.recordUserAction('Signin_Impression_FromSettings');
 
-<if expr="not chromeos">
+// <if expr="not chromeos">
     if (syncStatus.signedIn)
       settings.ProfileInfoBrowserProxyImpl.getInstance().getProfileStatsCount();
-</if>
+// </if>
 
     if (!syncStatus.signedIn && this.$.disconnectDialog.open)
       this.$.disconnectDialog.close();

chrome/browser/web_dev_style/js_checker.py

@@ -25,6 +25,14 @@ class JSChecker(object):
     return self.RegexCheck(i, line, r"chrome\.send\('[^']+'\s*(, \[\])\)",
         'Passing an empty array to chrome.send is unnecessary')
 
+  def CommentIfAndIncludeCheck(self, line_number, line):
+    return self.RegexCheck(line_number, line, r'(?<!\/\/ )(<if|<include) ',
+        '<if> or <include> should be in a single line comment with a space ' +
+        'after the slashes. Examples:\n' +
+        '    // <include src="...">\n' +
+        '    // <if expr="chromeos">\n' +
+        '    // </if>\n')
+
   def ConstCheck(self, i, line):
     """Check for use of the 'const' keyword."""
     if self.input_api.re.search(r'\*\s+@const', line):
@@ -79,39 +87,6 @@ class JSChecker(object):
     """
     return start * ' ' + length * '^'
 
-  def _MakeErrorOrWarning(self, error_text, filename):
-    """Takes a few lines of text indicating a style violation and turns it into
-       a PresubmitError (if |filename| is in a directory where we've already
-       taken out all the style guide violations) or a PresubmitPromptWarning
-       (if it's in a directory where we haven't done that yet).
-    """
-    # TODO(tbreisacher): Once we've cleaned up the style nits in all of
-    # resources/ we can get rid of this function.
-    path = self.input_api.os_path
-    resources = path.join(self.input_api.PresubmitLocalPath(), 'resources')
-    dirs = (
-        path.join(resources, 'bookmark_manager'),
-        path.join(resources, 'extensions'),
-        path.join(resources, 'file_manager'),
-        path.join(resources, 'help'),
-        path.join(resources, 'history'),
-        path.join(resources, 'net_export'),
-        path.join(resources, 'net_internals'),
-        path.join(resources, 'network_action_predictor'),
-        path.join(resources, 'ntp4'),
-        path.join(resources, 'options'),
-        path.join(resources, 'password_manager_internals'),
-        path.join(resources, 'print_preview'),
-        path.join(resources, 'profiler'),
-        path.join(resources, 'sync_promo'),
-        path.join(resources, 'tracing'),
-        path.join(resources, 'uber'),
-    )
-    if filename.startswith(dirs):
-      return self.output_api.PresubmitError(error_text)
-    else:
-      return self.output_api.PresubmitPromptWarning(error_text)
-
   def RunChecks(self):
     """Check for violations of the Chromium JavaScript style guide. See
        http://chromium.org/developers/web-development-style-guide#TOC-JavaScript
@@ -126,13 +101,10 @@ class JSChecker(object):
     for f in affected_js_files:
       error_lines = []
 
-      # Check for the following:
-      # * document.getElementById()
-      # * the 'const' keyword
-      # * Passing an empty array to 'chrome.send()'
       for i, line in enumerate(f.NewContents(), start=1):
         error_lines += filter(None, [
             self.ChromeSendCheck(i, line),
+            self.CommentIfAndIncludeCheck(i, line),
             self.ConstCheck(i, line),
             self.GetElementByIdCheck(i, line),
             self.EndJsDocCommentCheck(i, line),
@@ -147,8 +119,7 @@ class JSChecker(object):
         error_lines = [
             'Found JavaScript style violations in %s:' %
             f.LocalPath()] + error_lines
-        results.append(self._MakeErrorOrWarning(
-            '\n'.join(error_lines), f.AbsoluteLocalPath()))
+        results.append(self.output_api.PresubmitError('\n'.join(error_lines)))
 
     if results:
       results.append(self.output_api.PresubmitNotifyResult(

chrome/browser/web_dev_style/js_checker_test.py

@@ -26,6 +26,44 @@ class JsCheckerTest(SuperMoxTestBase):
     output_api = self.mox.CreateMockAnything()
     self.checker = js_checker.JSChecker(input_api, output_api)
 
+  def ShouldFailCommentCheck(self, line):
+    """Checks that uncommented '<if>' and '<include>' are a style error."""
+    error = self.checker.CommentIfAndIncludeCheck(1, line)
+    self.assertNotEqual('', error, 'Should be flagged as style error: ' + line)
+    highlight = test_util.GetHighlight(line, error).strip()
+    self.assertTrue(highlight.startswith(('<if', '<include')))
+
+  def ShouldPassCommentCheck(self, line):
+    """Checks that commented '<if>' and '<include>' are allowed."""
+    self.assertEqual('', self.checker.CommentIfAndIncludeCheck(1, line),
+        'Should not be flagged as style error: ' + line)
+
+  def testCommentFails(self):
+    lines = [
+        '<include src="blah.js">',
+        # Currently, only "// " is accepted (not just "//" or "//\s+") as Python
+        # can't do variable-length lookbehind.
+        '//<include src="blah.js">',
+        '//  <include src="blah.js">',
+        '             <include src="blee.js">',
+        '  <if expr="chromeos">',
+        '<if expr="lang == \'de\'">',
+        '//<if expr="bitness == 64">',
+    ]
+    for line in lines:
+      self.ShouldFailCommentCheck(line)
+
+  def testCommentPasses(self):
+    lines = [
+        '// <include src="assert.js">',
+        '             // <include src="util.js"/>',
+        '// <if expr="chromeos">',
+        '           // <if expr="not chromeos">',
+        "   '<iframe src=blah.html>';",
+    ]
+    for line in lines:
+      self.ShouldPassCommentCheck(line)
+
   def ShouldFailConstCheck(self, line):
     """Checks that the 'const' checker flags |line| as a style error."""
     error = self.checker.ConstCheck(1, line)