Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(720)

Unified Diff: media/PRESUBMIT.py

Issue 148553003: Clean up histogram'd media enum max values. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: fix naming collision Created 6 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « content/renderer/media/webrtc_audio_renderer.cc ('k') | media/PRESUBMIT_test.py » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: media/PRESUBMIT.py
diff --git a/media/PRESUBMIT.py b/media/PRESUBMIT.py
index 255769720f4f86dd98f64aa352c14a4ba27d5420..279115ccb2fd596e6f50efb053fa1653cc442cce 100644
--- a/media/PRESUBMIT.py
+++ b/media/PRESUBMIT.py
@@ -87,10 +87,83 @@ def _CheckForMessageLoopProxy(input_api, output_api):
return []
+def _CheckForHistogramOffByOne(input_api, output_api):
+ """Make sure histogram enum maxes are used properly"""
+
+ # A general-purpose chunk of regex to match whitespace and/or comments
+ # that may be interspersed with the code we're interested in:
+ comment = r'/\*.*?\*/|//[^\n]*'
+ whitespace = r'(?:[\n\t ]|(?:' + comment + r'))*'
+
+ # The name is assumed to be a literal string.
+ histogram_name = r'"[^"]*"'
+
+ # This can be an arbitrary expression, so just ensure it isn't a ; to prevent
+ # matching past the end of this statement.
+ histogram_value = r'[^;]*'
+
+ # In parens so we can retrieve it for further checks.
+ histogram_max = r'([^;,]*)'
+
+ # This should match a uma histogram enumeration macro expression.
+ uma_macro_re = input_api.re.compile(
+ r'\bUMA_HISTOGRAM_ENUMERATION\(' + whitespace + histogram_name + r',' +
+ whitespace + histogram_value + r',' + whitespace + histogram_max +
+ whitespace + r'\)' + whitespace + r';(?:' + whitespace +
+ r'\/\/ (PRESUBMIT_IGNORE_UMA_MAX))?')
+
+ uma_max_re = input_api.re.compile(r'.*(?:Max|MAX).* \+ 1')
+
+ problems = []
+
+ for f in input_api.AffectedSourceFiles(_FilterFile):
+ contents = input_api.ReadFile(f)
+
+ # We want to match across lines, but still report a line number, so we keep
+ # track of the line we're on as we search through the file.
+ line_number = 1
+
+ # We search the entire file, then check if any violations are in the changed
+ # areas, this is inefficient, but simple. A UMA_HISTOGRAM_ENUMERATION call
+ # will often span multiple lines, so finding a match looking just at the
+ # deltas line-by-line won't catch problems.
+ match = uma_macro_re.search(contents)
+ while match:
+ line_number += contents.count('\n', 0, match.start())
+ max_arg = match.group(1) # The third argument.
+
+ if (not uma_max_re.match(max_arg) and match.group(2) !=
+ 'PRESUBMIT_IGNORE_UMA_MAX'):
+ uma_range = range(match.start(), match.end() + 1)
+ # Check if any part of the match is in the changed lines:
+ for num, line in f.ChangedContents():
+ if line_number <= num <= line_number + match.group().count('\n'):
+ problems.append('%s:%d' % (f, line_number))
+ break
+
+ # Strip off the file contents up to the end of the match and update the
+ # line number.
+ contents = contents[match.end():]
+ line_number += match.group().count('\n')
+ match = uma_macro_re.search(contents)
+
+ if problems:
+ return [output_api.PresubmitError(
+ 'UMA_HISTOGRAM_ENUMERATION reports in src/media/ are expected to adhere\n'
+ 'to the following guidelines:\n'
+ ' - The max value (3rd argument) should be an enum value equal to the\n'
+ ' last valid value, e.g. FOO_MAX = LAST_VALID_FOO.\n'
+ ' - 1 must be added to that max value.\n'
+ 'Contact rileya@chromium.org if you have questions.' , problems)]
+
+ return []
+
+
def _CheckChange(input_api, output_api):
results = []
results.extend(_CheckForUseOfWrongClock(input_api, output_api))
results.extend(_CheckForMessageLoopProxy(input_api, output_api))
+ results.extend(_CheckForHistogramOffByOne(input_api, output_api))
return results
« no previous file with comments | « content/renderer/media/webrtc_audio_renderer.cc ('k') | media/PRESUBMIT_test.py » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698