Change actions.txt to actions.xml

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
 
-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
 # 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 65 matching lines @@
   'xkb:sk::slo', 'xkb:tr::tur', 'xkb:ua::ukr', 'xkb:us::eng',
   'xkb:us:altgr-intl:eng', 'xkb:us:colemak:eng', 'xkb:us:dvorak:eng',
   'xkb:us:intl:eng',
 )
 
 # The path to the root of the repository.
 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.
 
   Arguments:
     actions: set of actions to add to.
   """
 
   # Actions for back_forward_menu_model.cc.
   for dir in ('BackMenu_', 'ForwardMenu_'):
@@ skipping 443 matching lines @@
   actions.add('AutomaticReset_WebUIBanner_BannerShown')
   actions.add('AutomaticReset_WebUIBanner_ManuallyClosed')
   actions.add('AutomaticReset_WebUIBanner_ResetClicked')
 
   # These actions relate to the the automatic settings reset banner shown as
   # a result of settings hardening.
   actions.add('AutomaticSettingsReset_WebUIBanner_BannerShown')
   actions.add('AutomaticSettingsReset_WebUIBanner_ManuallyClosed')
   actions.add('AutomaticSettingsReset_WebUIBanner_LearnMoreClicked')
 
+
+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:
+    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.
+
+  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))
+
+  # 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):
-  if '--hash' in argv:
-    hash_output = True
-  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
+  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')
 
-  chromeactions_path = os.path.join(path_utils.ScriptDir(), "chromeactions.txt")
+  # Save the original file content.
+  with open(actions_xml_path, 'rb') as f:
+    original_xml = f.read()
 
-  if hash_output:
-    f = open(chromeactions_path)
-    for line in f:
-      part = line.rpartition("\t")
-      part = part[2].strip()
-      actions.add(part)
-    f.close()
-
+  # 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.
   # AddWebKitEditorActions(actions)
   AddAboutFlagsActions(actions)
   AddWebUIActions(actions)
 
   AddLiteralActions(actions)
 
   # print "Scanned {0} number of files".format(number_of_files_total)
   # print "Found {0} entries".format(len(actions))
 
   AddAndroidActions(actions)
   AddAutomaticResetBannerActions(actions)
   AddBookmarkManagerActions(actions)
   AddChromeOSActions(actions)
   AddClosedSourceActions(actions)
   AddExtensionActions(actions)
   AddHistoryPageActions(actions)
   AddKeySystemSupportActions(actions)
 
-  if hash_output:
-    f = open(chromeactions_path, "wb")
+  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 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
+  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.

 
-  if hash_output:
-    print "Done. Do not forget to add chromeactions.txt to your changelist"
+  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
 
 
 if '__main__' == __name__:
   sys.exit(main(sys.argv))