diff options
32 files changed, 902 insertions, 795 deletions
diff --git a/chrome/browser/extensions/PRESUBMIT.py b/chrome/browser/extensions/PRESUBMIT.py deleted file mode 100644 index 31cac0c..0000000 --- a/chrome/browser/extensions/PRESUBMIT.py +++ /dev/null @@ -1,22 +0,0 @@ -# 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. - -"""Chromium presubmit script for src/chrome/browser/extensions. - -See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts -for more details on the presubmit API built into gcl. -""" - -def GetPreferredTryMasters(project, change): - return { - 'tryserver.chromium': { - 'linux_chromium_chromeos_rel': set(['defaulttests']), - } - } - -def CheckChangeOnUpload(input_api, output_api): - results = [] - results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) - return results - diff --git a/extensions/browser/PRESUBMIT.py b/extensions/browser/PRESUBMIT.py index 29a709b..659c176 100644 --- a/extensions/browser/PRESUBMIT.py +++ b/extensions/browser/PRESUBMIT.py @@ -8,6 +8,8 @@ See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts for more details on the presubmit API built into gcl. """ +import sys + def GetPreferredTryMasters(project, change): return { 'tryserver.chromium': { @@ -15,294 +17,26 @@ def GetPreferredTryMasters(project, change): } } -class HistogramValueChecker(object): - """Verify that changes to "extension_function_histogram_value.h" are valid. - - See comments at the top of the "extension_function_histogram_value.h" file - for what are considered valid changes. There are situations where this script - gives false positive warnings, i.e. it warns even though the edit is - legitimate. Since the script warns using prompt warnings, the user can always - choose to continue. The main point is to attract the attention to all - (potentially or not) invalid edits. - - """ - - # The name of the file we want to check against - LOCAL_PATH = "extensions/browser/extension_function_histogram_value.h" - - # The markers we look for in the above source file as delimiters of the enum - # definition. - ENUM_START_MARKER = "enum HistogramValue {" - ENUM_END_MARKER = " // Last entry:" - - def __init__(self, input_api, output_api): - self.input_api = input_api - self.output_api = output_api - self.results = [] - - class EnumRange(object): - """Represents a range of line numbers (1-based)""" - def __init__(self, first_line, last_line): - self.first_line = first_line - self.last_line = last_line - - def Count(self): - return self.last_line - self.first_line + 1 - - def Contains(self, line_num): - return self.first_line <= line_num and line_num <= self.last_line - - def LogInfo(self, message): - self.input_api.logging.info(message) - return - - def LogDebug(self, message): - self.input_api.logging.debug(message) - return - - def ComputeEnumRangeInContents(self, contents): - """Returns an |EnumRange| object representing the line extent of the - HistogramValue enum members in |contents|. The line numbers are 1-based, - compatible with line numbers returned by AffectedFile.ChangeContents(). - |contents| is a list of strings reprenting the lines of a text file. - - If either ENUM_START_MARKER or ENUM_END_MARKER cannot be found in - |contents|, returns None and emits detailed warnings about the problem. - - """ - first_enum_line = 0 - last_enum_line = 0 - line_num = 1 # Line numbers are 1-based - for line in contents: - if line.startswith(self.ENUM_START_MARKER): - first_enum_line = line_num + 1 - elif line.startswith(self.ENUM_END_MARKER): - last_enum_line = line_num - line_num += 1 - - if first_enum_line == 0: - self.EmitWarning("The presubmit script could not find the start of the " - "enum definition (\"%s\"). Did the enum definition " - "change?" % self.ENUM_START_MARKER) - return None - - if last_enum_line == 0: - self.EmitWarning("The presubmit script could not find the end of the " - "enum definition (\"%s\"). Did the enum definition " - "change?" % self.ENUM_END_MARKER) - return None - - if first_enum_line >= last_enum_line: - self.EmitWarning("The presubmit script located the start of the enum " - "definition (\"%s\" at line %d) *after* its end " - "(\"%s\" at line %d). Something is not quite right." - % (self.ENUM_START_MARKER, first_enum_line, - self.ENUM_END_MARKER, last_enum_line)) - return None - - self.LogInfo("Line extent of |HistogramValue| enum definition: " - "first_line=%d, last_line=%d." - % (first_enum_line, last_enum_line)) - return self.EnumRange(first_enum_line, last_enum_line) - - def ComputeEnumRangeInNewFile(self, affected_file): - return self.ComputeEnumRangeInContents(affected_file.NewContents()) - - def GetLongMessage(self): - return str("The file \"%s\" contains the definition of the " - "|HistogramValue| enum which should be edited in specific ways " - "only - *** read the comments at the top of the header file ***" - ". There are changes to the file that may be incorrect and " - "warrant manual confirmation after review. Note that this " - "presubmit script can not reliably report the nature of all " - "types of invalid changes, especially when the diffs are " - "complex. For example, an invalid deletion may be reported " - "whereas the change contains a valid rename." - % self.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.LOCAL_PATH, line_number, line_text) - elif line_number is not None: - item = "%s(%d)" % (self.LOCAL_PATH, line_number) - else: - item = self.LOCAL_PATH - long_message = self.GetLongMessage() - self.LogInfo(message) - self.results.append( - self.output_api.PresubmitPromptWarning(message, [item], long_message)) - - def CollectRangesInsideEnumDefinition(self, affected_file, - first_line, last_line): - """Returns a list of triplet (line_start, line_end, line_text) of ranges of - edits changes. The |line_text| part is the text at line |line_start|. - Since it used only for reporting purposes, we do not need all the text - lines in the range. - - """ - results = [] - previous_line_number = 0 - previous_range_start_line_number = 0 - previous_range_start_text = "" - - def addRange(): - tuple = (previous_range_start_line_number, - previous_line_number, - previous_range_start_text) - results.append(tuple) - - for line_number, line_text in affected_file.ChangedContents(): - if first_line <= line_number and line_number <= last_line: - self.LogDebug("Line change at line number " + str(line_number) + ": " + - line_text) - # Start a new interval if none started - if previous_range_start_line_number == 0: - previous_range_start_line_number = line_number - previous_range_start_text = line_text - # Add new interval if we reached past the previous one - elif line_number != previous_line_number + 1: - addRange() - previous_range_start_line_number = line_number - previous_range_start_text = line_text - previous_line_number = line_number - - # Add a last interval if needed - if previous_range_start_line_number != 0: - addRange() - return results - - def CheckForFileDeletion(self, affected_file): - """Emits a warning notification if file has been deleted """ - if not affected_file.NewContents(): - self.EmitWarning("The file seems to be deleted in the changelist. If " - "your intent is to really delete the file, the code in " - "PRESUBMIT.py should be updated to remove the " - "|HistogramValueChecker| class."); - return False - return True - - def GetDeletedLinesFromScmDiff(self, affected_file): - """Return a list of of line numbers (1-based) corresponding to lines - deleted from the new source file (if they had been present in it). Note - that if multiple contiguous lines have been deleted, the returned list will - contain contiguous line number entries. To prevent false positives, we - return deleted line numbers *only* from diff chunks which decrease the size - of the new file. - - Note: We need this method because we have access to neither the old file - content nor the list of "delete" changes from the current presubmit script - API. - - """ - results = [] - line_num = 0 - deleting_lines = 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 - # Return line numbers only from diff chunks decreasing the size of the - # new file - deleting_lines = old_size > new_size - continue - if not line.startswith('-'): - line_num += 1 - if deleting_lines and line.startswith('-') and not line.startswith('--'): - results.append(line_num) - return results - - def CheckForEnumEntryDeletions(self, affected_file): - """Look for deletions inside the enum definition. We currently use a - simple heuristics (not 100% accurate): if there are deleted lines inside - the enum definition, this might be a deletion. - - """ - range_new = self.ComputeEnumRangeInNewFile(affected_file) - if not range_new: - return False - - is_ok = True - for line_num in self.GetDeletedLinesFromScmDiff(affected_file): - if range_new.Contains(line_num): - self.EmitWarning("It looks like you are deleting line(s) from the " - "enum definition. This should never happen.", - line_num) - is_ok = False - return is_ok - - def CheckForEnumEntryInsertions(self, affected_file): - range = self.ComputeEnumRangeInNewFile(affected_file) - if not range: - return False - - first_line = range.first_line - last_line = range.last_line - - # Collect the range of changes inside the enum definition range. - is_ok = True - for line_start, line_end, line_text in \ - self.CollectRangesInsideEnumDefinition(affected_file, - first_line, - last_line): - # The only edit we consider valid is adding 1 or more entries *exactly* - # at the end of the enum definition. Every other edit inside the enum - # definition will result in a "warning confirmation" message. - # - # TODO(rpaquay): We currently cannot detect "renames" of existing entries - # vs invalid insertions, so we sometimes will warn for valid edits. - is_valid_edit = (line_end == last_line - 1) - - self.LogDebug("Edit range in new file at starting at line number %d and " - "ending at line number %d: valid=%s" - % (line_start, line_end, is_valid_edit)) - - if not is_valid_edit: - self.EmitWarning("The change starting at line %d and ending at line " - "%d is *not* located *exactly* at the end of the " - "enum definition. Unless you are renaming an " - "existing entry, this is not a valid changes, as new " - "entries should *always* be added at the end of the " - "enum definition, right before the 'ENUM_BOUNDARY' " - "entry." % (line_start, line_end), - line_start, - line_text) - is_ok = False - return is_ok - - def PerformChecks(self, affected_file): - if not self.CheckForFileDeletion(affected_file): - return - if not self.CheckForEnumEntryDeletions(affected_file): - return - if not self.CheckForEnumEntryInsertions(affected_file): - return +def _CreateHistogramValueChecker(input_api, output_api): + original_sys_path = sys.path - def ProcessHistogramValueFile(self, affected_file): - self.LogInfo("Start processing file \"%s\"" % affected_file.LocalPath()) - self.PerformChecks(affected_file) - self.LogInfo("Done processing file \"%s\"" % affected_file.LocalPath()) + try: + sys.path.append(input_api.os_path.join( + input_api.PresubmitLocalPath(), '..', '..', 'tools', + 'strict_enum_value_checker')) + from strict_enum_value_checker import StrictEnumValueChecker + finally: + sys.path = original_sys_path - def Run(self): - for file in self.input_api.AffectedFiles(include_deletes=True): - if file.LocalPath() == self.LOCAL_PATH: - self.ProcessHistogramValueFile(file) - return self.results + return StrictEnumValueChecker(input_api, output_api, + start_marker='enum HistogramValue {', end_marker=' // Last entry:', + path='extensions/browser/extension_function_histogram_value.h') def CheckChangeOnUpload(input_api, output_api): results = [] - results += HistogramValueChecker(input_api, output_api).Run() + results += _CreateHistogramValueChecker(input_api, output_api).Run() results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) return results +def CheckChangeOnCommit(input_api, output_api): + return _CreateHistogramValueChecker(input_api, output_api).Run() diff --git a/extensions/browser/PRESUBMIT_test_new_file_1.txt b/extensions/browser/PRESUBMIT_test_new_file_1.txt deleted file mode 100644 index cf894a7..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_1.txt +++ /dev/null @@ -1,35 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - VALID_INSERTION, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_10.txt b/extensions/browser/PRESUBMIT_test_new_file_10.txt deleted file mode 100644 index 745b130..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_10.txt +++ /dev/null @@ -1,29 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_11.txt b/extensions/browser/PRESUBMIT_test_new_file_11.txt deleted file mode 100644 index d02dd03..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_11.txt +++ /dev/null @@ -1,32 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_2.txt b/extensions/browser/PRESUBMIT_test_new_file_2.txt deleted file mode 100644 index e242545..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_2.txt +++ /dev/null @@ -1,33 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_3.txt b/extensions/browser/PRESUBMIT_test_new_file_3.txt deleted file mode 100644 index 3e18f29b..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_3.txt +++ /dev/null @@ -1,34 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES2, // Rename here!!! - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_4.txt b/extensions/browser/PRESUBMIT_test_new_file_4.txt deleted file mode 100644 index 8bab558..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_4.txt +++ /dev/null @@ -1,5 +0,0 @@ -// Missing "HistrogramValue" enum - - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY diff --git a/extensions/browser/PRESUBMIT_test_new_file_5.txt b/extensions/browser/PRESUBMIT_test_new_file_5.txt deleted file mode 100644 index 5c75baa..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_5.txt +++ /dev/null @@ -1,5 +0,0 @@ - -enum HistogramValue { - - -// Missing end enum marker diff --git a/extensions/browser/PRESUBMIT_test_new_file_6.txt b/extensions/browser/PRESUBMIT_test_new_file_6.txt deleted file mode 100644 index d5d847b..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_6.txt +++ /dev/null @@ -1,9 +0,0 @@ -// End enum marker is before start enum marker - - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -// -enum HistogramValue { - UNKNOWN = 0, - diff --git a/extensions/browser/PRESUBMIT_test_new_file_7.txt b/extensions/browser/PRESUBMIT_test_new_file_7.txt deleted file mode 100644 index f928b71..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_7.txt +++ /dev/null @@ -1,40 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - BROWSINGDATA_REMOVEWEBSQL2, - BROWSINGDATA_REMOVEWEBSQL3, - BROWSINGDATA_REMOVEWEBSQL4, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - DOWNLOADSINTERNAL_DETERMINEFILENAME2, - DOWNLOADSINTERNAL_DETERMINEFILENAME3, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - MEDIAGALLERIESPRIVATE_GETHANDLERS2, - WALLPAPERPRIVATE_RESETWALLPAPER, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_8.txt b/extensions/browser/PRESUBMIT_test_new_file_8.txt deleted file mode 100644 index 8142053..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_8.txt +++ /dev/null @@ -1,35 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - BROWSINGDATA_REMOVEWEBSQL2, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_new_file_9.txt b/extensions/browser/PRESUBMIT_test_new_file_9.txt deleted file mode 100644 index 75d8cce..0000000 --- a/extensions/browser/PRESUBMIT_test_new_file_9.txt +++ /dev/null @@ -1,38 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - BLAH1, - BLAH2, - BLAH3, - BLAH4, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/browser/PRESUBMIT_test_old_file.txt b/extensions/browser/PRESUBMIT_test_old_file.txt deleted file mode 100644 index 51cd734..0000000 --- a/extensions/browser/PRESUBMIT_test_old_file.txt +++ /dev/null @@ -1,34 +0,0 @@ -// 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. - -#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ - - -namespace extensions { -namespace functions { - -// -// This is some comment. -// This is another comment. -// This is yet another comment. -// -enum HistogramValue { - UNKNOWN = 0, - WEBNAVIGATION_GETALLFRAMES, - BROWSINGDATA_REMOVEWEBSQL, - WALLPAPERPRIVATE_SETCUSTOMWALLPAPERLAYOUT, - DOWNLOADSINTERNAL_DETERMINEFILENAME, - SYNCFILESYSTEM_GETFILESYNCSTATUSES, - MEDIAGALLERIESPRIVATE_GETHANDLERS, - WALLPAPERPRIVATE_RESETWALLPAPER, - // Last entry: Add new entries above and ensure to update - // tools/metrics/histograms/histograms/histograms.xml. - ENUM_BOUNDARY -}; - -} // namespace functions -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_FUNCTION_HISTOGRAM_VALUE_H_ diff --git a/extensions/common/permissions/PRESUBMIT.py b/extensions/common/permissions/PRESUBMIT.py new file mode 100644 index 0000000..1e059bd --- /dev/null +++ b/extensions/common/permissions/PRESUBMIT.py @@ -0,0 +1,32 @@ +# 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. + +"""Chromium presubmit script for src/extensions/common/permissions. + +See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts +for more details on the presubmit API built into gcl. +""" +import sys + +def GetPreferredTrySlaves(): + return ['linux_chromeos'] + +def _CreatePermissionMessageEnumChecker(input_api, output_api): + original_sys_path = sys.path + + try: + sys.path.append(input_api.os_path.join( + input_api.PresubmitLocalPath(), '..', '..', '..', 'tools', + 'strict_enum_value_checker')) + from strict_enum_value_checker import StrictEnumValueChecker + finally: + sys.path = original_sys_path + + return StrictEnumValueChecker(input_api, output_api, + start_marker='enum ID {', end_marker=' kEnumBoundary', + path='extensions/common/permissions/permission_message.h') + +def CheckChangeOnUpload(input_api, output_api): + return _CreatePermissionMessageEnumChecker(input_api, output_api).Run() + diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 7257711..c1c5b5f 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -32302,6 +32302,71 @@ other types of suffix sets. <int value="28" label="SOCKET_SPECIFIC_HOSTS"/> </enum> +<enum name="ExtensionPermission2" type="int"> +<!-- Generated from ../../../extensions/common/permissions/permission_message.h --> + + <int value="0" label="kUnknown"/> + <int value="1" label="kNone"/> + <int value="2" label="kBookmarks"/> + <int value="3" label="kGeolocation"/> + <int value="4" label="kBrowsingHistory"/> + <int value="5" label="kTabs"/> + <int value="6" label="kManagement"/> + <int value="7" label="kDebugger"/> + <int value="8" label="kDesktopCapture"/> + <int value="9" label="kHid"/> + <int value="10" label="kHosts1"/> + <int value="11" label="kHosts2"/> + <int value="12" label="kHosts3"/> + <int value="13" label="kHosts4OrMore"/> + <int value="14" label="kHostsAll"/> + <int value="15" label="kFullAccess"/> + <int value="16" label="kClipboard"/> + <int value="17" label="kTtsEngine"/> + <int value="18" label="kContentSettings"/> + <int value="19" label="kPrivacy"/> + <int value="20" label="kManagedMode"/> + <int value="21" label="kInput"/> + <int value="22" label="kAudioCapture"/> + <int value="23" label="kVideoCapture"/> + <int value="24" label="kDownloads"/> + <int value="25" label="kFileSystemWrite"/> + <int value="26" label="kMediaGalleriesAllGalleriesRead"/> + <int value="27" label="kSerial"/> + <int value="28" label="kSocketAnyHost"/> + <int value="29" label="kSocketDomainHosts"/> + <int value="30" label="kSocketSpecificHosts"/> + <int value="31" label="kBluetooth"/> + <int value="32" label="kUsb"/> + <int value="33" label="kSystemIndicator"/> + <int value="34" label="kUsbDevice"/> + <int value="35" label="kMediaGalleriesAllGalleriesCopyTo"/> + <int value="36" label="kSystemInfoDisplay"/> + <int value="37" label="kNativeMessaging"/> + <int value="38" label="kSyncFileSystem"/> + <int value="39" label="kAudio"/> + <int value="40" label="kFavicon"/> + <int value="41" label="kMusicManagerPrivate"/> + <int value="42" label="kWebConnectable"/> + <int value="43" label="kActivityLogPrivate"/> + <int value="44" label="kBluetoothDevices"/> + <int value="45" label="kDownloadsOpen"/> + <int value="46" label="kNetworkingPrivate"/> + <int value="47" label="kDeclarativeWebRequest"/> + <int value="48" label="kFileSystemDirectory"/> + <int value="49" label="kFileSystemWriteDirectory"/> + <int value="50" label="kSignedInDevices"/> + <int value="51" label="kWallpaper"/> + <int value="52" label="kNetworkState"/> + <int value="53" label="kHomepage"/> + <int value="54" label="kSearchProvider"/> + <int value="55" label="kStartupPages"/> + <int value="56" label="kMediaGalleriesAllGalleriesDelete"/> + <int value="57" label="kScreenlockPrivate"/> + <int value="58" label="kOverrideBookmarksUI"/> + <int value="59" label="kAutomation"/> +</enum> + <enum name="ExtensionServiceVerifyAllSuccess" type="int"> <int value="0" label="VERIFY_ALL_BOOTSTRAP_SUCCESS"/> <int value="1" label="VERIFY_ALL_BOOTSTRAP_FAILURE"/> diff --git a/tools/metrics/histograms/update_extension_functions.py b/tools/metrics/histograms/update_extension_functions.py index 0783f32..dd390a2 100644 --- a/tools/metrics/histograms/update_extension_functions.py +++ b/tools/metrics/histograms/update_extension_functions.py @@ -8,145 +8,15 @@ extension_function_histogram_value.h. If the file was pretty-printed, the updated version is pretty-printed too. """ -import logging import os -import re -import sys - -from xml.dom import minidom -import print_style - -# Import the metrics/common module. -sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'common')) -from diff_util import PromptUserToAcceptDiff - -HISTOGRAMS_PATH = 'histograms.xml' -ENUM_NAME = 'ExtensionFunctions' - -EXTENSION_FUNCTIONS_HISTOGRAM_VALUE_PATH = \ - '../../../extensions/browser/extension_function_histogram_value.h' -ENUM_START_MARKER = "^enum HistogramValue {" -ENUM_END_MARKER = "^ENUM_BOUNDARY" - - -class UserError(Exception): - def __init__(self, message): - Exception.__init__(self, message) - - @property - def message(self): - return self.args[0] - -def ExtractRegexGroup(line, regex): - m = re.match(regex, line) - if m: - return m.group(1) - else: - return None - - -def ReadHistogramValues(filename): - """Returns a list of pairs (label, value) corresponding to HistogramValue. - - Reads the extension_function_histogram_value.h file, locates the - HistogramValue enum definition and returns a pair for each entry. - """ - - # Read the file as a list of lines - with open(filename) as f: - content = f.readlines() - - # Locate the enum definition and collect all entries in it - inside_enum = False # We haven't found the enum definition yet - result = [] - for line in content: - line = line.strip() - if inside_enum: - # Exit condition: we reached last enum value - if re.match(ENUM_END_MARKER, line): - inside_enum = False - else: - # Inside enum: generate new xml entry - label = ExtractRegexGroup(line.strip(), "^([\w]+)") - if label: - result.append((label, enum_value)) - enum_value += 1 - else: - if re.match(ENUM_START_MARKER, line): - inside_enum = True - enum_value = 0 # Start at 'UNKNOWN' - return result - - -def UpdateHistogramDefinitions(histogram_values, document): - """Sets the children of <enum name="ExtensionFunctions" ...> node in - |document| to values generated from policy ids contained in - |policy_templates|. - - Args: - histogram_values: A list of pairs (label, value) defining each extension - function - document: A minidom.Document object representing parsed histogram - definitions XML file. - - """ - # Find ExtensionFunctions enum. - for enum_node in document.getElementsByTagName('enum'): - if enum_node.attributes['name'].value == ENUM_NAME: - extension_functions_enum_node = enum_node - break - else: - raise UserError('No policy enum node found') - - # Remove existing values. - while extension_functions_enum_node.hasChildNodes(): - extension_functions_enum_node.removeChild( - extension_functions_enum_node.lastChild) - - # Add a "Generated from (...)" comment - comment = ' Generated from {0} '.format( - EXTENSION_FUNCTIONS_HISTOGRAM_VALUE_PATH) - extension_functions_enum_node.appendChild(document.createComment(comment)) - - # Add values generated from policy templates. - for (label, value) in histogram_values: - node = document.createElement('int') - node.attributes['value'] = str(value) - node.attributes['label'] = label - extension_functions_enum_node.appendChild(node) - -def Log(message): - logging.info(message) - -def main(): - if len(sys.argv) > 1: - print >>sys.stderr, 'No arguments expected!' - sys.stderr.write(__doc__) - sys.exit(1) - - Log('Reading histogram enum definition from "%s".' - % (EXTENSION_FUNCTIONS_HISTOGRAM_VALUE_PATH)) - histogram_values = ReadHistogramValues( - EXTENSION_FUNCTIONS_HISTOGRAM_VALUE_PATH) - - Log('Reading existing histograms from "%s".' % (HISTOGRAMS_PATH)) - with open(HISTOGRAMS_PATH, 'rb') as f: - histograms_doc = minidom.parse(f) - f.seek(0) - xml = f.read() - - Log('Comparing histograms enum with new enum definition.') - UpdateHistogramDefinitions(histogram_values, histograms_doc) - - Log('Writing out new histograms file.') - new_xml = print_style.GetPrintStyle().PrettyPrintNode(histograms_doc) - - if PromptUserToAcceptDiff(xml, new_xml, 'Is the updated version acceptable?'): - with open(HISTOGRAMS_PATH, 'wb') as f: - f.write(new_xml) - - Log('Done.') +from update_histogram_enum import UpdateHistogramEnum if __name__ == '__main__': - main() + UpdateHistogramEnum(histogram_enum_name='ExtensionFunctions', + source_enum_path= + os.path.join('..', '..', '..', 'extensions', + 'browser', + 'extension_function_histogram_value.h'), + start_marker='^enum HistogramValue {', + end_marker='^ENUM_BOUNDARY') diff --git a/tools/metrics/histograms/update_extension_permission.py b/tools/metrics/histograms/update_extension_permission.py new file mode 100644 index 0000000..1672841 --- /dev/null +++ b/tools/metrics/histograms/update_extension_permission.py @@ -0,0 +1,22 @@ +# 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. + +"""Updates ExtensionPermission2 enum in histograms.xml file with values read +from permission_message.h. + +If the file was pretty-printed, the updated version is pretty-printed too. +""" + +import os + +from update_histogram_enum import UpdateHistogramEnum + +if __name__ == '__main__': + UpdateHistogramEnum(histogram_enum_name='ExtensionPermission2', + source_enum_path=os.path.join('..', '..', '..', + 'extensions', 'common', + 'permissions', + 'permission_message.h'), + start_marker='^enum ID {', + end_marker='^kEnumBoundary') diff --git a/tools/metrics/histograms/update_histogram_enum.py b/tools/metrics/histograms/update_histogram_enum.py new file mode 100644 index 0000000..ea43a35 --- /dev/null +++ b/tools/metrics/histograms/update_histogram_enum.py @@ -0,0 +1,136 @@ +# 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. + +"""Updates enums in histograms.xml file with values read from provided C++ enum. + +If the file was pretty-printed, the updated version is pretty-printed too. +""" + +import logging +import print_style +import re +import sys + +from xml.dom import minidom +from diffutil import PromptUserToAcceptDiff + +class UserError(Exception): + def __init__(self, message): + Exception.__init__(self, message) + + @property + def message(self): + return self.args[0] + + +def Log(message): + logging.info(message) + + +def ExtractRegexGroup(line, regex): + m = re.match(regex, line) + if m: + # TODO(ahernandez.miralles): This can throw an IndexError + # if no groups are present; enclose in try catch block + return m.group(1) + else: + return None + + +def ReadHistogramValues(filename, start_marker, end_marker): + """Reads in values from |filename|, returning a list of (label, value) pairs + corresponding to the enum framed by |start_marker| and |end_marker|. + """ + # Read the file as a list of lines + with open(filename) as f: + content = f.readlines() + + # Locate the enum definition and collect all entries in it + inside_enum = False # We haven't found the enum definition yet + result = [] + for line in content: + line = line.strip() + if inside_enum: + # Exit condition: we reached last enum value + if re.match(end_marker, line): + inside_enum = False + else: + # Inside enum: generate new xml entry + label = ExtractRegexGroup(line.strip(), "^([\w]+)") + if label: + result.append((label, enum_value)) + enum_value += 1 + else: + if re.match(start_marker, line): + inside_enum = True + enum_value = 0 # Start at 'UNKNOWN' + return result + + +def UpdateHistogramDefinitions(histogram_enum_name, source_enum_values, + source_enum_path, document): + """Sets the children of <enum name=|histogram_enum_name| ...> node in + |document| to values generated from (label, value) pairs contained in + |source_enum_values|. + """ + # Find ExtensionFunctions enum. + for enum_node in document.getElementsByTagName('enum'): + if enum_node.attributes['name'].value == histogram_enum_name: + histogram_enum_node = enum_node + break + else: + raise UserError('No {0} enum node found'.format(histogram_enum_name)) + + # Remove existing values. + while histogram_enum_node.hasChildNodes(): + histogram_enum_node.removeChild(histogram_enum_node.lastChild) + + # Add a "Generated from (...)" comment + comment = ' Generated from {0} '.format(source_enum_path) + histogram_enum_node.appendChild(document.createComment(comment)) + + # Add values generated from policy templates. + for (label, value) in source_enum_values: + node = document.createElement('int') + node.attributes['value'] = str(value) + node.attributes['label'] = label + histogram_enum_node.appendChild(node) + + +def UpdateHistogramEnum(histogram_enum_name, source_enum_path, + start_marker, end_marker): + """Updates |histogram_enum_name| enum in histograms.xml file with values + read from |source_enum_path|, where |start_marker| and |end_marker| indicate + the beginning and end of the source enum definition, respectively. + """ + # TODO(ahernandez.miralles): The line below is present in nearly every + # file in this directory; factor out into a central location + HISTOGRAMS_PATH = 'histograms.xml' + + if len(sys.argv) > 1: + print >>sys.stderr, 'No arguments expected!' + sys.stderr.write(__doc__) + sys.exit(1) + + Log('Reading histogram enum definition from "{0}".'.format(source_enum_path)) + source_enum_values = ReadHistogramValues(source_enum_path, start_marker, + end_marker) + + Log('Reading existing histograms from "{0}".'.format(HISTOGRAMS_PATH)) + with open(HISTOGRAMS_PATH, 'rb') as f: + histograms_doc = minidom.parse(f) + f.seek(0) + xml = f.read() + + Log('Comparing histograms enum with new enum definition.') + UpdateHistogramDefinitions(histogram_enum_name, source_enum_values, + source_enum_path, histograms_doc) + + Log('Writing out new histograms file.') + new_xml = print_style.GetPrintStyle().PrettyPrintNode(histograms_doc) + if PromptUserToAcceptDiff(xml, new_xml, 'Is the updated version acceptable?'): + with open(HISTOGRAMS_PATH, 'wb') as f: + f.write(new_xml) + + Log('Done.') diff --git a/tools/strict_enum_value_checker/changed_file_1.h b/tools/strict_enum_value_checker/changed_file_1.h new file mode 100644 index 0000000..e74e408 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_1.h @@ -0,0 +1,30 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + mValidInsertion1, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_10.h b/tools/strict_enum_value_checker/changed_file_10.h new file mode 100644 index 0000000..d7b370d --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_10.h @@ -0,0 +1,23 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_2.h b/tools/strict_enum_value_checker/changed_file_2.h new file mode 100644 index 0000000..04a801b --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_2.h @@ -0,0 +1,28 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mFunction1, + mInfo2, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_3.h b/tools/strict_enum_value_checker/changed_file_3.h new file mode 100644 index 0000000..f839c14 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_3.h @@ -0,0 +1,29 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mErrata1, + mFunction1, + mInfo2, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_4.h b/tools/strict_enum_value_checker/changed_file_4.h new file mode 100644 index 0000000..157153e --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_4.h @@ -0,0 +1,26 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_5.h b/tools/strict_enum_value_checker/changed_file_5.h new file mode 100644 index 0000000..b788289 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_5.h @@ -0,0 +1,27 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_6.h b/tools/strict_enum_value_checker/changed_file_6.h new file mode 100644 index 0000000..7bbbf83 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_6.h @@ -0,0 +1,29 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// + mBoundary // Do not add below here +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_7.h b/tools/strict_enum_value_checker/changed_file_7.h new file mode 100644 index 0000000..61a2792 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_7.h @@ -0,0 +1,33 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mInvalid1, + mError1, + mFunction1, + mInvalid2, + mInvalid3, + mInfo2, + mInvalid4, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_8.h b/tools/strict_enum_value_checker/changed_file_8.h new file mode 100644 index 0000000..56e9483 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_8.h @@ -0,0 +1,30 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mInsertion, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/changed_file_9.h b/tools/strict_enum_value_checker/changed_file_9.h new file mode 100644 index 0000000..bea98f0 --- /dev/null +++ b/tools/strict_enum_value_checker/changed_file_9.h @@ -0,0 +1,32 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + mValidInsertion1, + mValidInsertion2, + mValidInsertion3, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/mock_enum.h b/tools/strict_enum_value_checker/mock_enum.h new file mode 100644 index 0000000..86e80b7 --- /dev/null +++ b/tools/strict_enum_value_checker/mock_enum.h @@ -0,0 +1,29 @@ +// 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. + +#ifndef MOCK_ENUM_H +#define MOCK_ENUM_H + +// Here is our mock enum. Beyond testing it is completely meaningless. +// MockEnum follows strict rules for valid modifications: +// 1. NO reordering of entries +// 2. NO deletions of entries +// 3. New entries must be added just before mBoundary, never after +// +enum MockEnum { + mEntry1, + mEntry2, + mData1, + mData2, + mEntry3, + mInfo1, + mData3, + mError1, + mFunction1, + mInfo2, + mData4, + mBoundary // Do not add below here +}; + +#endif diff --git a/tools/strict_enum_value_checker/strict_enum_value_checker.py b/tools/strict_enum_value_checker/strict_enum_value_checker.py new file mode 100644 index 0000000..65c4a9e --- /dev/null +++ b/tools/strict_enum_value_checker/strict_enum_value_checker.py @@ -0,0 +1,284 @@ +# 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. + +class StrictEnumValueChecker(object): + """Verify that changes to enums are valid. + + This class is used to check enums where reordering or deletion is not allowed, + and additions must be at the end of the enum, just prior to some "boundary" + entry. See comments at the top of the "extension_function_histogram_value.h" + file in chrome/browser/extensions for an example what are considered valid + changes. There are situations where this class gives false positive warnings, + i.e. it warns even though the edit is legitimate. Since the class warns using + prompt warnings, the user can always choose to continue. The main point is to + attract the attention to all (potentially or not) invalid edits. + + """ + def __init__(self, input_api, output_api, start_marker, end_marker, path): + self.input_api = input_api + self.output_api = output_api + self.start_marker = start_marker + self.end_marker = end_marker + self.path = path + self.results = [] + + class EnumRange(object): + """Represents a range of line numbers (1-based)""" + def __init__(self, first_line, last_line): + self.first_line = first_line + self.last_line = last_line + + def Count(self): + return self.last_line - self.first_line + 1 + + def Contains(self, line_num): + return self.first_line <= line_num and line_num <= self.last_line + + def LogInfo(self, message): + self.input_api.logging.info(message) + return + + def LogDebug(self, message): + self.input_api.logging.debug(message) + return + + def ComputeEnumRangeInContents(self, contents): + """Returns an |EnumRange| object representing the line extent of the + enum members in |contents|. The line numbers are 1-based, + compatible with line numbers returned by AffectedFile.ChangeContents(). + |contents| is a list of strings reprenting the lines of a text file. + + If either start_marker or end_marker cannot be found in + |contents|, returns None and emits detailed warnings about the problem. + + """ + first_enum_line = 0 + last_enum_line = 0 + line_num = 1 # Line numbers are 1-based + for line in contents: + if line.startswith(self.start_marker): + first_enum_line = line_num + 1 + elif line.startswith(self.end_marker): + last_enum_line = line_num + line_num += 1 + + if first_enum_line == 0: + self.EmitWarning("The presubmit script could not find the start of the " + "enum definition (\"%s\"). Did the enum definition " + "change?" % self.start_marker) + return None + + if last_enum_line == 0: + self.EmitWarning("The presubmit script could not find the end of the " + "enum definition (\"%s\"). Did the enum definition " + "change?" % self.end_marker) + return None + + if first_enum_line >= last_enum_line: + self.EmitWarning("The presubmit script located the start of the enum " + "definition (\"%s\" at line %d) *after* its end " + "(\"%s\" at line %d). Something is not quite right." + % (self.start_marker, first_enum_line, + self.end_marker, last_enum_line)) + return None + + self.LogInfo("Line extent of (\"%s\") enum definition: " + "first_line=%d, last_line=%d." + % (self.start_marker, first_enum_line, last_enum_line)) + return self.EnumRange(first_enum_line, last_enum_line) + + def ComputeEnumRangeInNewFile(self, affected_file): + return self.ComputeEnumRangeInContents(affected_file.NewContents()) + + def GetLongMessage(self, local_path): + return str("The file \"%s\" contains the definition of the " + "(\"%s\") enum which should be edited in specific ways " + "only - *** read the comments at the top of the header file ***" + ". There are changes to the file that may be incorrect and " + "warrant manual confirmation after review. Note that this " + "presubmit script can not reliably report the nature of all " + "types of invalid changes, especially when the diffs are " + "complex. For example, an invalid deletion may be reported " + "whereas the change contains a valid rename." + % local_path, self.start_marker) + + 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 + long_message = self.GetLongMessage(self.path) + self.LogInfo(message) + self.results.append( + self.output_api.PresubmitPromptWarning(message, [item], long_message)) + + def CollectRangesInsideEnumDefinition(self, affected_file, + first_line, last_line): + """Returns a list of triplet (line_start, line_end, line_text) of ranges of + edits changes. The |line_text| part is the text at line |line_start|. + Since it used only for reporting purposes, we do not need all the text + lines in the range. + + """ + results = [] + previous_line_number = 0 + previous_range_start_line_number = 0 + previous_range_start_text = "" + + def addRange(): + tuple = (previous_range_start_line_number, + previous_line_number, + previous_range_start_text) + results.append(tuple) + + for line_number, line_text in affected_file.ChangedContents(): + if first_line <= line_number and line_number <= last_line: + self.LogDebug("Line change at line number " + str(line_number) + ": " + + line_text) + # Start a new interval if none started + if previous_range_start_line_number == 0: + previous_range_start_line_number = line_number + previous_range_start_text = line_text + # Add new interval if we reached past the previous one + elif line_number != previous_line_number + 1: + addRange() + previous_range_start_line_number = line_number + previous_range_start_text = line_text + previous_line_number = line_number + + # Add a last interval if needed + if previous_range_start_line_number != 0: + addRange() + return results + + def CheckForFileDeletion(self, affected_file): + """Emits a warning notification if file has been deleted """ + if not affected_file.NewContents(): + self.EmitWarning("The file seems to be deleted in the changelist. If " + "your intent is to really delete the file, the code in " + "PRESUBMIT.py should be updated to remove the " + "|StrictEnumValueChecker| class."); + return False + return True + + def GetDeletedLinesFromScmDiff(self, affected_file): + """Return a list of of line numbers (1-based) corresponding to lines + deleted from the new source file (if they had been present in it). Note + that if multiple contiguous lines have been deleted, the returned list will + contain contiguous line number entries. To prevent false positives, we + return deleted line numbers *only* from diff chunks which decrease the size + of the new file. + + Note: We need this method because we have access to neither the old file + content nor the list of "delete" changes from the current presubmit script + API. + + """ + results = [] + line_num = 0 + deleting_lines = 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 + # Return line numbers only from diff chunks decreasing the size of the + # new file + deleting_lines = old_size > new_size + continue + if not line.startswith("-"): + line_num += 1 + if deleting_lines and line.startswith("-") and not line.startswith("--"): + results.append(line_num) + return results + + def CheckForEnumEntryDeletions(self, affected_file): + """Look for deletions inside the enum definition. We currently use a + simple heuristics (not 100% accurate): if there are deleted lines inside + the enum definition, this might be a deletion. + + """ + range_new = self.ComputeEnumRangeInNewFile(affected_file) + if not range_new: + return False + + is_ok = True + for line_num in self.GetDeletedLinesFromScmDiff(affected_file): + if range_new.Contains(line_num): + self.EmitWarning("It looks like you are deleting line(s) from the " + "enum definition. This should never happen.", + line_num) + is_ok = False + return is_ok + + def CheckForEnumEntryInsertions(self, affected_file): + range = self.ComputeEnumRangeInNewFile(affected_file) + if not range: + return False + + first_line = range.first_line + last_line = range.last_line + + # Collect the range of changes inside the enum definition range. + is_ok = True + for line_start, line_end, line_text in \ + self.CollectRangesInsideEnumDefinition(affected_file, + first_line, + last_line): + # The only edit we consider valid is adding 1 or more entries *exactly* + # at the end of the enum definition. Every other edit inside the enum + # definition will result in a "warning confirmation" message. + # + # TODO(rpaquay): We currently cannot detect "renames" of existing entries + # vs invalid insertions, so we sometimes will warn for valid edits. + is_valid_edit = (line_end == last_line - 1) + + self.LogDebug("Edit range in new file at starting at line number %d and " + "ending at line number %d: valid=%s" + % (line_start, line_end, is_valid_edit)) + + if not is_valid_edit: + self.EmitWarning("The change starting at line %d and ending at line " + "%d is *not* located *exactly* at the end of the " + "enum definition. Unless you are renaming an " + "existing entry, this is not a valid change, as new " + "entries should *always* be added at the end of the " + "enum definition, right before the \"%s\" " + "entry." % (line_start, line_end, self.end_marker), + line_start, + line_text) + is_ok = False + return is_ok + + def PerformChecks(self, affected_file): + if not self.CheckForFileDeletion(affected_file): + return + if not self.CheckForEnumEntryDeletions(affected_file): + return + if not self.CheckForEnumEntryInsertions(affected_file): + return + + def ProcessHistogramValueFile(self, affected_file): + self.LogInfo("Start processing file \"%s\"" % affected_file.LocalPath()) + self.PerformChecks(affected_file) + self.LogInfo("Done processing file \"%s\"" % affected_file.LocalPath()) + + def Run(self): + for file in self.input_api.AffectedFiles(include_deletes=True): + if file.LocalPath() == self.path: + self.ProcessHistogramValueFile(file) + return self.results diff --git a/extensions/browser/PRESUBMIT_test.py b/tools/strict_enum_value_checker/strict_enum_value_checker_test.py index 99cabf5..4f95efe 100755 --- a/extensions/browser/PRESUBMIT_test.py +++ b/tools/strict_enum_value_checker/strict_enum_value_checker_test.py @@ -8,7 +8,7 @@ import os import re import unittest -import PRESUBMIT +from strict_enum_value_checker import StrictEnumValueChecker class MockLogging(object): def __init__(self): @@ -34,25 +34,25 @@ class MockInputApi(object): class MockOutputApi(object): class PresubmitResult(object): - def __init__(self, message, items=None, long_text=''): + def __init__(self, message, items=None, long_text=""): self.message = message self.items = items self.long_text = long_text class PresubmitError(PresubmitResult): - def __init__(self, message, items, long_text=''): + def __init__(self, message, items, long_text=""): MockOutputApi.PresubmitResult.__init__(self, message, items, long_text) - self.type = 'error' + self.type = "error" class PresubmitPromptWarning(PresubmitResult): - def __init__(self, message, items, long_text=''): + def __init__(self, message, items, long_text=""): MockOutputApi.PresubmitResult.__init__(self, message, items, long_text) - self.type = 'warning' + self.type = "warning" class PresubmitNotifyResult(PresubmitResult): - def __init__(self, message, items, long_text=''): + def __init__(self, message, items, long_text=""): MockOutputApi.PresubmitResult.__init__(self, message, items, long_text) - self.type = 'notify' + self.type = "notify" class MockFile(object): @@ -100,13 +100,13 @@ class MockFile(object): return [] for line in self.GenerateScmDiff().splitlines(): - m = re.match(r'^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@', line) + m = re.match(r"^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@", line) if m: line_num = int(m.groups(1)[0]) continue - if line.startswith('+') and not line.startswith('++'): + if line.startswith("+") and not line.startswith("++"): self._cached_changed_contents.append((line_num, line[1:])) - if not line.startswith('-'): + if not line.startswith("-"): line_num += 1 return self._cached_changed_contents[:] @@ -119,15 +119,18 @@ class MockChange(object): return self._changed_files -class HistogramValueCheckerTest(unittest.TestCase): - TEST_FILE_PATTERN = "PRESUBMIT_test_new_file_%s.txt" +class StrictEnumValueCheckerTest(unittest.TestCase): + TEST_FILE_PATTERN = "changed_file_%s.h" + MOCK_FILE_LOCAL_PATH = "mock_enum.h" + START_MARKER = "enum MockEnum {" + END_MARKER = " mBoundary" def _ReadTextFileContents(self, path): """Given a path, returns a list of strings corresponding to the text lines in the file. Reads files in text format. """ - fo = open(path, 'r') + fo = open(path, "r") try: contents = fo.readlines() finally: @@ -135,7 +138,7 @@ class HistogramValueCheckerTest(unittest.TestCase): return contents def _ReadInputFile(self): - return self._ReadTextFileContents("PRESUBMIT_test_old_file.txt") + return self._ReadTextFileContents("mock_enum.h") def _PrepareTest(self, new_file_path): old_contents = self._ReadInputFile() @@ -144,7 +147,7 @@ class HistogramValueCheckerTest(unittest.TestCase): else: new_contents = self._ReadTextFileContents(new_file_path) input_api = MockInputApi() - mock_file = MockFile(PRESUBMIT.HistogramValueChecker.LOCAL_PATH, + mock_file = MockFile(self.MOCK_FILE_LOCAL_PATH, old_contents, new_contents) input_api.files.append(mock_file) @@ -153,7 +156,8 @@ class HistogramValueCheckerTest(unittest.TestCase): def _RunTest(self, new_file_path): input_api, output_api = self._PrepareTest(new_file_path) - checker = PRESUBMIT.HistogramValueChecker(input_api, output_api) + checker = StrictEnumValueChecker(input_api, output_api, self.START_MARKER, + self.END_MARKER, self.MOCK_FILE_LOCAL_PATH) results = checker.Run() return results @@ -161,7 +165,7 @@ class HistogramValueCheckerTest(unittest.TestCase): results = self._RunTest(new_file_path=None) # TODO(rpaquay) How to check it's the expected warning?' self.assertEquals(1, len(results), - "We hould get a single warning about file deletion.") + "We should get a single warning about file deletion.") def testSimpleValidEdit(self): results = self._RunTest(self.TEST_FILE_PATTERN % "1") @@ -226,11 +230,6 @@ class HistogramValueCheckerTest(unittest.TestCase): "We should not get a warning for a deletion outside of " "the enum") - def testCommentIsNotEnumEndMarker(self): - results = self._RunTest(self.TEST_FILE_PATTERN % "11") - self.assertEquals(1, len(results), - "We should get a warning if '// Last Entry' is not the " - "enum end marker") if __name__ == '__main__': unittest.main() |