Change actions.txt to actions.xml

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 11a2e0018fe3d670569b51dc06b206fba89f110f..f6c4651b640b5dc4f82af96cd23e0a5335bbbbf2 100755
--- a/tools/metrics/actions/extract_actions.py
+++ b/tools/metrics/actions/extract_actions.py
@@ -16,20 +16,31 @@ See also:
   base/metrics/user_metrics.h
   http://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics
 
-If run with a "--hash" argument, chromeactions.txt will be updated.
+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 be changed,

[Ilya Sherman, 2014/02/22 07:07:37]

nit: "needs be" -> "needs to be"

[yao, 2014/02/24 19:16:23]

Done.

+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)'
 
-import hashlib
 from HTMLParser import HTMLParser
+import logging
 import os
 import re
+import shutil
 import sys
+from xml.dom import minidom
+
 
 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
+
 # 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
@@ -115,6 +126,36 @@ REPOSITORY_ROOT = os.path.join(path_utils.ScriptDir(), '..', '..', '..')
 
 number_of_files_total = 0
 
+# Tags that need to be inserted to each 'action' tag and their default content.
+TAGS = {'description': 'Please enter the description of this user action.',
+        'owner': ('Please specify the owners of this user action. ' +
+                  'Add more owner tags as needed')}

[Ilya Sherman, 2014/02/22 07:07:37]

nit: Please end the sentence with a period.

[yao, 2014/02/24 19:16:23]

Done.

+
+# Doc for actions.xml
+_DOC = 'User action XML'

[Ilya Sherman, 2014/02/22 07:07:37]

nit: Why the leading underscore?

[yao, 2014/02/24 19:16:23]

Done.

+
+# Desired order for tag and tag attributes.
+# { tag_name: [attribute_name, ...] }
+ATTRIBUTE_ORDER = {
+    'action': ['name'],
+    'owners': [],
+    'owner':[],
+    'description': [],
+}
+
+# Tag names for top-level nodes whose children we don't want to indent.
+TAGS_THAT_DONT_INDENT = ['actions']
+
+# Extra vertical spacing rules for special tag names.
+# {tag_name: (newlines_after_open, newlines_before_close, newlines_after_close)}
+TAGS_THAT_HAVE_EXTRA_NEWLINE = {
+    'actions': (2, 1, 1),
+    'action': (1, 1, 1),
+}
+
+# Tags that we allow to be squished into a single line for brevity.
+TAGS_THAT_ALLOW_SINGLE_LINE = ['owner']
+
 
 def AddComputedActions(actions):
   """Add computed actions to the actions list.
@@ -578,27 +619,153 @@ def AddAutomaticResetBannerActions(actions):
   actions.add('AutomaticSettingsReset_WebUIBanner_ManuallyClosed')
   actions.add('AutomaticSettingsReset_WebUIBanner_LearnMoreClicked')
 
-def main(argv):
-  if '--hash' in argv:
-    hash_output = True
+
+class Action(object):

[Alexei Svitkine (slow), 2014/02/20 22:50:55]

Can you move this closer to where it's used?

[yao, 2014/02/24 19:16:23]

Done.

+  def __init__(self, name, description, owners):
+    self.name = name
+    self.description = description
+    self.owners = owners
+
+
+class Error(Exception):
+  pass
+
+
+def _ExtractText(parent_dom, tag_name, as_list=True):

[Ilya Sherman, 2014/02/22 07:07:37]

nit: Why the underscores before the method names?  (Alternately, should the
other methods in this file be prefixed with underscores?)

[Ilya Sherman, 2014/02/22 07:07:37]

Having as_list here seems pretty hacky.  Why not just have the client code fetch
the first element from the list, if need be?

[yao, 2014/02/24 19:16:23]

In google3, internal/private function names should start with an underscore. It
seems there's no such requirement in Chromium? 

[yao, 2014/02/24 19:16:23]

Done.

+  """Extract the text enclosed by |tag_name| under |parent_dom|
+
+  Args:
+    parent_dom: The parent Element under which text node is searched for.
+    tag_name: The name of the tag which contains a text node.
+    as_list: If set to True, returns a list of string. Otherwise, returns a

[Alexei Svitkine (slow), 2014/02/20 22:50:55]

nit: "a list of string" -> " a list of strings"

[yao, 2014/02/24 19:16:23]

