Fix presubmit_scheme_histograms.py and presubmit_bad_message_reasons.py

This CL fixes presubmit_scheme_histograms.py and
presubmit_bad_message_reasons.py which after r462591 would always report
presubmit errors (because it started returning a tuple from HistogramNeedsUpdate
which always coerces to True in the broken presubmit scripts).

After r462591 HistogramNeedsUpdate effectively checks all
histogram-enum-related presubmits (both for updating histograms.xml and
for detecting duplicate values).  Therefore the current CL renames this
function to CheckPresubmitErrors and makes it return a presubmit error
(rather than a tuple of (needs_updating, duplicates)).  This refactoring
has a nice side benefit - after this CL, all presubmits going through
CheckPresubmitErrors will check for duplicate values (r462591 only added
this check for UseCounter histograms).

This CL also tweaks content/browser/PRESUBMIT.py so that it applies
*both* to upload-time and commit-time checks (without this change
|git cl presubmit| would not hit the checks related to
content/browser/bad_message.h - they would only be hit when running
|git cl presubmit --upload|).

Manual testing done:
- Make changes in content/browser/bad_message.h,
  chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc,
  third_party/WebKit/Source/code/frame
- Run git cl presubmit and verify that the presubmit
  checks detect 1) duplicates and 2) the need to update histograms.xml

BUG=577772
TEST=See "Manual testing done" above.
TBR=sky@chromium.org, jochen@chromium.org, rdevlin.cronin@chromium.org

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

chrome/browser/PRESUBMIT.py

@@ -21,7 +21,7 @@ def CheckChangeOnCommit(input_api, output_api):
 
 def _RunHistogramChecks(input_api, output_api, histogram_name):
   try:
