Add PRESUBMIT check if modified UMA histogram name can be found (2)

PRESUBMIT.py

 # 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.
 
 """Top-level presubmit script for Chromium.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into gcl.
 """
 
@@ skipping 340 matching lines @@
 
     for line_num, line in f.ChangedContents():
       if 'UNIT_TEST ' in line or line.endswith('UNIT_TEST'):
         problems.append('    %s:%d' % (f.LocalPath(), line_num))
 
   if not problems:
     return []
   return [output_api.PresubmitPromptWarning('UNIT_TEST is only for headers.\n' +
       '\n'.join(problems))]
 
+# Helper function to smartly try to find a histogram name or prefix in a line.

[Alexei Svitkine (slow), 2015/01/30 15:15:36]

Make this a """ comment inside the function.

[mcasas, 2015/01/30 23:45:48]

Done.

+def _FindHistogramNameInLine(histogram_name, line):
+  if not "affected-histogram" in line:
+    return histogram_name in line;

[Alexei Svitkine (slow), 2015/01/30 15:15:37]

Nit: No need for trailing ;

[mcasas, 2015/01/30 23:45:48]

Done.

+  # A histogram_suffixes tag type has an affected-histogram name as a prefix of
+  # the histogram_name.
+  if not '"' in line:
+    return False;
+  histogram_prefix = line.split('\"')[1];
+  return histogram_prefix in histogram_name;
+

[Alexei Svitkine (slow), 2015/01/30 15:15:36]

Nit: Two empty lines between functions.

[mcasas, 2015/01/30 23:45:48]

Done.

+def _CheckUmaHistogramChanges(input_api, output_api):
+  """Check that UMA histogram names in touched lines can still be found in other
+  lines of the patch or in histograms.xml. Note that this check would not catch
+  the reverse: changes in histograms.xml not matched in the code itself."""
+

[Alexei Svitkine (slow), 2015/01/30 15:15:37]

Nit: No blank line.

[mcasas, 2015/01/30 23:45:48]

Done.

+  touched_histograms = []
+  histograms_xml_modifications = []
+  pattern = input_api.re.compile('UMA_HISTOGRAM.*\("(.*)"')
+  for f in input_api.AffectedFiles():
+    # If histograms.xml itself is modified, keep the modified lines for later.
+    if (f.LocalPath().endswith(('histograms.xml'))):

[Alexei Svitkine (slow), 2015/01/30 15:15:36]

Nit: No need for outer parens in Python. Same for the if below.

[mcasas, 2015/01/30 23:45:48]

Done.

+      histograms_xml_modifications = f.ChangedContents()
+      continue
+    if (not f.LocalPath().endswith(('cc', 'mm', 'cpp'))):
+      continue
+    for line_num, line in f.ChangedContents():
+      found = pattern.search(line)
+      if found:
+        touched_histograms.append([found.group(1), f, line_num])
+
+  # Search for the touched histogram names in the local modifications to
+  # histograms.xml, and, if not found, on the base file.
+  problems = []
+  for histogram_name, f, line_num in touched_histograms:
+    histogram_name_found = False
+    for line_num, line in histograms_xml_modifications:
+      histogram_name_found = _FindHistogramNameInLine(histogram_name, line);
+      if histogram_name_found:
+        break;
+    if histogram_name_found:
+      continue
+
+    with open('tools/metrics/histograms/histograms.xml') as histograms_xml:

[Alexei Svitkine (slow), 2015/01/30 15:15:37]

It seems inefficient to open the file and iterate through all of its lines
potentially multiple times.

Can you collect the list of histograms that weren't found and look for them all
at once with a single iteration over histograms.xml lines?

[mcasas, 2015/01/30 23:45:48]

Done.

+      for line in histograms_xml:
+        histogram_name_found = _FindHistogramNameInLine(histogram_name, line);
+        if histogram_name_found:
+          break;
+    if histogram_name_found:
+      continue
+
+    problems.append(' [%s:%d] %s' % (f.LocalPath(), line_num, histogram_name))
+
+  if not problems:
+    return []
+  return [output_api.PresubmitPromptWarning('Some UMA_HISTOGRAM lines have '
+    'been modified and the associated histogram name has no match in either '
+    'metrics/histograms.xml or the modifications of it:',  problems)]
+
 
 def _CheckNoNewWStrings(input_api, output_api):
   """Checks to make sure we don't introduce use of wstrings."""
   problems = []
   for f in input_api.AffectedFiles():
     if (not f.LocalPath().endswith(('.cc', '.h')) or
         f.LocalPath().endswith(('test.cc', '_win.cc', '_win.h')) or
         '/win/' in f.LocalPath()):
       continue
 
@@ skipping 1207 matching lines @@
   return []
 
 
 def CheckChangeOnUpload(input_api, output_api):
   results = []
   results.extend(_CommonChecks(input_api, output_api))
   results.extend(_CheckValidHostsInDEPS(input_api, output_api))
   results.extend(_CheckJavaStyle(input_api, output_api))
   results.extend(
       input_api.canned_checks.CheckGNFormatted(input_api, output_api))
+  results.extend(_CheckUmaHistogramChanges(input_api, output_api))
   return results
 
 
 def GetTryServerMasterForBot(bot):
   """Returns the Try Server master for the given bot.
 
   It tries to guess the master from the bot name, but may still fail
   and return None.  There is no longer a default master.
   """
   # Potentially ambiguous bot names are listed explicitly.
@@ skipping 96 matching lines @@
         if 'presubmit' in builder:
           builders[master].pop(builder)
 
   # Match things like path/aura/file.cc and path/file_aura.cc.
   # Same for chromeos.
   if any(re.search(r'[\\\/_](aura|chromeos)', f) for f in files):
     tryserver_linux = builders.setdefault('tryserver.chromium.linux', {})
     tryserver_linux['linux_chromium_chromeos_asan_rel_ng'] = ['defaulttests']
 
   return builders