Improve script to find user actions.

tools/metrics/actions/extract_actions.py

Index: tools/metrics/actions/extract_actions.py
diff --git a/tools/metrics/actions/extract_actions.py b/tools/metrics/actions/extract_actions.py
index c7f550f6981f20f64ba124321b25639e6387a2e0..51246b55a338a99483510bb18506dea93d61733b 100755
--- a/tools/metrics/actions/extract_actions.py
+++ b/tools/metrics/actions/extract_actions.py
@@ -14,7 +14,6 @@ there are many possible actions.
 
 See also:
   base/metrics/user_metrics.h
-  http://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics
 
 After extracting all actions, the content will go through a pretty print
 function to make sure it's well formatted. If the file content needs to be
@@ -42,6 +41,19 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'common'))
 import diff_util
 import pretty_print_xml
 
+USER_METRICS_ACTION_RE = re.compile(r"""
+  [^a-zA-Z]                   # Preceded by a non-alphabetical character.
+  UserMetricsAction           # Name of the function.
+  \(                          # Opening parenthesis.
+  \s*                         # Any amount of whitespace, including new lines.
+  (.+?)                       # A sequence of characters for the param.
+  \)                          # Closing parenthesis.
+  """,
+  re.VERBOSE | re.DOTALL      # Verbose syntax and makes . also match new lines.

[Ilya Sherman, 2015/01/14 19:15:20]

Why does . need to match newlines?  Is this behavior actually controlling
whether the "\s*" matches newlines?

[Alexei Svitkine (slow), 2015/01/14 20:28:21]

No, it's whether (.+?) matches newlines.  Without it, it doesn't so
testTernaryUserMetricsActionWithNewLines() fails without it.

[Ilya Sherman, 2015/01/14 23:16:45]

Ah, I see -- it's important for the better error messages.  Fair enough :)

+)
+COMPUTED_ACTION_RE = re.compile(r'RecordComputedAction')
+QUOTED_STRING_RE = re.compile(r'\"(.+?)\"')
+
 # Files that are known to use content::RecordComputedAction(), which means
 # they require special handling code in this script.
 # To add a new file, add it to this list and add the appropriate logic to
@@ -410,6 +422,50 @@ def AddExtensionActions(actions):
   # Actions sent by 'Ok Google' Hotwording.
   actions.add('Hotword.HotwordTrigger')
 
+def _LogWarning(message):
+  print >>sys.stderr, 'WARNING: ' + message

[Ilya Sherman, 2015/01/14 19:15:20]

Please use logging.warning() or logging.error().  (I see that you're just
preserving the previous code, but I'm surprised that the code was previously
written as it was.)

[Alexei Svitkine (slow), 2015/01/14 20:28:21]

Done.

+
+class InvalidStatementException(Exception):
+  """Indicates an invalid statement was found."""
+
+class ActionNameFinder:
+  """Helper class to find action names in source code file."""
+
+  def __init__(self, path, contents):
+    self.path = path
+    self.pos = 0
+    self.contents = contents

[Ilya Sherman, 2015/01/14 19:15:20]

nit: The private vars' names should all start with a double underscore.

[Alexei Svitkine (slow), 2015/01/14 20:28:21]

Done.

+
+  def FindNextAction(self):
+    """Finds the next action name in the file.
+
+    Returns:
+      The name of the action found or None if there are no more actions.
+    Raises:
+      InvalidStatementException if the next action statement is invalid
+      and could not be parsed. There may still be more actions in the file,
+      so FindNextAction() can continue to be called to find following ones.
+    """
+    match = USER_METRICS_ACTION_RE.search(self.contents, pos=self.pos)
+    if not match:
+      return None
+    match_start = match.start()
+    self.pos = match.end()
+    match = QUOTED_STRING_RE.match(match.group(1))
+    if not match:
+      self._RaiseException(match_start, self.pos)
+    return match.group(1)
+
+  def _RaiseException(self, match_start, match_end):
+    """Raises an InvalidStatementException for the specified code range."""
+    line_number = self.contents.count('\n', 0, match_start) + 1
+    # Add 1 to |match_start| since the RE checks the preceding character.
+    statement = self.contents[match_start + 1:match_end]
+    raise InvalidStatementException(
+      '%s uses UserMetricsAction incorrectly on line %d:\n%s' %
+      (self.path, line_number, statement))
+
+
 def GrepForActions(path, actions):
   """Grep a source file for calls to UserMetrics functions.
 
@@ -419,26 +475,24 @@ def GrepForActions(path, actions):
   """
   global number_of_files_total
   number_of_files_total = number_of_files_total + 1
-  # we look for the UserMetricsAction structure constructor
-  # this should be on one line
-  action_re = re.compile(r'[^a-zA-Z]UserMetricsAction\("([^"]*)')
-  malformed_action_re = re.compile(r'[^a-zA-Z]UserMetricsAction\([^"]')

[Ilya Sherman, 2015/01/14 18:48:50]

Hmm, if we're now in agreement to only loosen the restriction around newlines,
why do you need more changes than just adding \s* to each of these regexes?

[Alexei Svitkine (slow), 2015/01/14 18:52:19]

The old code iterates line by line, so that wouldn't work when you want to match
multi-line statements. You need to use the .search() API on the full string and
keep track of the |pos| pointer.

The new code also provides better error messages, actually outputting the
statement in question and is structured in a way that can be tested more easily.

[Ilya Sherman, 2015/01/14 19:15:20]

Fair enough :)

-  computed_action_re = re.compile(r'RecordComputedAction')
+
+  finder = ActionNameFinder(path, open(path).read())
+  while True:
+    try:
+      action_name = finder.FindNextAction()

[Ilya Sherman, 2015/01/14 19:15:20]

It looks like you have dropped the line to update the set of actions.  Is that
intentional?

[Alexei Svitkine (slow), 2015/01/14 20:28:22]

Ack - no, this was accidentally lost when I removed some local print statements
I was using for debugging. Re-added.

+      if not action_name:
+        break
+    except InvalidStatementException, e:
+      _LogWarning(str(e))
+
   line_number = 0
   for line in open(path):
     line_number = line_number + 1
-    match = action_re.search(line)
-    if match:  # Plain call to RecordAction
-      actions.add(match.group(1))
-    elif malformed_action_re.search(line):
-      # Warn if this line is using RecordAction incorrectly.
-      print >>sys.stderr, ('WARNING: %s has malformed call to RecordAction'
-                           ' at %d' % (path, line_number))
-    elif computed_action_re.search(line):
+    if COMPUTED_ACTION_RE.search(line):
       # Warn if this file shouldn't be calling RecordComputedAction.
       if os.path.basename(path) not in KNOWN_COMPUTED_USERS:
-        print >>sys.stderr, ('WARNING: %s has RecordComputedAction at %d' %
-                             (path, line_number))
+          _LogWarning('%s has RecordComputedAction statement on line %d' %
+                      (path, line_number))
 
 class WebUIActionsParser(HTMLParser):
   """Parses an HTML file, looking for all tags with a 'metric' attribute.