Remove MPArch triage rotation presubmit
We are winding down the triage rotation, so the associated presubmit script can be removed. Bug: 1190019 Change-Id: If6226bae54a7bfc71df95211d6c00f12db10dd6c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4228276 Auto-Submit: Kevin McNee <mcnee@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/main@{#1103922}
This commit is contained in:
Kevin McNee
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
3bef07decd
commit
1746306427
124
PRESUBMIT.py
124
PRESUBMIT.py
@ -6473,130 +6473,6 @@ def CheckConsistentGrdChanges(input_api, output_api):
|
||||
return errors
|
||||
|
||||
|
||||
def CheckMPArchApiUsage(input_api, output_api):
|
||||
"""CC the MPArch watchlist if the CL uses an API that is ambiguous in the
|
||||
presence of MPArch features such as bfcache, prerendering, and fenced frames.
|
||||
"""
|
||||
|
||||
# Only consider top-level directories that (1) can use content APIs or
|
||||
# problematic blink APIs, (2) apply to desktop or android chrome, and (3)
|
||||
# are known to have a significant number of uses of the APIs of concern.
|
||||
files_to_check = (
|
||||
r'^(chrome|components|content|extensions|third_party/blink/renderer)/.+%s' %
|
||||
_IMPLEMENTATION_EXTENSIONS,
|
||||
r'^(chrome|components|content|extensions|third_party/blink/renderer)/.+%s' %
|
||||
_HEADER_EXTENSIONS,
|
||||
)
|
||||
files_to_skip = (_EXCLUDED_PATHS + _TEST_CODE_EXCLUDED_PATHS +
|
||||
input_api.DEFAULT_FILES_TO_SKIP)
|
||||
source_file_filter = lambda f: input_api.FilterSourceFile(
|
||||
f, files_to_check=files_to_check, files_to_skip=files_to_skip)
|
||||
|
||||
# Here we list the classes/methods we're monitoring. For the "fyi" cases,
|
||||
# we add the CL to the watchlist, but we don't omit a warning or have it be
|
||||
# included in the triage rotation.
|
||||
# Note that since these are are just regular expressions and we don't have
|
||||
# the compiler's AST, we could have spurious matches (e.g. an unrelated class
|
||||
# could have a method named IsInMainFrame).
|
||||
fyi_concerning_class_pattern = input_api.re.compile(
|
||||
r'WebContentsObserver|WebContentsUserData')
|
||||
# A subset of WebContentsObserver overrides where there's particular risk for
|
||||
# confusing tab and page level operations and data (e.g. incorrectly
|
||||
# resetting page state in DidFinishNavigation).
|
||||
fyi_concerning_wco_methods = [
|
||||
'DidStartNavigation',
|
||||
'ReadyToCommitNavigation',
|
||||
'DidFinishNavigation',
|
||||
'RenderViewReady',
|
||||
'RenderViewDeleted',
|
||||
'RenderViewHostChanged',
|
||||
'DOMContentLoaded',
|
||||
'DidFinishLoad',
|
||||
]
|
||||
concerning_nav_handle_methods = [
|
||||
'IsInMainFrame',
|
||||
]
|
||||
concerning_web_contents_methods = [
|
||||
'FromRenderFrameHost',
|
||||
'FromRenderViewHost',
|
||||
]
|
||||
fyi_concerning_web_contents_methods = [
|
||||
'GetRenderViewHost',
|
||||
]
|
||||
concerning_rfh_methods = [
|
||||
'GetParent',
|
||||
'GetMainFrame',
|
||||
]
|
||||
fyi_concerning_rfh_methods = [
|
||||
'GetFrameTreeNodeId',
|
||||
]
|
||||
concerning_rfhi_methods = [
|
||||
'is_main_frame',
|
||||
]
|
||||
concerning_ftn_methods = [
|
||||
'IsMainFrame',
|
||||
]
|
||||
concerning_blink_frame_methods = [
|
||||
'IsCrossOriginToNearestMainFrame',
|
||||
]
|
||||
concerning_method_pattern = input_api.re.compile(r'(' + r'|'.join(
|
||||
item for sublist in [
|
||||
concerning_nav_handle_methods,
|
||||
concerning_web_contents_methods, concerning_rfh_methods,
|
||||
concerning_rfhi_methods, concerning_ftn_methods,
|
||||
concerning_blink_frame_methods,
|
||||
] for item in sublist) + r')\(')
|
||||
fyi_concerning_method_pattern = input_api.re.compile(r'(' + r'|'.join(
|
||||
item for sublist in [
|
||||
fyi_concerning_wco_methods, fyi_concerning_web_contents_methods,
|
||||
fyi_concerning_rfh_methods,
|
||||
] for item in sublist) + r')\(')
|
||||
|
||||
used_apis = set()
|
||||
used_fyi_methods = False
|
||||
for f in input_api.AffectedFiles(include_deletes=False,
|
||||
file_filter=source_file_filter):
|
||||
for line_num, line in f.ChangedContents():
|
||||
fyi_class_match = fyi_concerning_class_pattern.search(line)
|
||||
if fyi_class_match:
|
||||
used_fyi_methods = True
|
||||
fyi_method_match = fyi_concerning_method_pattern.search(line)
|
||||
if fyi_method_match:
|
||||
used_fyi_methods = True
|
||||
method_match = concerning_method_pattern.search(line)
|
||||
if method_match:
|
||||
used_apis.add(method_match[1])
|
||||
|
||||
if not used_apis:
|
||||
if used_fyi_methods:
|
||||
output_api.AppendCC('mparch-reviews+watchfyi@chromium.org')
|
||||
|
||||
return []
|
||||
|
||||
output_api.AppendCC('mparch-reviews+watch@chromium.org')
|
||||
message = ('This change uses API(s) that are ambiguous in the presence of '
|
||||
'MPArch features such as bfcache, prerendering, and fenced '
|
||||
'frames.')
|
||||
explanation = (
|
||||
'Please double check whether new code assumes that a WebContents only '
|
||||
'contains a single page at a time. Notably, checking whether a frame '
|
||||
'is the \"main frame\" is not specific enough to determine whether it '
|
||||
'corresponds to the document reflected in the omnibox. A WebContents '
|
||||
'may have additional main frames for prerendered pages, bfcached '
|
||||
'pages, fenced frames, etc. '
|
||||
'See this doc [1] and the comments on the individual APIs '
|
||||
'for guidance and this doc [2] for context. The MPArch review '
|
||||
'watchlist has been CC\'d on this change to help identify any issues.\n'
|
||||
'[1] https://docs.google.com/document/d/13l16rWTal3o5wce4i0RwdpMP5ESELLKr439Faj2BBRo/edit?usp=sharing\n'
|
||||
'[2] https://docs.google.com/document/d/1NginQ8k0w3znuwTiJ5qjYmBKgZDekvEPC22q0I4swxQ/edit?usp=sharing'
|
||||
)
|
||||
return [
|
||||
output_api.PresubmitNotifyResult(message,
|
||||
items=list(used_apis),
|
||||
long_text=explanation)
|
||||
]
|
||||
|
||||
|
||||
def CheckAssertAshOnlyCode(input_api, output_api):
|
||||
"""Errors if a BUILD.gn file in an ash/ directory doesn't include
|
||||
assert(is_chromeos_ash).
|
||||
|
||||
@ -4461,103 +4461,6 @@ class CheckCrosApiNeedBrowserTestTest(unittest.TestCase):
|
||||
self.assertEqual(0, len(result))
|
||||
|
||||
|
||||
class MPArchApiUsage(unittest.TestCase):
|
||||
def _assert_notify(
|
||||
self, expected_uses, expect_fyi, msg, local_path, new_contents):
|
||||
mock_input_api = MockInputApi()
|
||||
mock_output_api = MockOutputApi()
|
||||
mock_input_api.files = [
|
||||
MockFile(local_path, new_contents),
|
||||
]
|
||||
result = PRESUBMIT.CheckMPArchApiUsage(mock_input_api, mock_output_api)
|
||||
|
||||
watchlist_email = ('mparch-reviews+watchfyi@chromium.org'
|
||||
if expect_fyi else 'mparch-reviews+watch@chromium.org')
|
||||
self.assertEqual(
|
||||
bool(expected_uses or expect_fyi),
|
||||
watchlist_email in mock_output_api.more_cc,
|
||||
msg)
|
||||
if expected_uses:
|
||||
self.assertEqual(1, len(result), msg)
|
||||
self.assertEqual(result[0].type, 'notify', msg)
|
||||
self.assertEqual(sorted(result[0].items), sorted(expected_uses), msg)
|
||||
else:
|
||||
self.assertEqual(0, len(result), msg)
|
||||
|
||||
def testNotify(self):
|
||||
self._assert_notify(
|
||||
['IsInMainFrame'],
|
||||
False,
|
||||
'Introduce IsInMainFrame',
|
||||
'chrome/my_feature.cc',
|
||||
['void DoSomething(content::NavigationHandle* navigation_handle) {',
|
||||
' if (navigation_handle->IsInMainFrame())',
|
||||
' all_of_our_page_state.reset();',
|
||||
'}',
|
||||
])
|
||||
self._assert_notify(
|
||||
['FromRenderFrameHost'],
|
||||
False,
|
||||
'Introduce WC::FromRenderFrameHost',
|
||||
'chrome/my_feature.cc',
|
||||
['void DoSomething(content::RenderFrameHost* rfh) {',
|
||||
' auto* wc = content::WebContents::FromRenderFrameHost(rfh);',
|
||||
' ChangeTabState(wc);',
|
||||
'}',
|
||||
])
|
||||
|
||||
def testFyi(self):
|
||||
self._assert_notify(
|
||||
[],
|
||||
True,
|
||||
'Introduce WCO and WCUD',
|
||||
'chrome/my_feature.h',
|
||||
['class MyFeature',
|
||||
' : public content::WebContentsObserver,',
|
||||
' public content::WebContentsUserData<MyFeature> {};',
|
||||
])
|
||||
self._assert_notify(
|
||||
[],
|
||||
True,
|
||||
'Introduce WCO override',
|
||||
'chrome/my_feature.h',
|
||||
['void DidFinishNavigation(',
|
||||
' content::NavigationHandle* navigation_handle) override;',
|
||||
])
|
||||
|
||||
def testNoNotify(self):
|
||||
self._assert_notify(
|
||||
[],
|
||||
False,
|
||||
'No API usage',
|
||||
'chrome/my_feature.cc',
|
||||
['void DoSomething() {',
|
||||
' // TODO: Something',
|
||||
'}',
|
||||
])
|
||||
# Something under a top level directory we're not concerned about happens
|
||||
# to share a name with a content API.
|
||||
self._assert_notify(
|
||||
[],
|
||||
False,
|
||||
'Uninteresting top level directory',
|
||||
'third_party/my_dep/my_code.cc',
|
||||
['bool HasParent(Node* node) {',
|
||||
' return node->GetParent();',
|
||||
'}',
|
||||
])
|
||||
# We're not concerned with usage in test code.
|
||||
self._assert_notify(
|
||||
[],
|
||||
False,
|
||||
'Usage in test code',
|
||||
'chrome/my_feature_unittest.cc',
|
||||
['TEST_F(MyFeatureTest, DoesSomething) {',
|
||||
' EXPECT_TRUE(rfh()->GetMainFrame());',
|
||||
'}',
|
||||
])
|
||||
|
||||
|
||||
class AssertAshOnlyCodeTest(unittest.TestCase):
|
||||
def testErrorsOnlyOnAshDirectories(self):
|
||||
files_in_ash = [
|
||||
|
||||
Reference in New Issue
Block a user