[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. |
Joe Mason (Gerrit)
Joe Mason added 6 comments![]( "Open in Gerrit")
I'm very excited to reduce the duplication!
I made a first pass at this. I still want to double-check the logic in TabLifecycleUnit::CanDiscard and make sure there are no important checks there missing from DiscardEligibilityPolicy, and go through the deleted tests and make sure there's test coverage for everything that's being removed.
ExpectCanDiscardFalseTrivialAllReasons(first_lifecycle_unit);I think we still want to check that DiscardEligibilityPolicy::CanDiscard returns false here. Or else call Dicard and verify that it doesn't actually get discarded (as long as that's not expected to crash when CanDiscard returns false.)
// Insert the tab into the second tab strip without focusing it. Verify thatNit: I think this comment should be kept, since it helps understand what the rest of the test is doing.
TEST_F(TabLifecycleUnitTest, CanDiscardByDefault) {Why delete this test?
ExpectCanDiscardTrueAllReasons(&tab_lifecycle_unit);I think we should be testing DiscardEligibilityPolicy::CanDiscard at all these points, if we're keeping this test at all.
// |reason|. Exposes |minimum_time_in_background_to_discard| so test can setNit: "tests"
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 added 1 comment![]( "Open in Gerrit")
ExpectCanDiscardFalseTrivialAllReasons(first_lifecycle_unit);I think we still want to check that DiscardEligibilityPolicy::CanDiscard returns false here. Or else call Dicard and verify that it doesn't actually get discarded (as long as that's not expected to crash when CanDiscard returns false.)
Ok, I am working on adding back ExpectCanDiscard* functions and adding back some of the tests.
Related details
- Joe Mason
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 added 5 comments![]( "Open in Gerrit")
ExpectCanDiscardFalseTrivialAllReasons(first_lifecycle_unit);Vovo YangI think we still want to check that DiscardEligibilityPolicy::CanDiscard returns false here. Or else call Dicard and verify that it doesn't actually get discarded (as long as that's not expected to crash when CanDiscard returns false.)
Ok, I am working on adding back ExpectCanDiscard* functions and adding back some of the tests.
Adds check to verify Discard() returns false when the tab is detached. Adds check to verify Discard() returns true when the tab is reattached.
// Insert the tab into the second tab strip without focusing it. Verify thatNit: I think this comment should be kept, since it helps understand what the rest of the test is doing.
Done
TEST_F(TabLifecycleUnitTest, CanDiscardByDefault) {Vovo YangWhy delete this test?
Added back CanDiscardByDefault.
ExpectCanDiscardTrueAllReasons(&tab_lifecycle_unit);I think we should be testing DiscardEligibilityPolicy::CanDiscard at all these points, if we're keeping this test at all.
Added comments to explain why SetFocused and some other unit tests are removed.
// |reason|. Exposes |minimum_time_in_background_to_discard| so test can setVovo YangNit: "tests"
Done
Related details
- Joe Mason
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 added 1 comment![]( "Open in Gerrit")
ExpectCanDiscardTrueAllReasons(&tab_lifecycle_unit);Vovo YangI think we should be testing DiscardEligibilityPolicy::CanDiscard at all these points, if we're keeping this test at all.
Added comments to explain why SetFocused and some other unit tests are removed.
Updated the commit message to explain the reason to remove some unit tests.
Related details
- Joe Mason
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 2 comments![]( "Open in Gerrit")
I made a first pass at this. I still want to double-check the logic in TabLifecycleUnit::CanDiscard and make sure there are no important checks there missing from DiscardEligibilityPolicy, and go through the deleted tests and make sure there's test coverage for everything that's being removed.
Found one missed check in CanDiscard. Still need to check the tests.
if (tab_helper && tab_helper->is_in_app_window()) {This check isn't in DiscardEligibilityPolicy::CanDiscard - it should be added there.
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 5 comments![]( "Open in Gerrit")
Joe MasonI'm very excited to reduce the duplication!
I made a first pass at this. I still want to double-check the logic in TabLifecycleUnit::CanDiscard and make sure there are no important checks there missing from DiscardEligibilityPolicy, and go through the deleted tests and make sure there's test coverage for everything that's being removed.
Found one missed check in CanDiscard. Still need to check the tests.
Looks good except for the WebAppTabHelper check and the few tests that I left "note to self" comments on.
TEST_F(TabLifecycleUnitTest, SetFocused) {Note to self: check tests in this file.
ProtectRecentlyUsedTabsFromUrgentDiscarding) {Note to self: check this.
performance_manager::PerformanceManager::GetGraph();Nit: you can just use `page_node->GetGraph()` (which never returns null)
EXPECT_TRUE(find_result != failure_reasons.end());Nit: prefer `EXPECT_TRUE(base::Contains(failure_reasons, failure_reason))`.
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 added 4 comments![]( "Open in Gerrit")
Joe MasonI'm very excited to reduce the duplication!
I made a first pass at this. I still want to double-check the logic in TabLifecycleUnit::CanDiscard and make sure there are no important checks there missing from DiscardEligibilityPolicy, and go through the deleted tests and make sure there's test coverage for everything that's being removed.
Joe MasonFound one missed check in CanDiscard. Still need to check the tests.
Looks good except for the WebAppTabHelper check and the few tests that I left "note to self" comments on.
Added the WebAppTabHelper check in DiscardEligibilityPolicy::CanDiscard().
if (tab_helper && tab_helper->is_in_app_window()) {This check isn't in DiscardEligibilityPolicy::CanDiscard - it should be added there.
Added the web app check to DiscardEligibilityPolicy::CanDiscard().
Nit: you can just use `page_node->GetGraph()` (which never returns null)
Done
Nit: prefer `EXPECT_TRUE(base::Contains(failure_reasons, failure_reason))`.
- Joe Mason
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 11 comments![]( "Open in Gerrit")
#include "testing/gtest/include/gtest/gtest.h"Nit: please include the following headers for IWYU:
```
<string>
<string_view>
<vector>
base/memory/weak_ptr.h
base/types/expected.h
chrome/browser/performance_manager/policies/cannot_discard_reason.h
chrome/browser/profiles/profile.h
chrome/browser/ui/browser.h
chrome/browser/ui/tabs/tab_strip_model.h
components/performance_manager/public/graph/graph.h
components/performance_manager/public/performance_manager.h
components/webapps/common/web_app_id.h
url/gurl.h
content/public/browser/web_contents.h
```
ASSERT_EQ(apps::test::EnableLinkCapturingByUser(profile(), app_id),I recently had to fix quite a few flakes in WebApp tests that don't wait for all the different async steps, but AFAICT this test is doing everything right. (I didn't know about the EnableLinkCapturingByUser helper function - it would have saved me a lot of time.) Nice job!
// Wait for & get the web contents that was loaded.Nit: I keep thinking the & here means "pointer". "...and get" would be clearer.
Also I think this comment should go above the UrlLoadObserver?
TEST_F(TabLifecycleUnitTest, SetFocused) {Note to self: check tests in this file.
Done
TEST_F(TabLifecycleUnitTest, SetFocused) {Confirmed: this is exercised by `PageDiscardingHelperBrowserTest.DiscardTabsWithOccludedWindow`
TEST_F(TabLifecycleUnitTest, SetFocusedSplit) {I think we need to move this test into discard_eligibility_policy_unittest.cc or page_discarding_helper_unittest.cc/browsertest.cc.
DiscardEligibilityPolicy doesn't include explicit split tab handling so we need test coverage.
TEST_F(TabLifecycleUnitTest, CannotDiscardCrashed) {We still need a test for this. DiscardEligibilityPolicy doesn't explicitly check for crashed tabs, but a comment says "Don't discard tabs that don't have a main frame (..., crashed tab)." But we need test coverage to be sure the comment is correct.
(Discarding a "crashed tab" would only be a minor regression, I think, so this could be a followup.)
TEST_F(TabLifecycleUnitTest, CannotDiscardActive) {Exercised by `PageDiscardingHelperBrowserTest.DiscardTabsWithOccludedWindow`.
LifecycleUnitDiscardReason::FROZEN_WITH_GROWING_MEMORY);Please add `FROZEN_WITH_GROWING_MEMORY` tests to chrome/browser/performance_manager/policies/discard_eligibility_policy_unittest.cc (this specific one is covered by `DiscardEligibilityPolicyTest.TestCannotDiscardRecentlyVisiblePageUnlessExplicitlyRequested`)
TEST_F(TabLifecycleUnitTest, CannotDiscardVideoCapture) {The rest of these all have obvious counterparts in `DiscardEligibilityPolicyTest`
ProtectRecentlyUsedTabsFromUrgentDiscarding) {Note to self: check this.
I can't actually find any end-to-end browsertest that checks minimum time in background, but there's plenty of end-to-end coverage of foreground vs. background tabs. (eg. `PageDiscardingHelperBrowserTest.DiscardTabsWithOccludedWindow`) and there are unit tests. So losing this is probably fine.
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 5 comments![]( "Open in Gerrit")
Votes added by Vovo Yang
| Commit-Queue | +1 |
5 comments
Nit: please include the following headers for IWYU:
```
<string>
<string_view>
<vector>base/memory/weak_ptr.h
base/types/expected.h
chrome/browser/performance_manager/policies/cannot_discard_reason.h
chrome/browser/profiles/profile.h
chrome/browser/ui/browser.h
chrome/browser/ui/tabs/tab_strip_model.h
components/performance_manager/public/graph/graph.h
components/performance_manager/public/performance_manager.h
components/webapps/common/web_app_id.h
url/gurl.h
content/public/browser/web_contents.h
```
Done
Nit: I keep thinking the & here means "pointer". "...and get" would be clearer.
Also I think this comment should go above the UrlLoadObserver?
Done
TEST_F(TabLifecycleUnitTest, SetFocusedSplit) {I think we need to move this test into discard_eligibility_policy_unittest.cc or page_discarding_helper_unittest.cc/browsertest.cc.
DiscardEligibilityPolicy doesn't include explicit split tab handling so we need test coverage.
Added test DiscardEligibilityPolicyBrowserTest::CannotDiscardVisibleInSplit to
replaces TabLifecycleUnitTest::SetFocusedSplit.
TEST_F(TabLifecycleUnitTest, CannotDiscardCrashed) {We still need a test for this. DiscardEligibilityPolicy doesn't explicitly check for crashed tabs, but a comment says "Don't discard tabs that don't have a main frame (..., crashed tab)." But we need test coverage to be sure the comment is correct.
(Discarding a "crashed tab" would only be a minor regression, I think, so this could be a followup.)
This CL is already quite large. I think it's better to add the test in another CL. I added a TODO(crbug.com/463291982) in chrome/browser/performance_manager/policies/discard_eligibility_policy.cc comments.
LifecycleUnitDiscardReason::FROZEN_WITH_GROWING_MEMORY);Please add `FROZEN_WITH_GROWING_MEMORY` tests to chrome/browser/performance_manager/policies/discard_eligibility_policy_unittest.cc (this specific one is covered by `DiscardEligibilityPolicyTest.TestCannotDiscardRecentlyVisiblePageUnlessExplicitlyRequested`)
Added FROZEN_WITH_GROWING_MEMORY and SUGGESTED checks in chrome/browser/performance_manager/policies/discard_eligibility_policy_unittest.cc.
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 and added 1 comment![]( "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. |
Vovo Yang (Gerrit)
Vovo Yang voted Commit-Queue+2![]( "Open in Gerrit")
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
26 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/resource_coordinator/tab_manager_browsertest.cc
Insertions: 50, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
[resource coordinator] TabManager discards via PageDiscardingHelper
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.
- 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. |
Vovo Yang (Gerrit)
Vovo Yang added 1 comment![]( "Open in Gerrit")
Thanks for all the hard work on this!
Thanks for your review and catching the missing logic in DiscardEligibilityPolicy::CanDiscard().
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lingqi Chi (Gerrit)
Lingqi Chi has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |