Make histogram_suffixes separators and enum labels required.

It's somewhat common for metric authors to specify the wrong separator when
using a histogram_suffixes element, as the default is underscore, whereas dot is
common. This CL makes it harder to use the wrong separator by making the
attribute required.

Relatedly, this CL makes enum labels required (fixing one enum that lacked
labels), and makes name attributes always required.

BUG=724311
TEST=git cl format
R=asvitkine@chromium.org

Review-Url: https://codereview.chromium.org/2894833005
Cr-Commit-Position: refs/heads/master@{#473425}

tools/metrics/actions/print_style.py

@@ -27,6 +27,13 @@ ATTRIBUTE_ORDER = {
     'with-suffix': ['name'],
 }
 
+# Attribute names that must be explicitly specified on nodes that support them.
+REQUIRED_ATTRIBUTES = [
+    'label',
+    'name',
+    'separator',
+]
+
 # Tag names for top-level nodes whose children we don't want to indent.
 TAGS_THAT_DONT_INDENT = [
     'actions',
@@ -59,6 +66,7 @@ TAGS_ALPHABETIZATION_RULES = {
 def GetPrintStyle():
   """Returns an XmlStyle object for pretty printing actions."""
   return pretty_print_xml.XmlStyle(ATTRIBUTE_ORDER,
+                                   REQUIRED_ATTRIBUTES,
                                    TAGS_THAT_HAVE_EXTRA_NEWLINE,
                                    TAGS_THAT_DONT_INDENT,
                                    TAGS_THAT_ALLOW_SINGLE_LINE,

tools/metrics/common/pretty_print_xml.py

@@ -66,10 +66,11 @@ def SplitParagraphs(text):
 class XmlStyle(object):
   """A class that stores all style specification for an output xml file."""
 
-  def __init__(self, attribute_order, tags_that_have_extra_newline,
-               tags_that_dont_indent, tags_that_allow_single_line,
-               tags_alphabetization_rules):
+  def __init__(self, attribute_order, required_attributes,
+               tags_that_have_extra_newline, tags_that_dont_indent,
+               tags_that_allow_single_line, tags_alphabetization_rules):
     self.attribute_order = attribute_order
+    self.required_attributes = required_attributes
     self.tags_that_have_extra_newline = tags_that_have_extra_newline
     self.tags_that_dont_indent = tags_that_dont_indent
     self.tags_that_allow_single_line = tags_that_allow_single_line
@@ -199,8 +200,27 @@ class XmlStyle(object):
       if not node.childNodes:
         closing_chars = 2
 
-      # Pretty-print the attributes.
       attributes = node.attributes.keys()
+      required_attributes = [attribute for attribute in self.required_attributes
+                             if attribute in self.attribute_order[node.tagName]]
+      missing_attributes = [attribute for attribute in required_attributes
+                            if attribute not in attributes]
+
+      for attribute in missing_attributes:
+        logging.error(
+            'Missing attribute "%s" in tag "%s"', attribute, node.tagName)
+      if missing_attributes:
+        missing_attributes_str = (
+            ', '.join('"%s"' % attribute for attribute in missing_attributes))
+        present_attributes = [
+            ' {0}="{1}"'.format(name, value)
+            for name, value in node.attributes.items()]
+        node_str = '<{0}{1}>'.format(node.tagName, ''.join(present_attributes))
+        raise Error(
+            'Missing attributes {0} in tag "{1}"'.format(
+                missing_attributes_str, node_str))
+
+      # Pretty-print the attributes.
       if attributes:
         # Reorder the attributes.
         unrecognized_attributes = (

tools/metrics/histograms/enums.xml

@@ -15995,15 +15995,14 @@ uploading your change for review.  These are checked by presubmit scripts.
 </enum>
 
 <enum name="FtpDataConnectionError" type="int">
-  <int value="0">Data connection successful</int>
-  <int value="1">Local firewall blocked the connection</int>
-  <int value="2">Connection timed out</int>
-  <int value="3">
-    Connection has been established, but then got broken (either reset or
-    aborted)
-  </int>
-  <int value="4">Connection has been refused</int>
-  <int value="20">Other kind of error</int>
+  <int value="0" label="Data connection successful"/>
+  <int value="1" label="Local firewall blocked the connection"/>
+  <int value="2" label="Connection timed out"/>
+  <int value="3"
+      label="Connection has been established, but then got broken (either
+             reset or aborted)"/>
+  <int value="4" label="Connection has been refused"/>
+  <int value="20" label="Other kind of error"/>
 </enum>
 
 <enum name="FtpServerType" type="int">
@@ -33431,9 +33430,7 @@ from previous Chrome versions.
   <int value="11" label="SELF_SIGNED: The cert is self-signed">
     This cause is recorded only for CERT_AUTHORITY_INVALID errors.
   </int>
-  <int value="12" label="EXPIRED_RECENTLY: Cert expired within last 28 days.">
-
-  </int>
+  <int value="12" label="EXPIRED_RECENTLY: Cert expired within last 28 days."/>
   <int value="13" label="LIKELY_SAME_DOMAIN: (Deprecated)">
     (Deprecated in favor of LIKELY_SAME_DOMAIN2)
   </int>

tools/metrics/histograms/print_style.py

@@ -32,6 +32,15 @@ ATTRIBUTE_ORDER = {
     'with-suffix': ['name'],
 }
 
+# Attribute names that must be explicitly specified on nodes that support them.
+REQUIRED_ATTRIBUTES = [
+    # TODO(isherman): Make the 'label' attribute required as well. This requires
+    # fixing up existing suffixes that omit a label.
+    'name',
+    'separator',
+    'value',
+]
+
 # Tag names for top-level nodes whose children we don't want to indent.
 TAGS_THAT_DONT_INDENT = [
     'histogram-configuration',
@@ -72,6 +81,7 @@ TAGS_ALPHABETIZATION_RULES = {
 def GetPrintStyle():
   """Returns an XmlStyle object for pretty printing histograms."""
   return pretty_print_xml.XmlStyle(ATTRIBUTE_ORDER,
+                                   REQUIRED_ATTRIBUTES,
                                    TAGS_THAT_HAVE_EXTRA_NEWLINE,
                                    TAGS_THAT_DONT_INDENT,
                                    TAGS_THAT_ALLOW_SINGLE_LINE,