Presubmit checks for user actions intorduced in HTML files.

chrome/browser/resources/PRESUBMIT.py

+# Copyright 2014 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 HTML files in chrome/browser/resources.
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details about the presubmit API built into gcl.
+"""
+
+import re
+
+ACTION_XML_PATH = '../../../tools/metrics/actions/actions.xml'
+
+def CheckUserActionUpdate(input_api, output_api, action_xml_path):
+  """Checks if any new user action has been added."""
+  if any('actions.xml' == input_api.os_path.basename(f.LocalPath()) for f in
+         input_api.AffectedFiles()):
+    # If actions.xml is already included in the changelist, the PRESUBMIT
+    # for actions.xml will do a more complete presubmit check.
+    return []
+
+  file_filter = lambda f: f.LocalPath().endswith(('.html'))
+  action_re = r'(^|\s+)metric\s*=\s*"([^ ]*)"'
+  current_actions = None
+  for f in input_api.AffectedFiles(file_filter=file_filter):
+    for line_num, line in f.ChangedContents():
+      match = input_api.re.search(action_re, line)
+      if match:
+        # Loads contents in tools/metrics/actions/actions.xml to memory. It's
+        # loaded only once.
+        if not current_actions:
+          with open(action_xml_path) as actions_f:
+            current_actions = actions_f.read()
+
+        action_name = match.group(2)

[Alexei Svitkine (slow), 2014/11/15 01:07:48]

Hmm, maybe name this metric_name rather than action_name (to match the param
it's being extracted from and the fact that it might not be the actual name of
any action (due to suffixes). Please change throughout.

[gayane -on leave until 09-2017, 2014/11/18 18:18:57]

Done.

+        is_boolean = IsBoolean(f.NewContents(), action_name)
+        # Search for the matched user action name in |current_actions|.
+        if not IsActionPresent(current_actions, action_name, is_boolean):
+          return [output_api.PresubmitPromptWarning(
+            'File %s line %d: %s is missing in '
+            'tools/metrics/actions/actions.xml. Please run '
+            'tools/metrics/actions/extract_actions.py to update.'
+            % (f.LocalPath(), line_num, action_name), [])]
+  return []
+
+
+def IsActionPresent(current_actions, action_name, is_boolean):
+  """Checks if action_name is defined in the actions file.
+
+  This function tries to match the action definition for the provided action
+  name if it is not boolean action. If it is boolean then it tries to match
+  first the disabled action. In case of success it contiunes to search for
+  enabled one.

[Alexei Svitkine (slow), 2014/11/15 01:07:48]

You can make this shorter by describing the purpose of the function, rather than
the implementation details.

e.g.

Checks whether there's matching entries in an actions.xml file for the given
|action_name|, depending on whether it is a boolean action.

[gayane -on leave until 09-2017, 2014/11/18 18:18:57]

Done.

+
+  Args:
+    current_actions: The content of the actions.xml file.
+    action_name: The action_name for which the check should be done.
+    is_boolean: The identifer for the action type.

[Alexei Svitkine (slow), 2014/11/15 01:07:48]

What's an identifier for the action type? It would be clearer if you just say
"Whether the action comes from a boolean control."

[gayane -on leave until 09-2017, 2014/11/18 18:18:57]

Done.

+  """
+  if not is_boolean:
+    action = 'name="{0}"'.format(action_name)
+    return action in current_actions
+
+  action_disabled = 'name="{0}_Disabled"'.format(action_name)
+  action_enabled = 'name="{0}_Enabled"'.format(action_name)
+
+  pos = current_actions.find(action_disabled)
+  return pos >= 0 and current_actions.find(action_enabled, pos) >= 0

[Alexei Svitkine (slow), 2014/11/15 01:07:48]

Why does "action in current_actions" work for the above case, but these need the
.find() cases?

Is it just an optimization so that you can pass in pos? Even if that's the
intention, I don't think it's worth it. I don't think trying to optimize the
case where there is an action in the CL, since that's more rare and is already
going to run a bigger check.

[gayane -on leave until 09-2017, 2014/11/18 18:18:57]

Done.

+
+
+def IsBoolean(new_content_lines, action_name):
+  """Check whether action defined in the changed code is boolean or not.
+
+  This function first tries to match the HTML tag in which the action has been
+  mention. Afterwards it tries to match boolean indicators from the remaining
+  text ot the HTML tag.
+
+  Args:
+    new_content_lines: List of changed lines.
+    action_name: Action name for which the check should be done.
+  """
+  new_content = '\n'.join(new_content_lines)
+
+  html_element_re = r'<(.*?)(^|\s+)metric\s*=\s*"%s"(.*?)>' % (action_name)
+  type_re = r'datatype="boolean"|type="checkbox"|type="radio".*?'\
+             'value=("true"|"false")'
+
+  match = re.search(html_element_re, new_content)
+  if match:
+    left_content = match.group(1)
+    right_content = match.group(3)
+    if re.search(type_re, left_content) or re.search(type_re, right_content):
+      return True
+  return False
+
+
+def CheckChangeOnUpload(input_api, output_api):
+  return CheckUserActionUpdate(input_api, output_api, ACTION_XML_PATH)
+
+
+def CheckChangeOnCommit(input_api, output_api):
+  return CheckUserActionUpdate(input_api, output_api, ACTION_XML_PATH)