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.

[Ilya Sherman, 2015/01/12 22:17:19]

Ah, I see now that "+?" means non-greedy.  Okay, that makes more sense to me
now.

I'm pretty sure that this regex will mismatch on the following:
"""base::UserMetricsAction(ReverseString("raB.ooF"));"""?  In my experience,
ternary operators within base::UserMetricsAction are fairly rare, so it's not
too much effort to clean them up.  OTOH, getting the regex right to match them,
but to not accidentally mismatch other code, is fairly involved -- assuming it's
even possible with a simple regex.

[Alexei Svitkine (slow), 2015/01/13 18:06:17]

Right. It will also catch base::UserMetricsAction(GetMetricNameFor("Foo")) - so
there will be false positives.

But I think this is OK. The script will prompt the user to add the bad strings
to the actions.xml file - at which point they'll either realise that's not what
they want and restructure their code or worse case scenario we catch it during
metrics code review.

[Ilya Sherman, 2015/01/13 23:05:37]

I guess this is where we disagree -- I would much rather have false negatives
than false positives.  I don't think that Chromium developers always understand
the metrics infrastructure well enough to understand when changes to the XML
files are or aren't surprising.  And, if the code really looks like
base::UserMetricsAction(GetMetricWithSuffix("UserClickedTheRedButton",
one_of_a_hundred_possible_suffixes));, I'm not sure that I would catch the error
in a metrics review, given that I'd see "UserClickedTheRedButton" in
actions.xml.

How common a problem are ternary statements within base::UserMetricsAction?  I
don't recall having encountered this issue ever before, even when I cleaned up a
bunch of other warnings that the script generated.

[Alexei Svitkine (slow), 2015/01/14 18:32:05]

OK, I caved in and changed the code to not support ternary operators and to warn
on all non-literal params. Turns out, to support this, I had to rejigger things
quite a bit and so the code actually needed to be smarter / more complicated to
make it not be smart enough to parse ternary operators - go figure. Anyway, all
done, PTAL.

(There were only a couple cases where the ternary operator was used.)

+  \)                          # Closing parenthesis.
+  """, re.VERBOSE)
+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 FindActionNames(contents, pos):
+  """Finds actions from the first UserMetricsAction() call in |contents|.
+
+  Arguments:
+    contents: string to search through
+    pos: position in |contents| to start the search from
+
+  Returns:
+    A tuple consisting of:
+      - The list action names (strings) that was found.

[Ilya Sherman, 2015/01/12 22:17:19]

nit: "list action names" -> "list of action names"

[Alexei Svitkine (slow), 2015/01/13 18:06:17]

Done.

+      - The index in |content| indicating the end of the matching string.
+  """
+  match = USER_METRICS_ACTION_RE.search(contents, pos=pos)
+  if not match:
+    return None, None
+  return QUOTED_STRING_RE.search(match.group(1)), match.end()
+
 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\([^"]')
-  computed_action_re = re.compile(r'RecordComputedAction')
+
+  contents = open(path).read()
+  pos = 0
+  while True:
+    action_names, pos = FindActionNames(contents, pos)
+    if not action_names:
+      break
+    actions.update(action_names)
+
   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))
 
 class WebUIActionsParser(HTMLParser):
   """Parses an HTML file, looking for all tags with a 'metric' attribute.
   Adds user actions corresponding to any metrics found.
 
   Arguments:
@@ skipping 366 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))