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

Side by Side Diff: extensions/browser/PRESUBMIT.py

Issue 170233008: Add presubmit check and automatic update script for ExtensionPermission enum. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Created 6 years, 9 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 unified diff | Download patch
« no previous file with comments | « chrome/browser/extensions/PRESUBMIT.py ('k') | extensions/browser/PRESUBMIT_test.py » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 # Copyright 2014 The Chromium Authors. All rights reserved. 1 # Copyright 2014 The Chromium Authors. All rights reserved.
2 # Use of this source code is governed by a BSD-style license that can be 2 # Use of this source code is governed by a BSD-style license that can be
3 # found in the LICENSE file. 3 # found in the LICENSE file.
4 4
5 """Chromium presubmit script for src/extensions/browser. 5 """Chromium presubmit script for src/extensions/browser.
6 6
7 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts 7 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
8 for more details on the presubmit API built into gcl. 8 for more details on the presubmit API built into gcl.
9 """ 9 """
10 10
11 import sys
12
11 def GetPreferredTryMasters(project, change): 13 def GetPreferredTryMasters(project, change):
12 return { 14 return {
13 'tryserver.chromium': { 15 'tryserver.chromium': {
14 'linux_chromium_chromeos_rel': set(['defaulttests']), 16 'linux_chromium_chromeos_rel': set(['defaulttests']),
15 } 17 }
16 } 18 }
17 19
18 class HistogramValueChecker(object): 20 def _CreateHistogramValueChecker(input_api, output_api):
19 """Verify that changes to "extension_function_histogram_value.h" are valid. 21 original_sys_path = sys.path
20 22
21 See comments at the top of the "extension_function_histogram_value.h" file 23 try:
22 for what are considered valid changes. There are situations where this script 24 sys.path.append(input_api.os_path.join(
23 gives false positive warnings, i.e. it warns even though the edit is 25 input_api.PresubmitLocalPath(), '..', '..', 'tools',
24 legitimate. Since the script warns using prompt warnings, the user can always 26 'strict_enum_value_checker'))
25 choose to continue. The main point is to attract the attention to all 27 from strict_enum_value_checker import StrictEnumValueChecker
26 (potentially or not) invalid edits. 28 finally:
29 sys.path = original_sys_path
27 30
28 """ 31 return StrictEnumValueChecker(input_api, output_api,
29 32 start_marker='enum HistogramValue {', end_marker=' // Last entry:',
30 # The name of the file we want to check against 33 path='extensions/browser/extension_function_histogram_value.h')
31 LOCAL_PATH = "extensions/browser/extension_function_histogram_value.h"
32
33 # The markers we look for in the above source file as delimiters of the enum
34 # definition.
35 ENUM_START_MARKER = "enum HistogramValue {"
36 ENUM_END_MARKER = " // Last entry:"
37
38 def __init__(self, input_api, output_api):
39 self.input_api = input_api
40 self.output_api = output_api
41 self.results = []
42
43 class EnumRange(object):
44 """Represents a range of line numbers (1-based)"""
45 def __init__(self, first_line, last_line):
46 self.first_line = first_line
47 self.last_line = last_line
48
49 def Count(self):
50 return self.last_line - self.first_line + 1
51
52 def Contains(self, line_num):
53 return self.first_line <= line_num and line_num <= self.last_line
54
55 def LogInfo(self, message):
56 self.input_api.logging.info(message)
57 return
58
59 def LogDebug(self, message):
60 self.input_api.logging.debug(message)
61 return
62
63 def ComputeEnumRangeInContents(self, contents):
64 """Returns an |EnumRange| object representing the line extent of the
65 HistogramValue enum members in |contents|. The line numbers are 1-based,
66 compatible with line numbers returned by AffectedFile.ChangeContents().
67 |contents| is a list of strings reprenting the lines of a text file.
68
69 If either ENUM_START_MARKER or ENUM_END_MARKER cannot be found in
70 |contents|, returns None and emits detailed warnings about the problem.
71
72 """
73 first_enum_line = 0
74 last_enum_line = 0
75 line_num = 1 # Line numbers are 1-based
76 for line in contents:
77 if line.startswith(self.ENUM_START_MARKER):
78 first_enum_line = line_num + 1
79 elif line.startswith(self.ENUM_END_MARKER):
80 last_enum_line = line_num
81 line_num += 1
82
83 if first_enum_line == 0:
84 self.EmitWarning("The presubmit script could not find the start of the "
85 "enum definition (\"%s\"). Did the enum definition "
86 "change?" % self.ENUM_START_MARKER)
87 return None
88
89 if last_enum_line == 0:
90 self.EmitWarning("The presubmit script could not find the end of the "
91 "enum definition (\"%s\"). Did the enum definition "
92 "change?" % self.ENUM_END_MARKER)
93 return None
94
95 if first_enum_line >= last_enum_line:
96 self.EmitWarning("The presubmit script located the start of the enum "
97 "definition (\"%s\" at line %d) *after* its end "
98 "(\"%s\" at line %d). Something is not quite right."
99 % (self.ENUM_START_MARKER, first_enum_line,
100 self.ENUM_END_MARKER, last_enum_line))
101 return None
102
103 self.LogInfo("Line extent of |HistogramValue| enum definition: "
104 "first_line=%d, last_line=%d."
105 % (first_enum_line, last_enum_line))
106 return self.EnumRange(first_enum_line, last_enum_line)
107
108 def ComputeEnumRangeInNewFile(self, affected_file):
109 return self.ComputeEnumRangeInContents(affected_file.NewContents())
110
111 def GetLongMessage(self):
112 return str("The file \"%s\" contains the definition of the "
113 "|HistogramValue| enum which should be edited in specific ways "
114 "only - *** read the comments at the top of the header file ***"
115 ". There are changes to the file that may be incorrect and "
116 "warrant manual confirmation after review. Note that this "
117 "presubmit script can not reliably report the nature of all "
118 "types of invalid changes, especially when the diffs are "
119 "complex. For example, an invalid deletion may be reported "
120 "whereas the change contains a valid rename."
121 % self.LOCAL_PATH)
122
123 def EmitWarning(self, message, line_number=None, line_text=None):
124 """Emits a presubmit prompt warning containing the short message
125 |message|. |item| is |LOCAL_PATH| with optional |line_number| and
126 |line_text|.
127
128 """
129 if line_number is not None and line_text is not None:
130 item = "%s(%d): %s" % (self.LOCAL_PATH, line_number, line_text)
131 elif line_number is not None:
132 item = "%s(%d)" % (self.LOCAL_PATH, line_number)
133 else:
134 item = self.LOCAL_PATH
135 long_message = self.GetLongMessage()
136 self.LogInfo(message)
137 self.results.append(
138 self.output_api.PresubmitPromptWarning(message, [item], long_message))
139
140 def CollectRangesInsideEnumDefinition(self, affected_file,
141 first_line, last_line):
142 """Returns a list of triplet (line_start, line_end, line_text) of ranges of
143 edits changes. The |line_text| part is the text at line |line_start|.
144 Since it used only for reporting purposes, we do not need all the text
145 lines in the range.
146
147 """
148 results = []
149 previous_line_number = 0
150 previous_range_start_line_number = 0
151 previous_range_start_text = ""
152
153 def addRange():
154 tuple = (previous_range_start_line_number,
155 previous_line_number,
156 previous_range_start_text)
157 results.append(tuple)
158
159 for line_number, line_text in affected_file.ChangedContents():
160 if first_line <= line_number and line_number <= last_line:
161 self.LogDebug("Line change at line number " + str(line_number) + ": " +
162 line_text)
163 # Start a new interval if none started
164 if previous_range_start_line_number == 0:
165 previous_range_start_line_number = line_number
166 previous_range_start_text = line_text
167 # Add new interval if we reached past the previous one
168 elif line_number != previous_line_number + 1:
169 addRange()
170 previous_range_start_line_number = line_number
171 previous_range_start_text = line_text
172 previous_line_number = line_number
173
174 # Add a last interval if needed
175 if previous_range_start_line_number != 0:
176 addRange()
177 return results
178
179 def CheckForFileDeletion(self, affected_file):
180 """Emits a warning notification if file has been deleted """
181 if not affected_file.NewContents():
182 self.EmitWarning("The file seems to be deleted in the changelist. If "
183 "your intent is to really delete the file, the code in "
184 "PRESUBMIT.py should be updated to remove the "
185 "|HistogramValueChecker| class.");
186 return False
187 return True
188
189 def GetDeletedLinesFromScmDiff(self, affected_file):
190 """Return a list of of line numbers (1-based) corresponding to lines
191 deleted from the new source file (if they had been present in it). Note
192 that if multiple contiguous lines have been deleted, the returned list will
193 contain contiguous line number entries. To prevent false positives, we
194 return deleted line numbers *only* from diff chunks which decrease the size
195 of the new file.
196
197 Note: We need this method because we have access to neither the old file
198 content nor the list of "delete" changes from the current presubmit script
199 API.
200
201 """
202 results = []
203 line_num = 0
204 deleting_lines = False
205 for line in affected_file.GenerateScmDiff().splitlines():
206 # Parse the unified diff chunk optional section heading, which looks like
207 # @@ -l,s +l,s @@ optional section heading
208 m = self.input_api.re.match(
209 r'^@@ \-([0-9]+)\,([0-9]+) \+([0-9]+)\,([0-9]+) @@', line)
210 if m:
211 old_line_num = int(m.group(1))
212 old_size = int(m.group(2))
213 new_line_num = int(m.group(3))
214 new_size = int(m.group(4))
215 line_num = new_line_num
216 # Return line numbers only from diff chunks decreasing the size of the
217 # new file
218 deleting_lines = old_size > new_size
219 continue
220 if not line.startswith('-'):
221 line_num += 1
222 if deleting_lines and line.startswith('-') and not line.startswith('--'):
223 results.append(line_num)
224 return results
225
226 def CheckForEnumEntryDeletions(self, affected_file):
227 """Look for deletions inside the enum definition. We currently use a
228 simple heuristics (not 100% accurate): if there are deleted lines inside
229 the enum definition, this might be a deletion.
230
231 """
232 range_new = self.ComputeEnumRangeInNewFile(affected_file)
233 if not range_new:
234 return False
235
236 is_ok = True
237 for line_num in self.GetDeletedLinesFromScmDiff(affected_file):
238 if range_new.Contains(line_num):
239 self.EmitWarning("It looks like you are deleting line(s) from the "
240 "enum definition. This should never happen.",
241 line_num)
242 is_ok = False
243 return is_ok
244
245 def CheckForEnumEntryInsertions(self, affected_file):
246 range = self.ComputeEnumRangeInNewFile(affected_file)
247 if not range:
248 return False
249
250 first_line = range.first_line
251 last_line = range.last_line
252
253 # Collect the range of changes inside the enum definition range.
254 is_ok = True
255 for line_start, line_end, line_text in \
256 self.CollectRangesInsideEnumDefinition(affected_file,
257 first_line,
258 last_line):
259 # The only edit we consider valid is adding 1 or more entries *exactly*
260 # at the end of the enum definition. Every other edit inside the enum
261 # definition will result in a "warning confirmation" message.
262 #
263 # TODO(rpaquay): We currently cannot detect "renames" of existing entries
264 # vs invalid insertions, so we sometimes will warn for valid edits.
265 is_valid_edit = (line_end == last_line - 1)
266
267 self.LogDebug("Edit range in new file at starting at line number %d and "
268 "ending at line number %d: valid=%s"
269 % (line_start, line_end, is_valid_edit))
270
271 if not is_valid_edit:
272 self.EmitWarning("The change starting at line %d and ending at line "
273 "%d is *not* located *exactly* at the end of the "
274 "enum definition. Unless you are renaming an "
275 "existing entry, this is not a valid changes, as new "
276 "entries should *always* be added at the end of the "
277 "enum definition, right before the 'ENUM_BOUNDARY' "
278 "entry." % (line_start, line_end),
279 line_start,
280 line_text)
281 is_ok = False
282 return is_ok
283
284 def PerformChecks(self, affected_file):
285 if not self.CheckForFileDeletion(affected_file):
286 return
287 if not self.CheckForEnumEntryDeletions(affected_file):
288 return
289 if not self.CheckForEnumEntryInsertions(affected_file):
290 return
291
292 def ProcessHistogramValueFile(self, affected_file):
293 self.LogInfo("Start processing file \"%s\"" % affected_file.LocalPath())
294 self.PerformChecks(affected_file)
295 self.LogInfo("Done processing file \"%s\"" % affected_file.LocalPath())
296
297 def Run(self):
298 for file in self.input_api.AffectedFiles(include_deletes=True):
299 if file.LocalPath() == self.LOCAL_PATH:
300 self.ProcessHistogramValueFile(file)
301 return self.results
302 34
303 def CheckChangeOnUpload(input_api, output_api): 35 def CheckChangeOnUpload(input_api, output_api):
304 results = [] 36 results = []
305 results += HistogramValueChecker(input_api, output_api).Run() 37 results += _CreateHistogramValueChecker(input_api, output_api).Run()
306 results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) 38 results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api)
307 return results 39 return results
308 40
41 def CheckChangeOnCommit(input_api, output_api):
42 return _CreateHistogramValueChecker(input_api, output_api).Run()
OLDNEW
« no previous file with comments | « chrome/browser/extensions/PRESUBMIT.py ('k') | extensions/browser/PRESUBMIT_test.py » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698