Done.

+        single string.
+
+  Returns:
+    A (list of) string enclosed by |tag_name| under |parent_dom|.
+  """
+  texts = []
+  for child_dom in parent_dom.getElementsByTagName(tag_name):
+    text_dom = child_dom.childNodes
+    if text_dom.length != 1:
+      raise Error('More than 1 child node exists under %s' % tag_name)
+    if text_dom[0].nodeType != minidom.Node.TEXT_NODE:
+      raise Error('%s\'s child node is not a text node.' % tag_name)
+    texts.append(text_dom[0].data)
+  if not as_list:
+    if texts:
+      return texts[0]

[Ilya Sherman, 2014/02/22 07:07:37]

Should it be an error for tests to have a size not equal to 1 in this case? 
Otherwise, I don't understand why I'd ever want to call this method with as_list
set to False -- I'd potentially be losing data.

[yao, 2014/02/24 19:16:23]

Done.

+    else:
+      return None
+  return texts
+
+
+def _CreateActionTag(top_dom, action_name, action_object):
+  """Create a new action tag.
+
+  Format of an action tag:
+  <action name="name">
+    <owners>
+      <owner>Owner </owner>

[Ilya Sherman, 2014/02/22 07:07:37]

nit: Spurious space after "Owner"?

[yao, 2014/02/24 19:16:23]

Done.

+    </owners>
+    <description>
+      Description.
+    </description>
+  </action>
+
+  If action_name is in actions_dict, the values to be inserted is based on the

[Ilya Sherman, 2014/02/22 07:07:37]

nit: "values ... is" -> "values ... are"

[yao, 2014/02/24 19:16:23]

Done.

+  corresponding Action object. If action_name is not in actions_dict, the
+  default value from TAGS is used.
+
+  Args:
+    top_dom: The parent node under which the new action tag is created.
+    action_name: The name of an action.
+    actions_dict: A map from action name to Action object.
+
+  Returns:
+    An action tag Element with proper children elements.
+  """
+  action_dom = top_dom.createElement('action')
+  action_dom.setAttribute('name', action_name)
+  owners_dom = top_dom.createElement('owners')
+  action_dom.appendChild(owners_dom)
+  description_dom = top_dom.createElement('description')
+  action_dom.appendChild(description_dom)
+
+  # Create description tag.
+  if action_object and action_object.description:
+    # If description for this action is not None, use the store value.
+    # Otherwise, use the default value.
+    description_dom.appendChild(top_dom.createTextNode(
+        action_object.description))
   else:
-    hash_output = False
-    print >>sys.stderr, "WARNING: If you added new UMA tags, you must" + \
-           " use the --hash option to update chromeactions.txt."
-  # if we do a hash output, we want to only append NEW actions, and we know
-  # the file we want to work on
-  actions = set()
+    description_dom.appendChild(top_dom.createTextNode(
+        TAGS.get('description', '')))
+
+  # Create owner tag.
+  if action_object and action_object.owners:
+    # If owners for this action is not None, use the stored value. Otherwise,
+    # use the default value.
+    for owner in action_object.owners:
+      owner_dom = top_dom.createElement('owner')
+      owner_dom.appendChild(top_dom.createTextNode(owner))
+      owners_dom.appendChild(owner_dom)
+  else:
+    # Use default value.
+    owner_dom = top_dom.createElement('owner')
+    owner_dom.appendChild(top_dom.createTextNode(TAGS.get('owner', '')))
+    owners_dom.appendChild(owner_dom)
+  return action_dom
+
+
+def _PrettyPrint(actions, actions_dict):
+  """Given a list of action data, create a well-printed minidom document.
+
+  Args:
+    actions: A list of action names.
+    actions_dict: A mappting from action name to Action object.
 
