Reland "[resource coordinator] TabManager discards via PageDiscardingHelper" [chromium/src : main]
Vovo Yang (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vovo Yang (Gerrit)
Vovo Yang added 1 comment![]( "Open in Gerrit")
The only difference from https://crrev.com/c/7129640 is disabling DiscardEligibilityPolicyBrowserTest.CannotDiscardVisibleInSplit on Chrome OS to avoid test flakiness on linux-chromeos-chrome, bots that aren't on the CQ.
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted Code-Review+1![]( "Open in Gerrit")
- Vovo Yang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 1 comment![]( "Open in Gerrit")
CHECK(page_node1);Missed this in my first review: it's better to use ASSERT_TRUE here, because then the test will fail cleanly instead of crashing. The flake in https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/55030/overview is a crash, which might be caused by this.
I suggest changing this to ASSERT_TRUE but leaving the test disabled. As a follow-up you could try to diagnose the flake by re-enabling the test and adding `linux-chromeos-chrome` to the CQ with `Cq-Include-Trybots` or `Include-Ci-Only-Tests` (see https://chromium.googlesource.com/chromium/src/+/main/docs/infra/cq.md)
Related details
- Vovo Yang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 1 comment![]( "Open in Gerrit")
CHECK(page_node1);Missed this in my first review: it's better to use ASSERT_TRUE here, because then the test will fail cleanly instead of crashing. The flake in https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/55030/overview is a crash, which might be caused by this.
I suggest changing this to ASSERT_TRUE but leaving the test disabled. As a follow-up you could try to diagnose the flake by re-enabling the test and adding `linux-chromeos-chrome` to the CQ with `Cq-Include-Trybots` or `Include-Ci-Only-Tests` (see https://chromium.googlesource.com/chromium/src/+/main/docs/infra/cq.md)
I added some LOG statements to the failing test and found out that it's not hitting the CHECKs. It's crashing in
```
tab_strip_model->AddToNewSplit(
{index2}, split_tabs::SplitTabVisualData(),
split_tabs::SplitTabCreatedSource::kToolbarButton);
```
Which suggests that split tabs just aren't enabled on the failing bot, or something. Regardless I'm ok to leave this disabled on ChromeOS since it seems to be a problem with the setup, not the discarding code that's being tested.
(Leaving this unresolved to be sure you see it before submitting; feel free to resolve it and submit.)
Related details
- Vovo Yang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vovo Yang (Gerrit)
Vovo Yang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vovo Yang
| Commit-Queue | +1 |
1 comment
Joe MasonMissed this in my first review: it's better to use ASSERT_TRUE here, because then the test will fail cleanly instead of crashing. The flake in https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/55030/overview is a crash, which might be caused by this.
I suggest changing this to ASSERT_TRUE but leaving the test disabled. As a follow-up you could try to diagnose the flake by re-enabling the test and adding `linux-chromeos-chrome` to the CQ with `Cq-Include-Trybots` or `Include-Ci-Only-Tests` (see https://chromium.googlesource.com/chromium/src/+/main/docs/infra/cq.md)
I added some LOG statements to the failing test and found out that it's not hitting the CHECKs. It's crashing in
```
tab_strip_model->AddToNewSplit(
{index2}, split_tabs::SplitTabVisualData(),
split_tabs::SplitTabCreatedSource::kToolbarButton);
```Which suggests that split tabs just aren't enabled on the failing bot, or something. Regardless I'm ok to leave this disabled on ChromeOS since it seems to be a problem with the setup, not the discarding code that's being tested.
(Leaving this unresolved to be sure you see it before submitting; feel free to resolve it and submit.)
Replaced CHECK() with ASSERT_TRUE(). Added TODO(crbug.com/464057202) to investigate enabling this test on Chrome OS.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vovo Yang (Gerrit)
Vovo Yang added 1 comment![]( "Open in Gerrit")
CHECK(page_node1);Joe MasonMissed this in my first review: it's better to use ASSERT_TRUE here, because then the test will fail cleanly instead of crashing. The flake in https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/55030/overview is a crash, which might be caused by this.
I suggest changing this to ASSERT_TRUE but leaving the test disabled. As a follow-up you could try to diagnose the flake by re-enabling the test and adding `linux-chromeos-chrome` to the CQ with `Cq-Include-Trybots` or `Include-Ci-Only-Tests` (see https://chromium.googlesource.com/chromium/src/+/main/docs/infra/cq.md)
Vovo YangI added some LOG statements to the failing test and found out that it's not hitting the CHECKs. It's crashing in
```
tab_strip_model->AddToNewSplit(
{index2}, split_tabs::SplitTabVisualData(),
split_tabs::SplitTabCreatedSource::kToolbarButton);
```Which suggests that split tabs just aren't enabled on the failing bot, or something. Regardless I'm ok to leave this disabled on ChromeOS since it seems to be a problem with the setup, not the discarding code that's being tested.
(Leaving this unresolved to be sure you see it before submitting; feel free to resolve it and submit.)
Replaced CHECK() with ASSERT_TRUE(). Added TODO(crbug.com/464057202) to investigate enabling this test on Chrome OS.
Thanks for your review and investigation.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vovo Yang (Gerrit)
Vovo Yang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/performance_manager/policies/discard_eligibility_policy_browsertest.cc
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
Reland "[resource coordinator] TabManager discards via PageDiscardingHelper"
This relands commit 0d54250ca6a72b86a12cc0e50d87bd355648e373.
The TabManager and TabLifecycleUnit code to select the page to discard
should be replaced by PageDiscardingHelper::DiscardAPage.
Modified some browser tests because of the difference between
TabLifecycleUnit::CanDiscard and DiscardEligibilityPolicy::CanDiscard.
Some DiscardEligibilityPolicy::CanDiscard differences:
1. It always returns eligible for external discards. So a few tests are
modified to use urgent discard instead of external discard.
2. ScopedSetClocksForTesting doesn't work. Passing
minimum_time_in_background_to_discard = 0 instead.
3. Discarding a tab with file:// url is not eligible.
Some subtests of TabLifecycleUnitTest are removed:
DiscardEligibilityPolicyTest::TestCannotDiscardVisiblePage replaces
SetFocused and SetFocusedSplit in TabLifecycleUnitTest. In
DiscardEligibilityPolicy, all visible pages are protected instead of
only the focused page.
DiscardEligibilityPolicyTest::TestCannotDiscardPageWithoutMainFrame
replaces TabLifecycleUnitTest::CannotDiscardCrashed.
DiscardEligibilityPolicy::CanDiscard returns kDisallowed for tab without
a main frame, which includes the crashed tab case.
DiscardEligibilityPolicyTest::TestCannotDiscardVisiblePage replaces
TabLifecycleUnitTest::CannotDiscardActive. In DiscardEligibilityPolicy,
all visible pages are protected.
TabLifecycleUnit::CanDiscard returns false when the discard count is not
zero. It's replaced by the DiscardAttemptMarker mechanism in
DiscardEligibilityPolicy. So TabLifecycleUnitTest::UrgentDiscardProtections
is replaced by
DiscardEligibilityPolicyTest::TestCannotDiscardPageWithDiscardAttemptMarker.
DiscardEligibilityPolicyTest::TestCannotDiscardPageCapturingVideo
replaces TabLifecycleUnitTest::CannotDiscardVideoCapture.
DiscardEligibilityPolicyTest::TestCannotDiscardPageWithFormInteractions
replaces TabLifecycleUnitTest::CannotDiscardHasFormInteractions.
DiscardEligibilityPolicyTest::TestCannotDiscardPageCapturingDisplay
replaces TabLifecycleUnitTest::CannotDiscardDesktopCapture.
DiscardEligibilityPolicyTest::TestCannotDiscardRecentlyAudiblePage
replaces TabLifecycleUnitTest::CannotDiscardRecentlyAudible.
DiscardEligibilityPolicyTest::TestCanDiscardNeverAudiblePage replaces
TabLifecycleUnitTest::CanDiscardNeverAudibleTab.
DiscardEligibilityPolicyTest::TestCannotDiscardPdf replaces
TabLifecycleUnitTest::CannotDiscardPDF.
DiscardEligibilityPolicyBrowserTest::CannotDiscardVisibleInSplit
replaces TabLifecycleUnitTest::SetFocusedSplit.
DiscardEligibilityPolicyBrowserTest::CannotDiscardVisibleInSplit is
disabled on ChromeOS build to avoid flakiness on linux-chromeos-chrome,
bots that aren't on the CQ.
- M chrome/browser/BUILD.gn
- M chrome/browser/performance_manager/policies/cannot_discard_reason.cc
- M chrome/browser/performance_manager/policies/cannot_discard_reason.h
- M chrome/browser/performance_manager/policies/discard_eligibility_policy.cc
- A chrome/browser/performance_manager/policies/discard_eligibility_policy_browsertest.cc
- M chrome/browser/performance_manager/policies/discard_eligibility_policy_unittest.cc
- M chrome/browser/performance_manager/policies/page_discarding_helper.cc
- M chrome/browser/performance_manager/policies/page_discarding_helper.h
- M chrome/browser/performance_manager/policies/page_discarding_helper_browsertest.cc
- M chrome/browser/performance_manager/policies/page_discarding_helper_unittest.cc
- M chrome/browser/performance_manager/test_support/page_discarding_utils.cc
- M chrome/browser/performance_manager/test_support/page_discarding_utils.h
- M chrome/browser/resource_coordinator/BUILD.gn
- D chrome/browser/resource_coordinator/decision_details.cc
- D chrome/browser/resource_coordinator/decision_details.h
- D chrome/browser/resource_coordinator/decision_details_unittest.cc
- M chrome/browser/resource_coordinator/lifecycle_unit.h
- M chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
- M chrome/browser/resource_coordinator/tab_lifecycle_unit.h
- M chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc
- M chrome/browser/resource_coordinator/tab_lifecycle_unit_unittest.cc
- M chrome/browser/resource_coordinator/tab_manager.cc
- M chrome/browser/resource_coordinator/tab_manager.h
- M chrome/browser/resource_coordinator/tab_manager_browsertest.cc
- M chrome/browser/resource_coordinator/tab_manager_unittest.cc
- M chrome/browser/resource_coordinator/test_lifecycle_unit.cc
- M chrome/browser/resource_coordinator/test_lifecycle_unit.h
- M chrome/test/BUILD.gn
Code-Review: +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |