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..a6bca86ac318a3a3327ef8d4f56323ab39c6edf9 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,17 @@ 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
@@ -410,6 +420,23 @@ def AddExtensionActions(actions):
   # 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.
 
@@ -419,22 +446,19 @@ 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\([^"]')
-  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' %