Improve script to find user actions.

tools/metrics/actions/extract_actions.py

 #!/usr/bin/env python
 #
 # 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.
 
 """Extract UserMetrics "actions" strings from the Chrome source.
 
 This program generates the list of known actions we expect to see in the
 user behavior logs.  It walks the Chrome source, looking for calls to
 UserMetrics functions, extracting actions and warning on improper calls,
 as well as generating the lists of possible actions in situations where
 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
 changed, a window will be prompted asking for user's consent. The old version
 will also be saved in a backup file.
 """
 
 __author__ = 'evanm (Evan Martin)'
 
 from HTMLParser import HTMLParser
 import logging
 import os
 import re
 import shutil
 import sys
 from xml.dom import minidom
 
 import print_style
 
 sys.path.insert(1, os.path.join(sys.path[0], '..', '..', 'python'))
 from google import path_utils
 
 # Import the metrics/common module for pretty print xml.
 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
 # generate the known actions to AddComputedActions() below.
 KNOWN_COMPUTED_USERS = (
   'back_forward_menu_model.cc',
   'options_page_view.cc',
   'render_view_host.cc',  # called using webkit identifiers
   'user_metrics.cc',  # method definition
   'new_tab_ui.cc',  # most visited clicks 1-9
@@ skipping 348 matching lines @@
   actions.add('ConnectivityDiagnostics.LaunchSource.WebStore')
   actions.add('ConnectivityDiagnostics.UA.LogsShown')
   actions.add('ConnectivityDiagnostics.UA.PassingTestsShown')
   actions.add('ConnectivityDiagnostics.UA.SettingsShown')
   actions.add('ConnectivityDiagnostics.UA.TestResultExpanded')
   actions.add('ConnectivityDiagnostics.UA.TestSuiteRun')
 
   # 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.
 
   Arguments:
     path: path to the file
     actions: set of actions to add to
   """
   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.
   Adds user actions corresponding to any metrics found.
 
   Arguments:
     actions: set of actions to add to
   """
   def __init__(self, actions):
     HTMLParser.__init__(self)
@@ skipping 362 matching lines @@
 
   with open(actions_xml_path, 'wb') as f:
     f.write(pretty)
   print ('Updated %s. Don\'t forget to add it to your changelist' %
          actions_xml_path)
   return 0
 
 
 if '__main__' == __name__:
   sys.exit(main(sys.argv))