Check deprecation of preferences
It's a common mistake that developers just remove Register...Pref() calls for preferences they don't need anymore. If they do this, the preference stays in the prefs files on disk for ever. A proper approach is to first ClearPref() the preference for some releases and then to remove the code. This CL introduces a presubmit warning if we detect that a Register...Pref() call is removed from a non-unittest. Bug: 1153014 Change-Id: If8f19933de039553ef47e0ddce12eb194df45ce1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2560205 Commit-Queue: Dominic Battré <battre@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: Dominic Battré <battre@chromium.org> Cr-Commit-Position: refs/heads/master@{#833730}
This commit is contained in:
Dominic Battre
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
2d3a3d1cbc
commit
645d4234a6
112
PRESUBMIT.py
112
PRESUBMIT.py
@ -5214,3 +5214,115 @@ def CheckStableMojomChanges(input_api, output_api):
|
|||||||
'in a way that is not backward-compatible.',
|
'in a way that is not backward-compatible.',
|
||||||
long_text=error)]
|
long_text=error)]
|
||||||
return []
|
return []
|
||||||
|
|
||||||
|
def CheckDeprecationOfPreferences(input_api, output_api):
|
||||||
|
"""Removing a preference should come with a deprecation."""
|
||||||
|
|
||||||
|
def FilterFile(affected_file):
|
||||||
|
"""Accept only .cc files and the like."""
|
||||||
|
file_inclusion_pattern = [r'.+%s' % _IMPLEMENTATION_EXTENSIONS]
|
||||||
|
files_to_skip = (_EXCLUDED_PATHS +
|
||||||
|
_TEST_CODE_EXCLUDED_PATHS +
|
||||||
|
input_api.DEFAULT_FILES_TO_SKIP)
|
||||||
|
return input_api.FilterSourceFile(
|
||||||
|
affected_file,
|
||||||
|
files_to_check=file_inclusion_pattern,
|
||||||
|
files_to_skip=files_to_skip)
|
||||||
|
|
||||||
|
def ModifiedLines(affected_file):
|
||||||
|
"""Returns a list of tuples (line number, line text) of added and removed
|
||||||
|
lines.
|
||||||
|
|
||||||
|
Deleted lines share the same line number as the previous line.
|
||||||
|
|
||||||
|
This relies on the scm diff output describing each changed code section
|
||||||
|
with a line of the form
|
||||||
|
|
||||||
|
^@@ <old line num>,<old size> <new line num>,<new size> @@$
|
||||||
|
"""
|
||||||
|
line_num = 0
|
||||||
|
modified_lines = []
|
||||||
|
for line in affected_file.GenerateScmDiff().splitlines():
|
||||||
|
# Extract <new line num> of the patch fragment (see format above).
|
||||||
|
m = input_api.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('++')) or
|
||||||
|
(line.startswith('-') and not line.startswith('--'))):
|
||||||
|
modified_lines.append((line_num, line))
|
||||||
|
|
||||||
|
if not line.startswith('-'):
|
||||||
|
line_num += 1
|
||||||
|
return modified_lines
|
||||||
|
|
||||||
|
def FindLineWith(lines, needle):
|
||||||
|
"""Returns the line number (i.e. index + 1) in `lines` containing `needle`.
|
||||||
|
|
||||||
|
If 0 or >1 lines contain `needle`, -1 is returned.
|
||||||
|
"""
|
||||||
|
matching_line_numbers = [
|
||||||
|
# + 1 for 1-based counting of line numbers.
|
||||||
|
i + 1 for i, line
|
||||||
|
in enumerate(lines)
|
||||||
|
if needle in line]
|
||||||
|
return matching_line_numbers[0] if len(matching_line_numbers) == 1 else -1
|
||||||
|
|
||||||
|
def ModifiedPrefMigration(affected_file):
|
||||||
|
"""Returns whether the MigrateObsolete.*Pref functions were modified."""
|
||||||
|
# Determine first and last lines of MigrateObsolete.*Pref functions.
|
||||||
|
new_contents = affected_file.NewContents();
|
||||||
|
range_1 = (
|
||||||
|
FindLineWith(new_contents, 'BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS'),
|
||||||
|
FindLineWith(new_contents, 'END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS'))
|
||||||
|
range_2 = (
|
||||||
|
FindLineWith(new_contents, 'BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS'),
|
||||||
|
FindLineWith(new_contents, 'END_MIGRATE_OBSOLETE_PROFILE_PREFS'))
|
||||||
|
if (-1 in range_1 + range_2):
|
||||||
|
raise Exception(
|
||||||
|
'Broken .*MIGRATE_OBSOLETE_.*_PREFS markers in browser_prefs.cc.')
|
||||||
|
|
||||||
|
# Check whether any of the modified lines are part of the
|
||||||
|
# MigrateObsolete.*Pref functions.
|
||||||
|
for line_nr, line in ModifiedLines(affected_file):
|
||||||
|
if (range_1[0] <= line_nr <= range_1[1] or
|
||||||
|
range_2[0] <= line_nr <= range_2[1]):
|
||||||
|
return True
|
||||||
|
return False
|
||||||
|
|
||||||
|
register_pref_pattern = input_api.re.compile(r'Register.+Pref')
|
||||||
|
browser_prefs_file_pattern = input_api.re.compile(
|
||||||
|
r'chrome/browser/prefs/browser_prefs.cc')
|
||||||
|
|
||||||
|
changes = input_api.AffectedFiles(include_deletes=True,
|
||||||
|
file_filter=FilterFile)
|
||||||
|
potential_problems = []
|
||||||
|
for f in changes:
|
||||||
|
for line in f.GenerateScmDiff().splitlines():
|
||||||
|
# Check deleted lines for pref registrations.
|
||||||
|
if (line.startswith('-') and not line.startswith('--') and
|
||||||
|
register_pref_pattern.search(line)):
|
||||||
|
potential_problems.append('%s: %s' % (f.LocalPath(), line))
|
||||||
|
|
||||||
|
if browser_prefs_file_pattern.search(f.LocalPath()):
|
||||||
|
# If the developer modified the MigrateObsolete.*Prefs() functions, we
|
||||||
|
# assume that they knew that they have to deprecate preferences and don't
|
||||||
|
# warn.
|
||||||
|
try:
|
||||||
|
if ModifiedPrefMigration(f):
|
||||||
|
return []
|
||||||
|
except Exception as e:
|
||||||
|
return [output_api.PresubmitError(str(e))]
|
||||||
|
|
||||||
|
if potential_problems:
|
||||||
|
return [output_api.PresubmitPromptWarning(
|
||||||
|
'Discovered possible removal of preference registrations.\n\n'
|
||||||
|
'Please make sure to properly deprecate preferences by clearing their\n'
|
||||||
|
'value for a couple of milestones before finally removing the code.\n'
|
||||||
|
'Otherwise data may stay in the preferences files forever. See\n'
|
||||||
|
'Migrate*Prefs() in chrome/browser/prefs/browser_prefs.cc for examples.\n'
|
||||||
|
'This may be a false positive warning (e.g. if you move preference\n'
|
||||||
|
'registrations to a different place).\n',
|
||||||
|
potential_problems
|
||||||
|
)]
|
||||||
|
return []
|
||||||
|
|||||||
@ -3658,5 +3658,156 @@ class MojomStabilityCheckTest(unittest.TestCase):
|
|||||||
self.assertEqual([], errors)
|
self.assertEqual([], errors)
|
||||||
|
|
||||||
|
|
||||||
|
class CheckDeprecationOfPreferencesTest(unittest.TestCase):
|
||||||
|
# Test that a warning is generated if a preference registration is removed
|
||||||
|
# from a random file.
|
||||||
|
def testWarning(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
MockAffectedFile(
|
||||||
|
'foo.cc',
|
||||||
|
['A', 'B'],
|
||||||
|
['A', 'prefs->RegisterStringPref("foo", "default");', 'B'],
|
||||||
|
scm_diff='\n'.join([
|
||||||
|
'--- foo.cc.old 2020-12-02 20:40:54.430676385 +0100',
|
||||||
|
'+++ foo.cc.new 2020-12-02 20:41:02.086700197 +0100',
|
||||||
|
'@@ -1,3 +1,2 @@',
|
||||||
|
' A',
|
||||||
|
'-prefs->RegisterStringPref("foo", "default");',
|
||||||
|
' B']),
|
||||||
|
action='M')
|
||||||
|
]
|
||||||
|
mock_output_api = MockOutputApi()
|
||||||
|
errors = PRESUBMIT.CheckDeprecationOfPreferences(mock_input_api,
|
||||||
|
mock_output_api)
|
||||||
|
self.assertEqual(1, len(errors))
|
||||||
|
self.assertTrue(
|
||||||
|
'Discovered possible removal of preference registrations' in
|
||||||
|
errors[0].message)
|
||||||
|
|
||||||
|
# Test that a warning is inhibited if the preference registration was moved
|
||||||
|
# to the deprecation functions in browser prefs.
|
||||||
|
def testNoWarningForMigration(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
# RegisterStringPref was removed from foo.cc.
|
||||||
|
MockAffectedFile(
|
||||||
|
'foo.cc',
|
||||||
|
['A', 'B'],
|
||||||
|
['A', 'prefs->RegisterStringPref("foo", "default");', 'B'],
|
||||||
|
scm_diff='\n'.join([
|
||||||
|
'--- foo.cc.old 2020-12-02 20:40:54.430676385 +0100',
|
||||||
|
'+++ foo.cc.new 2020-12-02 20:41:02.086700197 +0100',
|
||||||
|
'@@ -1,3 +1,2 @@',
|
||||||
|
' A',
|
||||||
|
'-prefs->RegisterStringPref("foo", "default");',
|
||||||
|
' B']),
|
||||||
|
action='M'),
|
||||||
|
# But the preference was properly migrated.
|
||||||
|
MockAffectedFile(
|
||||||
|
'chrome/browser/prefs/browser_prefs.cc',
|
||||||
|
[
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
'prefs->RegisterStringPref("foo", "default");',
|
||||||
|
'// END_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
],
|
||||||
|
[
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
],
|
||||||
|
scm_diff='\n'.join([
|
||||||
|
'--- browser_prefs.cc.old 2020-12-02 20:51:40.812686731 +0100',
|
||||||
|
'+++ browser_prefs.cc.new 2020-12-02 20:52:02.936755539 +0100',
|
||||||
|
'@@ -2,3 +2,4 @@',
|
||||||
|
' // END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
' // BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
'+prefs->RegisterStringPref("foo", "default");',
|
||||||
|
' // END_MIGRATE_OBSOLETE_PROFILE_PREFS']),
|
||||||
|
action='M'),
|
||||||
|
]
|
||||||
|
mock_output_api = MockOutputApi()
|
||||||
|
errors = PRESUBMIT.CheckDeprecationOfPreferences(mock_input_api,
|
||||||
|
mock_output_api)
|
||||||
|
self.assertEqual(0, len(errors))
|
||||||
|
|
||||||
|
# Test that a warning is NOT inhibited if the preference registration was
|
||||||
|
# moved to a place outside of the migration functions in browser_prefs.cc
|
||||||
|
def testWarningForImproperMigration(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
# RegisterStringPref was removed from foo.cc.
|
||||||
|
MockAffectedFile(
|
||||||
|
'foo.cc',
|
||||||
|
['A', 'B'],
|
||||||
|
['A', 'prefs->RegisterStringPref("foo", "default");', 'B'],
|
||||||
|
scm_diff='\n'.join([
|
||||||
|
'--- foo.cc.old 2020-12-02 20:40:54.430676385 +0100',
|
||||||
|
'+++ foo.cc.new 2020-12-02 20:41:02.086700197 +0100',
|
||||||
|
'@@ -1,3 +1,2 @@',
|
||||||
|
' A',
|
||||||
|
'-prefs->RegisterStringPref("foo", "default");',
|
||||||
|
' B']),
|
||||||
|
action='M'),
|
||||||
|
# The registration call was moved to a place in browser_prefs.cc that
|
||||||
|
# is outside the migration functions.
|
||||||
|
MockAffectedFile(
|
||||||
|
'chrome/browser/prefs/browser_prefs.cc',
|
||||||
|
[
|
||||||
|
'prefs->RegisterStringPref("foo", "default");',
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
],
|
||||||
|
[
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
],
|
||||||
|
scm_diff='\n'.join([
|
||||||
|
'--- browser_prefs.cc.old 2020-12-02 20:51:40.812686731 +0100',
|
||||||
|
'+++ browser_prefs.cc.new 2020-12-02 20:52:02.936755539 +0100',
|
||||||
|
'@@ -1,2 +1,3 @@',
|
||||||
|
'+prefs->RegisterStringPref("foo", "default");',
|
||||||
|
' // BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
' // END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS']),
|
||||||
|
action='M'),
|
||||||
|
]
|
||||||
|
mock_output_api = MockOutputApi()
|
||||||
|
errors = PRESUBMIT.CheckDeprecationOfPreferences(mock_input_api,
|
||||||
|
mock_output_api)
|
||||||
|
self.assertEqual(1, len(errors))
|
||||||
|
self.assertTrue(
|
||||||
|
'Discovered possible removal of preference registrations' in
|
||||||
|
errors[0].message)
|
||||||
|
|
||||||
|
# Check that the presubmit fails if a marker line in brower_prefs.cc is
|
||||||
|
# deleted.
|
||||||
|
def testDeletedMarkerRaisesError(self):
|
||||||
|
mock_input_api = MockInputApi()
|
||||||
|
mock_input_api.files = [
|
||||||
|
MockAffectedFile('chrome/browser/prefs/browser_prefs.cc',
|
||||||
|
[
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS',
|
||||||
|
'// BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
# The following line is deleted for this test
|
||||||
|
# '// END_MIGRATE_OBSOLETE_PROFILE_PREFS',
|
||||||
|
])
|
||||||
|
]
|
||||||
|
mock_output_api = MockOutputApi()
|
||||||
|
errors = PRESUBMIT.CheckDeprecationOfPreferences(mock_input_api,
|
||||||
|
mock_output_api)
|
||||||
|
self.assertEqual(1, len(errors))
|
||||||
|
self.assertEqual(
|
||||||
|
'Broken .*MIGRATE_OBSOLETE_.*_PREFS markers in browser_prefs.cc.',
|
||||||
|
errors[0].message)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == '__main__':
|
if __name__ == '__main__':
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
@ -179,16 +179,21 @@ class MockFile(object):
|
|||||||
MockInputApi for presubmit unittests.
|
MockInputApi for presubmit unittests.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def __init__(self, local_path, new_contents, old_contents=None, action='A'):
|
def __init__(self, local_path, new_contents, old_contents=None, action='A',
|
||||||
|
scm_diff=None):
|
||||||
self._local_path = local_path
|
self._local_path = local_path
|
||||||
self._new_contents = new_contents
|
self._new_contents = new_contents
|
||||||
self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)]
|
self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)]
|
||||||
self._action = action
|
self._action = action
|
||||||
self._scm_diff = "--- /dev/null\n+++ %s\n@@ -0,0 +1,%d @@\n" % (local_path,
|
if scm_diff:
|
||||||
len(new_contents))
|
self._scm_diff = scm_diff
|
||||||
|
else:
|
||||||
|
self._scm_diff = (
|
||||||
|
"--- /dev/null\n+++ %s\n@@ -0,0 +1,%d @@\n" %
|
||||||
|
(local_path, len(new_contents)))
|
||||||
|
for l in new_contents:
|
||||||
|
self._scm_diff += "+%s\n" % l
|
||||||
self._old_contents = old_contents
|
self._old_contents = old_contents
|
||||||
for l in new_contents:
|
|
||||||
self._scm_diff += "+%s\n" % l
|
|
||||||
|
|
||||||
def Action(self):
|
def Action(self):
|
||||||
return self._action
|
return self._action
|
||||||
|
|||||||
@ -1077,6 +1077,9 @@ void RegisterSigninProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
|
|||||||
|
|
||||||
// This method should be periodically pruned of year+ old migrations.
|
// This method should be periodically pruned of year+ old migrations.
|
||||||
void MigrateObsoleteLocalStatePrefs(PrefService* local_state) {
|
void MigrateObsoleteLocalStatePrefs(PrefService* local_state) {
|
||||||
|
// BEGIN_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS
|
||||||
|
// Please don't delete the preceding line. It is used by PRESUBMIT.py.
|
||||||
|
|
||||||
// Added 1/2020
|
// Added 1/2020
|
||||||
#if defined(OS_MAC)
|
#if defined(OS_MAC)
|
||||||
local_state->ClearPref(kKeyCreated);
|
local_state->ClearPref(kKeyCreated);
|
||||||
@ -1094,10 +1097,16 @@ void MigrateObsoleteLocalStatePrefs(PrefService* local_state) {
|
|||||||
// Added 4/2020.
|
// Added 4/2020.
|
||||||
local_state->ClearPref(kSupervisedUsersNextId);
|
local_state->ClearPref(kSupervisedUsersNextId);
|
||||||
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
|
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
|
||||||
|
|
||||||
|
// Please don't delete the following line. It is used by PRESUBMIT.py.
|
||||||
|
// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS
|
||||||
}
|
}
|
||||||
|
|
||||||
// This method should be periodically pruned of year+ old migrations.
|
// This method should be periodically pruned of year+ old migrations.
|
||||||
void MigrateObsoleteProfilePrefs(Profile* profile) {
|
void MigrateObsoleteProfilePrefs(Profile* profile) {
|
||||||
|
// BEGIN_MIGRATE_OBSOLETE_PROFILE_PREFS
|
||||||
|
// Please don't delete the preceding line. It is used by PRESUBMIT.py.
|
||||||
|
|
||||||
PrefService* profile_prefs = profile->GetPrefs();
|
PrefService* profile_prefs = profile->GetPrefs();
|
||||||
|
|
||||||
// Check MigrateDeprecatedAutofillPrefs() to see if this is safe to remove.
|
// Check MigrateDeprecatedAutofillPrefs() to see if this is safe to remove.
|
||||||
@ -1188,4 +1197,7 @@ void MigrateObsoleteProfilePrefs(Profile* profile) {
|
|||||||
|
|
||||||
// Added 11/2020
|
// Added 11/2020
|
||||||
profile_prefs->ClearPref(kDRMSalt);
|
profile_prefs->ClearPref(kDRMSalt);
|
||||||
|
|
||||||
|
// Please don't delete the following line. It is used by PRESUBMIT.py.
|
||||||
|
// END_MIGRATE_OBSOLETE_PROFILE_PREFS
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user