-    # Setup sys.path so that we can call histrogram code
+    # Setup sys.path so that we can call histograms code.
     import sys
     original_sys_path = sys.path
     sys.path = sys.path + [input_api.os_path.join(

components/nacl/browser/PRESUBMIT.py

@@ -8,7 +8,7 @@ match the corresponding bad_message.h file.
 
 def _RunHistogramChecks(input_api, output_api, histogram_name):
   try:
-    # Setup sys.path so that we can call histrogram code
+    # Setup sys.path so that we can call histograms code.
     import sys
     original_sys_path = sys.path
     sys.path = sys.path + [input_api.os_path.join(

components/password_manager/content/browser/PRESUBMIT.py

@@ -8,7 +8,7 @@ match the corresponding bad_message.h file.
 
 def _RunHistogramChecks(input_api, output_api, histogram_name):
   try:
-    # Setup sys.path so that we can call histrogram code
+    # Setup sys.path so that we can call histograms code.
     import sys
     original_sys_path = sys.path
     sys.path = sys.path + [input_api.os_path.join(

content/browser/PRESUBMIT.py

@@ -6,9 +6,15 @@
 match the corresponding bad_message.h file.
 """
 
+def CheckChangeOnCommit(input_api, output_api):
+  return _CommonChecks(input_api, output_api)
+
+def CheckChangeOnUpload(input_api, output_api):
+  return _CommonChecks(input_api, output_api)
+
 def _RunHistogramChecks(input_api, output_api, histogram_name):
   try:
-    # Setup sys.path so that we can call histrogram code
+    # Setup sys.path so that we can call histograms code.
     import sys
     original_sys_path = sys.path
     sys.path = sys.path + [input_api.os_path.join(
@@ -24,5 +30,5 @@ def _RunHistogramChecks(input_api, output_api, histogram_name):
   finally:
     sys.path = original_sys_path
 
-def CheckChangeOnUpload(input_api, output_api):
+def _CommonChecks(input_api, output_api):
   return _RunHistogramChecks(input_api, output_api, "BadMessageReasonContent")

extensions/browser/PRESUBMIT.py

@@ -36,7 +36,7 @@ def _RunHistogramValueCheckers(input_api, output_api):
 
 def _RunHistogramChecks(input_api, output_api, histogram_name):
   try:
-    # Setup sys.path so that we can call histrogram code
+    # Setup sys.path so that we can call histograms code.
     import sys
     original_sys_path = sys.path
     sys.path = sys.path + [input_api.os_path.join(

third_party/WebKit/Source/core/frame/PRESUBMIT.py

@@ -74,24 +74,16 @@ def _RunUmaHistogramChecks(input_api, output_api):
 
     start_marker = '^enum Feature : uint32_t {'
     end_marker = '^kNumberOfFeatures'
-    should_update_histogram, duplicated_values = update_histogram_enum.HistogramNeedsUpdate(
+    presubmit_error = update_histogram_enum.CheckPresubmitErrors(
         histogram_enum_name='FeatureObserver',
+        update_script_name='update_use_counter_feature_enum.py',
         source_enum_path=source_path,
         start_marker=start_marker,
         end_marker=end_marker,
         strip_k_prefix=True)
-    if duplicated_values:
-        return [output_api.PresubmitPromptWarning(
-            'UseCounter::Feature has been updated and there exists duplicated '
-            'values between (%s) and (%s)' % duplicated_values,
-            items=[source_path])]
-    if should_update_histogram:
-        return [output_api.PresubmitPromptWarning(
-            'UseCounter::Feature has been updated and the UMA mapping needs to '
-            'be regenerated. Please run update_use_counter_feature_enum.py in '
-            'src/tools/metrics/histograms/ to update the mapping.',
-            items=[source_path])]
-
+    if presubmit_error:
+        return [output_api.PresubmitPromptWarning(presubmit_error,
+                                                  items=[source_path])]
     return []
 
 

tools/metrics/histograms/presubmit_bad_message_reasons.py

@@ -26,13 +26,13 @@ def PrecheckBadMessage(input_api, output_api, histogram_name):
 
   START_MARKER='^enum (class )?BadMessageReason {'
   END_MARKER='^BAD_MESSAGE_MAX'
-  if update_histogram_enum.HistogramNeedsUpdate(
+  presubmit_error = update_histogram_enum.CheckPresubmitErrors(
       histogram_enum_name=histogram_name,
+      update_script_name='update_bad_message_reasons.py',
       source_enum_path=source_path,
       start_marker=START_MARKER,
-      end_marker=END_MARKER):
-    return [output_api.PresubmitPromptWarning(
-        'bad_messages.h has been updated but histogram.xml does not '
-        'appear to be updated.\nPlease run:\n'
-        '  python tools/metrics/histograms/update_bad_message_reasons.py\n')]
+      end_marker=END_MARKER)
+  if presubmit_error:
+    return [output_api.PresubmitPromptWarning(presubmit_error,
+                                              items=[source_path])]
   return []

tools/metrics/histograms/presubmit_scheme_histograms.py

@@ -18,14 +18,13 @@ def PrecheckShouldAllowOpenURLEnums(input_api, output_api):
   if source_file not in affected_files:
     return []
 
-  if update_histogram_enum.HistogramNeedsUpdate(
+  presubmit_error = update_histogram_enum.CheckPresubmitErrors(
       histogram_enum_name='ShouldAllowOpenURLFailureScheme',
+      update_script_name='update_should_allow_open_url_histograms.py',
       source_enum_path=source_file,
       start_marker='^enum ShouldAllowOpenURLFailureScheme {',
-      end_marker='^SCHEME_LAST'):
-    return [output_api.PresubmitPromptWarning(
-        'ShouldAllowOpenURLFailureScheme has been updated but histogram.xml '
-        'does not appear to be updated.\nPlease run:\n'
-        '  python tools/metrics/histograms/'
-        'update_should_allow_open_url_histograms.py\n')]
+      end_marker='^SCHEME_LAST')
+  if presubmit_error:
+    return [output_api.PresubmitPromptWarning(presubmit_error,
+                                              items=[source_file])]
   return []

tools/metrics/histograms/update_histogram_enum.py

@@ -87,12 +87,12 @@ def ReadHistogramValues(filename, start_marker, end_marker, strip_k_prefix):
         label = m.group(1)
       else:
         continue
-    # If two enum labels have the same value
-    if enum_value in result:
-      return result, (result[enum_value], label)
     if strip_k_prefix:
       assert label.startswith('k'), "Enum " + label + " should start with 'k'."
       label = label[1:]
+    # If two enum labels have the same value
+    if enum_value in result:
+      return result, (result[enum_value], label)
     result[enum_value] = label
     enum_value += 1
   return result, None
@@ -180,30 +180,45 @@ def _GetOldAndUpdatedXml(histogram_enum_name, source_enum_values,
   return (xml, new_xml)
 
 
-def HistogramNeedsUpdate(histogram_enum_name, source_enum_path, start_marker,
+def CheckPresubmitErrors(histogram_enum_name, update_script_name,
+                         source_enum_path, start_marker,
                          end_marker, strip_k_prefix = False):
-  """Reads a C++ enum from a .h file and does a dry run of updating
-  histograms.xml to match. Returns true if the histograms.xml file would be
-  changed.
+  """Reads a C++ enum from a .h file and checks for presubmit violations:
+  1. Failure to update histograms.xml to match
+  2. Introduction of duplicate values.
 
   Args:
       histogram_enum_name: The name of the XML <enum> attribute to update.
+      update_script_name: The name of an update script to run to update the UMA
+          mappings for the enum.
       source_enum_path: A unix-style path, relative to src/, giving
           the C++ header file from which to read the enum.
       start_marker: A regular expression that matches the start of the C++ enum.
       end_marker: A regular expression that matches the end of the C++ enum.
       strip_k_prefix: Set to True if enum values are declared as kFoo and the
           'k' should be stripped.
+
+  Returns:
+      A string with presubmit failure description, or None (if no failures).
   """
   Log('Reading histogram enum definition from "{0}".'.format(source_enum_path))
   source_enum_values, duplicated_values = ReadHistogramValues(
       source_enum_path, start_marker, end_marker, strip_k_prefix)
+
   if duplicated_values:
-    return False, duplicated_values
+    return ('%s enum has been updated and there exist '
+            'duplicated values between (%s) and (%s)' % (histogram_enum_name,
+                                                         duplicated_values[0],
+                                                         duplicated_values[1]))
 
   (xml, new_xml) = _GetOldAndUpdatedXml(histogram_enum_name, source_enum_values,
                                         source_enum_path)
-  return xml != new_xml, None
+  if xml != new_xml:
+    return ('%s enum has been updated and the UMA mapping needs to be '
+            'regenerated. Please run %s in src/tools/metrics/histograms/ to '
+            'update the mapping.' % (histogram_enum_name, update_script_name))
+
+  return None
 
 
 def UpdateHistogramFromDict(histogram_enum_name, source_enum_values,