Skip to content

Commit

Permalink
[android/ios/presubmit] Warn about deprecated sync consent predicates
Browse files Browse the repository at this point in the history
Bug: 40066949
Change-Id: Ib3dda14f33c6bea7ffcd12b6f8803d6d1a5e4095
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5553534
Reviewed-by: Dominic Battre <[email protected]>
Auto-Submit: Victor Vianna <[email protected]>
Commit-Queue: Victor Vianna <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1303675}
  • Loading branch information
Victor Hugo Vianna Silva authored and Chromium LUCI CQ committed May 21, 2024
1 parent cd6139d commit dbe8154
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 0 deletions.
81 changes: 81 additions & 0 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -1966,6 +1966,67 @@ class BanRule:
),
)

_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING = (
'Used a predicate related to signin::ConsentLevel::kSync which will always '
'return false in the future (crbug.com/40066949). Prefer using a predicate '
'that also supports signin::ConsentLevel::kSignin when appropriate. It is '
'safe to ignore this warning if you are just moving an existing call, or if '
'you want special handling for users in the legacy state. In doubt, reach '
'out to //components/sync/OWNERS.'
)

# C++ functions related to signin::ConsentLevel::kSync which are deprecated.
_DEPRECATED_SYNC_CONSENT_CPP_FUNCTIONS : Sequence[BanRule] = (
BanRule(
'HasSyncConsent',
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
False,
),
BanRule(
'CanSyncFeatureStart',
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
False,
),
BanRule(
'IsSyncFeatureEnabled',
(
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
),
False,
),
BanRule(
'IsSyncFeatureActive',
(
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
),
False,
),
)

# Java functions related to signin::ConsentLevel::kSync which are deprecated.
_DEPRECATED_SYNC_CONSENT_JAVA_FUNCTIONS : Sequence[BanRule] = (
BanRule(
'hasSyncConsent',
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
False,
),
BanRule(
'canSyncFeatureStart',
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
False,
),
BanRule(
'isSyncFeatureEnabled',
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
False,
),
BanRule(
'isSyncFeatureActive',
_DEPRECATED_SYNC_CONSENT_FUNCTION_WARNING,
False,
),
)

_BANNED_MOJOM_PATTERNS : Sequence[BanRule] = (
BanRule(
'handle<shared_buffer>',
Expand Down Expand Up @@ -2735,6 +2796,26 @@ def CheckForMatch(affected_file, line_num: int, line: str,
for ban_rule in _BANNED_CPP_FUNCTIONS:
CheckForMatch(f, line_num, line, ban_rule)

# As of 05/2024, iOS fully migrated ConsentLevel::kSync to kSignin, and
# Android is in the process of preventing new users from entering kSync.
# So the warning is restricted to those platforms.
ios_pattern = input_api.re.compile('(^|[\W_])ios[\W_]')
file_filter = lambda f: (f.LocalPath().endswith(('.cc', '.mm', '.h')) and
('android' in f.LocalPath() or
# Simply checking for an 'ios' substring would
# catch unrelated cases, use a regex.
ios_pattern.search(f.LocalPath())))
for f in input_api.AffectedFiles(file_filter=file_filter):
for line_num, line in f.ChangedContents():
for ban_rule in _DEPRECATED_SYNC_CONSENT_CPP_FUNCTIONS:
CheckForMatch(f, line_num, line, ban_rule)

file_filter = lambda f: f.LocalPath().endswith(('.java'))
for f in input_api.AffectedFiles(file_filter=file_filter):
for line_num, line in f.ChangedContents():
for ban_rule in _DEPRECATED_SYNC_CONSENT_JAVA_FUNCTIONS:
CheckForMatch(f, line_num, line, ban_rule)

file_filter = lambda f: f.LocalPath().endswith(('.mojom'))
for f in input_api.AffectedFiles(file_filter=file_filter):
for line_num, line in f.ChangedContents():
Expand Down
60 changes: 60 additions & 0 deletions PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5331,5 +5331,65 @@ def testTodoBugReferencesWithUpdatedBugId(self):
warnings = PRESUBMIT.CheckTodoBugReferences(input_api, MockOutputApi())
self.assertEqual(0, len(warnings))

class CheckDeprecatedSyncConsentFunctionsTest(unittest.TestCase):
"""Test the presubmit for deprecated ConsentLevel::kSync functions."""

def testCppMobilePlatformPath(self):
input_api = MockInputApi()
input_api.files = [
MockFile('chrome/browser/android/file.cc', ['OtherFunction']),
MockFile('chrome/android/file.cc', ['HasSyncConsent']),
MockFile('ios/file.mm', ['CanSyncFeatureStart']),
MockFile('components/foo/ios/file.cc', ['IsSyncFeatureEnabled']),
MockFile('components/foo/delegate_android.cc', ['IsSyncFeatureActive']),
MockFile('components/foo/delegate_ios.cc', ['IsSyncFeatureActive']),
MockFile('components/foo/android_delegate.cc', ['IsSyncFeatureActive']),
MockFile('components/foo/ios_delegate.cc', ['IsSyncFeatureActive']),
]

results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())

self.assertEqual(1, len(results))
self.assertFalse('chrome/browser/android/file.cc' in results[0].message),
self.assertTrue('chrome/android/file.cc' in results[0].message),
self.assertTrue('ios/file.mm' in results[0].message),
self.assertTrue('components/foo/ios/file.cc' in results[0].message),
self.assertTrue('components/foo/delegate_android.cc' in results[0].message),
self.assertTrue('components/foo/delegate_ios.cc' in results[0].message),
self.assertTrue('components/foo/android_delegate.cc' in results[0].message),
self.assertTrue('components/foo/ios_delegate.cc' in results[0].message),

def testCppNonMobilePlatformPath(self):
input_api = MockInputApi()
input_api.files = [
MockFile('chrome/browser/file.cc', ['HasSyncConsent']),
MockFile('bios/file.cc', ['HasSyncConsent']),
MockFile('components/kiosk/file.cc', ['HasSyncConsent']),
]

results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())

self.assertEqual(0, len(results))

def testJavaPath(self):
input_api = MockInputApi()
input_api.files = [
MockFile('components/foo/file1.java', ['otherFunction']),
MockFile('components/foo/file2.java', ['hasSyncConsent']),
MockFile('chrome/foo/file3.java', ['canSyncFeatureStart']),
MockFile('chrome/foo/file4.java', ['isSyncFeatureEnabled']),
MockFile('chrome/foo/file5.java', ['isSyncFeatureActive']),
]

results = PRESUBMIT.CheckNoBannedFunctions(input_api, MockOutputApi())

self.assertEqual(1, len(results))
self.assertFalse('components/foo/file1.java' in results[0].message),
self.assertTrue('components/foo/file2.java' in results[0].message),
self.assertTrue('chrome/foo/file3.java' in results[0].message),
self.assertTrue('chrome/foo/file4.java' in results[0].message),
self.assertTrue('chrome/foo/file5.java' in results[0].message),


if __name__ == '__main__':
unittest.main()

0 comments on commit dbe8154

Please sign in to comment.