Collect UMA statistics on which chrome://flags lead to chrome restart on ChromeOS.

tools/metrics/histograms/verify_enum_custom_flags.py

+# Copyright 2014 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.
+
+"""Check enum LoginCustomFlags ."""
+
+class LoginCustomFlagsChecker(object):
+  """Verify that changes to enum LoginCustomFlags in histograms.xml are valid.
+
+  This class is used to check that LoginCustomFlags enum is never shrinked.
+
+  Note: this check would probably fail if enum is completely moved to
+  another part of histograms.xml, but it works for changes inside enum.
+  """
+  def __init__(self, input_api, output_api, path):
+    self.input_api = input_api
+    self.output_api = output_api
+    self.path = path
+    self.warnings_exist = False
+    self.enum_start = 0
+    self.enum_end = 0
+    self.results = []
+
+  def LogInfo(self, message):
+    self.input_api.logging.info(message)
+    return
+
+  def LogDebug(self, message):
+    self.input_api.logging.debug(message)
+    return
+
+  def GetLongMessage(self, local_path):
+    return str("The file \"%s\"\n"
+               "contains the definition of the enum LoginCustomFlags,\n"
+               "which should never be shrinked or removed.\n"
+               % (local_path))
+
+  def EmitWarning(self, message, line_number=None, line_text=None):
+    """Emits a presubmit prompt warning containing the short message
+    |message|. |item| is |LOCAL_PATH| with optional |line_number| and
+    |line_text|.
+
+    """
+    if line_number is not None and line_text is not None:
+      item = "%s(%d): %s" % (self.path, line_number, line_text)
+    elif line_number is not None:
+      item = "%s(%d)" % (self.path, line_number)
+    else:
+      item = self.path
+    if self.warnings_exist :
+      long_message = ''
+    else :
+      long_message = self.GetLongMessage(self.path)
+    self.warnings_exist = True
+
+    self.LogInfo(message)
+    self.results.append(
+      self.output_api.PresubmitPromptWarning(message, [item], long_message))
+
+  def ReportDeletedLabels(self, removed_labels, prefix=''):
+    message = prefix
+    message += str("It looks like you are deleting entries from the "
+                  "enum definition. This should never happen.\n")
+    message += "The following labels have been deleted:\n"
+    for value in removed_labels :
+      message += "\t" + value + "\n"
+    self.EmitWarning(message)
+
+  def ExtractFistAndLasteLoginCustomFlagsLineNumberFromHistograms(self):
+    """Returns tuple (first line number, last line number) in the new version.
+       Line number starts with 1.
+    """
+    import os
+    with open(self.path, 'r') as file_handle:
+      line_number = 0
+      enum_start = 0
+      enum_end = 0
+      for line in file_handle:
+        line_number = line_number + 1
+        if self.input_api.re.match(r"^<enum name=\"LoginCustomFlags\"", line):
+          if enum_start > 0:
+            error = str("Bad '" + str(xml_file) + "' format: start of enum "
+                        "LoginCustomFlags found twice: at lines "
+                        + str(enum_start) + " and " + str(line_number)
+                        + ".")
+            self.EmitWarning(error, line_number, line)
+            return
+          enum_start = line_number
+        elif enum_start > 0 and self.input_api.re.match(r"^ *<enum", line):
+          error = str("Bad '" + str(xml_file) + "' format: end of enum "
+                      "LoginCustomFlags not found. Found new enum at line "
+                      + str(line_number) + ":\n" + line)
+          self.EmitWarning(error, line_number, line)
+          return
+        elif enum_start > 0 and self.input_api.re.match(r"^</enum", line):
+          enum_end = line_number
+          break
+
+      if enum_start > 0 and enum_end > enum_start:
+        return (enum_start, enum_end)
+
+      if enum_start == 0:
+        error = str("Bad '" + str(xml_file) + "' format: start of enum "
+                    "LoginCustomFlags is not found.")
+        self.EmitWarning(error, line_number, line)
+        return
+
+      if enum_end == 0:
+        error = str("Bad '" + str(xml_file) + "' format: end of enum "
+                    "LoginCustomFlags is not found.")
+        self.EmitWarning(error, line_number, line)
+        return
+
+      if enum_start == enum_end:
+        error = str("Bad '" + str(xml_file) + "' format: end of enum "
+                    "LoginCustomFlags is empty.")
+        self.EmitWarning(error, line_number, line)
+        return
+
+      raise Exception("Not reached.")
+
+  def CheckDiff(self, affected_file):
+    """Check diff is valid."""
+    added_labels = []
+    removed_labels = []
+    skip_chunk = True
+    line_num = 0
+    old_enum_start_found = False
+    old_enum_end_found = False
+    for line in affected_file.GenerateScmDiff().splitlines():
+      # Parse the unified diff chunk optional section heading, which looks like
+      # @@ -l,s +l,s @@ optional section heading
+      m = self.input_api.re.match(
+        r"^@@ \-([0-9]+)\,([0-9]+) \+([0-9]+)\,([0-9]+) @@", line)
+      if m:
+        old_line_num = int(m.group(1))
+        old_size = int(m.group(2))
+        new_line_num = int(m.group(3))
+        new_size = int(m.group(4))
+        line_num = new_line_num
+        skip_chunk = False
+        if line_num >= self.enum_end:
+          skip_chunk = True
+        elif line_num + new_size <= self.enum_start:
+          skip_chunk = True
+        # This line number applies for the next input line, so subtract 1.
+        line_num -= 1
+        continue
+
+      if skip_chunk:
+        continue
+
+      if not line.startswith("-"):
+        line_num += 1
+
+      if line_num >= self.enum_end:
+        break
+
+      self.LogDebug("Analysing line '" + line + "'")
+
+      if line_num == self.enum_start:
+        self.LogDebug("Found enum start.")
+
+      if line_num >= self.enum_start:
+        if not old_enum_start_found:
+          self.LogDebug("old_enum_start_found is true because "
+                        "line_num >= self.enum_start")
+        old_enum_start_found = True
+      else:
+        m = self.input_api.re.match(
+          r"^([+ -])<enum name=\"LoginCustomFlags\"", line)
+        if m:
+          if m.group(1) == "-" :
+            old_enum_start_found = True
+            self.LogDebug("old_enum_start_found is true because "
+                          "removal of old enum found.")
+          else:
+            # Raise exception if enum LoginCustomFlags found added (or not
+            # modified) at the line number before it was found in current
+            # histograms.xml version.
+            raise Exception("Error parsing changes to histograms.xml:\n"
+                            "Diff line '" + line + "' unexpected.");
+
+      if not old_enum_start_found:
+        continue
+
+      m = self.input_api.re.match(r"^- *</enum", line)
+      if m:
+        old_enum_end_found = True
+        continue
+
+      # Ignore unchanged items here.
+      m = self.input_api.re.match(
+        r"^([+-])  *<int .*label *= *\"?([^\"]+)", line)
+
+      if not m:
+        continue
+
+      self.LogDebug("MATCHED '" + line + "'")
+
+      if m.group(1) == "+" and line_num > self.enum_start and \
+          line_num < self.enum_end:
+        added_labels.append(m.group(2))
+      elif m.group(1) == "-" and old_enum_start_found and \
+          not old_enum_end_found:
+        removed_labels.append(m.group(2))
+
+    self.LogDebug("removed_labels: " + str(removed_labels))
+    self.LogDebug("added_labels: " + str(added_labels))
+
+    removed_completely = []
+    for label in removed_labels:
+      if label not in added_labels:
+        removed_completely.append(label)
+    if len(removed_completely):
+      self.ReportDeletedLabels(removed_completely)
+
+    return False if len(removed_completely) else True
+
+  def PerformChecks(self, affected_file):
+    (self.enum_start, self.enum_end) = \
+        self.ExtractFistAndLasteLoginCustomFlagsLineNumberFromHistograms()
+    self.LogDebug("enum first-last lines are:"
+                 + str((self.enum_start, self.enum_end)))
+
+    if len(self.results) == 0:
+      self.CheckDiff(affected_file)
+
+  def Run(self):
+    for file in self.input_api.AffectedFiles(include_deletes=True):
+      if self.input_api.basename(file.LocalPath()) == \
+          self.input_api.basename(self.path):
+        self.LogInfo("LoginCustomFlagsChecker: Start processing file.")
+        self.PerformChecks(file)
+        self.LogInfo("LoginCustomFlagsChecker: Done processing file.")
+    return self.results