-  chromeactions_path = os.path.join(path_utils.ScriptDir(), "chromeactions.txt")
+  Returns:
+    A well-printed minidom document that represents the input action data.
+  """
+  # Form new minidom nodes based on updated |actions|.
+  new_dom = minidom.getDOMImplementation().createDocument(None, 'actions', None)
+  top_element = new_dom.documentElement
+  top_element.appendChild(new_dom.createComment(_DOC))
 
-  if hash_output:
-    f = open(chromeactions_path)
-    for line in f:
-      part = line.rpartition("\t")
-      part = part[2].strip()
-      actions.add(part)
-    f.close()
+  # Print out the actions as a sorted list.
+  for action in sorted(actions):
+    top_element.appendChild(_CreateActionTag(new_dom, action,
+                                             actions_dict.get(action, None)))
 
+  # Pretty print the generated minidom node |new_dom|.
+  xml_style = pretty_print_xml.XmlStyle(ATTRIBUTE_ORDER,

[Alexei Svitkine (slow), 2014/02/20 22:50:55]

I noticed you moved this to its own print_style file for histograms? Maybe do
the same here, so that it's consistent?

[yao, 2014/02/24 19:16:23]

Done.

+                                        TAGS_THAT_HAVE_EXTRA_NEWLINE,
+                                        TAGS_THAT_DONT_INDENT,
+                                        TAGS_THAT_ALLOW_SINGLE_LINE)
+  return xml_style.PrettyPrintNode(new_dom)
+
+
+def main(argv):
+  presubmit = ('--presubmit' in argv)
+
+  # A set to store all actions.
+  actions = set()
+  # A map from action name to an Action object.
+  actions_dict = {}
+  actions_xml_path = os.path.join(path_utils.ScriptDir(), 'actions.xml')
+
+  # Save the original file content.
+  with open(actions_xml_path, 'rb') as f:
+    original_xml = f.read()
+
+  # Parse and store the XML data currently stored in file.

[Alexei Svitkine (slow), 2014/02/20 22:50:55]

Nit: "in file" -> "in the file"

[yao, 2014/02/24 19:16:23]

Done.

+  dom = minidom.parse(actions_xml_path)
+  for action_dom in dom.getElementsByTagName('action'):
+    action_name = action_dom.getAttribute('name')
+    actions.add(action_name)
+    description = _ExtractText(action_dom, 'description', False)
+    owners = _ExtractText(action_dom, 'owner', True)
+    actions_dict[action_name] = Action(action_name, description, owners)

[Alexei Svitkine (slow), 2014/02/20 22:50:55]

Nit: Actually, I'd also make a function for this block you're adding, and have
it return the two outputs (actions and actions_dict).

[yao, 2014/02/24 19:16:23]

Done.

 
   AddComputedActions(actions)
   # TODO(fmantek): bring back webkit editor actions.
@@ -620,21 +787,28 @@ def main(argv):
   AddHistoryPageActions(actions)
   AddKeySystemSupportActions(actions)
 
-  if hash_output:
-    f = open(chromeactions_path, "wb")
-
-
-  # Print out the actions as a sorted list.
-  for action in sorted(actions):
-    if hash_output:
-      hash = hashlib.md5()
-      hash.update(action)
-      print >>f, '0x%s\t%s' % (hash.hexdigest()[:16], action)
-    else:
-      print action
-
-  if hash_output:
-    print "Done. Do not forget to add chromeactions.txt to your changelist"
+  pretty = _PrettyPrint(actions, actions_dict)
+  if original_xml == pretty:
+    print 'actions.xml is correctly pretty-printed.'
+    sys.exit(0)
+  if presubmit:
+    logging.info('actions.xml is not formatted correctly; run '
+                 'extract_actions.py to fix.')
+    sys.exit(1)
+
+  # Prompt user to consent on the change.
+  if not diff_util.PromptUserToAcceptDiff(
+      original_xml, pretty, 'Is the new version acceptable?'):
+    logging.error('Aborting')
+    sys.exit(0)

[Ilya Sherman, 2014/02/22 07:07:37]

This should probably be sys.exit(1), since the script failed.

[yao, 2014/02/24 19:16:23]

Done.

+
+  print 'Creating backup file: actions.before.xml.'
+  shutil.move(actions_xml_path, 'actions.before.xml')

[Alexei Svitkine (slow), 2014/02/20 22:50:55]

I'd name this actions.old.xml.

[yao, 2014/02/24 19:16:23]

Done.

+
+  with open(actions_xml_path, 'wb') as f:
+    f.write(pretty)
+  print ('Updated %s. Do not forget to add it to your changelist' %

[Ilya Sherman, 2014/02/22 07:07:37]

Optional nit: "Do not" -> "Don't" (reads more naturally, IMO)

[yao, 2014/02/24 19:16:23]

Done.

+         actions_xml_path)
   return 0