[Ilya Sherman, 2014/08/05 20:14:47]

IMO this presubmit script is not needed now that you're using hashes, since
there aren't any collisions expected.  The reason that the other approach needed
a presubmit check was that one person accidentally inserting an entry in the
wrong spot in the enum would ruin the entire data collection process.  That's
not a concern with hashes.

[Alexei Svitkine (slow), 2014/08/05 20:47:52]

Don't we want to have a presubmit that ensures that histograms.xml is updated as
new flags are added to the C++ code?

[Ilya Sherman, 2014/08/05 20:50:36]

Alex added a unit test that does this.  According to the comment at the top of
the file, the presubmit script just verifies that nothing is removed from the
enum.

[Alexander Alekseev, 2014/08/07 23:24:56]

Unit test cannot check that values are not removed.
So I would better add this check.

[Ilya Sherman, 2014/08/08 03:49:45]

(a) None of the reviewers of this file would be likely to LGTM a CL that removed
a switch.
(b) Nobody editing this file is likely to accidentally remove a switch
(c) Even if a switch were accidentally removed, it would be easy to restore,
e.g. using code history.
(d) No data is corrupted if the histograms.xml file is not up-to-date -- it's
purely metadata.

For all of these reasons, I don't think it's worthwhile to have over 200 lines
of code (plus tests) just to verify that values are not removed.  I strongly
encourage you to either keep the C++ unittest or to write a complete presubmit
check; but not both.

[Alexander Alekseev, 2014/08/09 01:30:01]

Done.