Enable PageDiscardingHelper on Android [chromium/src : main]
Vovo Yang (Gerrit)
Vovo Yang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted and added 13 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
13 comments
performance_manager::policies::PageDiscardingHelper>());Nit: it's hard to keep track of the same object created in two spots in the same file. How about moving the existing PageDiscardingHelper outside the #if and changing it to something like:
```
#if BUILDFLAG(IS_ANDROID)
const bool need_page_discarding_helper =
base::FeatureList::IsEnabled(features::kWebContentsDiscard);
#else
const bool need_page_discarding_helper = true;
#endif
if (need_page_discarding_helper) {
graph->PassToGraph(...);
}
```
#include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom-forward.h"Nit: this isn't needed because page_discarder.h file already includes `lifecycle_unit_state.mojom.h`.
Technically the .h should include `lifecycle_unit_state.mojom-forward.h` and the .cc should include `lifecycle_unit_state.mojom.h` so if you want to make that cleanup while you're here, feel free.
using ::mojom::LifecycleUnitDiscardReason;Nit: instead of this, please just change `PageDiscarder::DiscardPageNode` to use `::mojom::LifecycleUnitDiscardReason`. The header file already uses the mojom symbol, leaving it in the .cc file was an oversight.
if (contents && base::FeatureList::IsEnabled(features::kWebContentsDiscard)) {Nit: can it actually get here with out the feature enabled? If not, I'd CHECK this before the `if` instead of making it part of the condition.
Actually this whole `if` isn't needed because there's an `if (!contents)` block just above that does the exact same thing.
Also the `memory_footprint_estimate` is the same between the Android and non-Android branches, and (a) it's not that expensive to calculate, (b) null `lifecycle_unit` should be rare, or even impossible.
So I would structure this as:
```
CHECK(contents, base::NotFatalUntil::M140);
if (!contents) {
outcome = DiscardPageOnUIThreadOutcome::kNoContents;
return std::nullopt;
}
base::ByteCount memory_footprint_estimate =
user_tuning::GetDiscardedMemoryEstimateForPage(page_node);
#if BUILDFLAG(IS_ANDROID)
CHECK(base::FeatureList::IsEnabled(features::kWebContentsDiscard));
resource_coordinator::AttemptFastKillForDiscard(contents.get(),
discard_reason);
contents->Discard(base::NullCallback());
#else
auto* lifecycle_unit =
resource_coordinator::TabLifecycleUnitSource::GetTabLifecycleUnitExternal(
contents.get());
if (!lifecycle_unit) {
return std::nullopt;
}
if (!lifecycle_unit->DiscardTab(discard_reason,
memory_footprint_estimate.InKiB())) {
outcome = DiscardPageOnUIThreadOutcome::kDiscardTabFailure;
return std::nullopt;
}
#endif
outcome = DiscardPageOnUIThreadOutcome::kSuccess;
return memory_footprint_estimate;
}
```
void AttemptFastKillForDiscard(content::WebContents* web_contents,Nice cleanup! This has no reason to be in this class.
#include "content/public/browser/render_frame_host.h"Nit: this is also unused now.
// This function is exposed for performance manager PageDiscarder in AndroidNit: I think this comment isn't needed. This is a good place for this function in all cases.
#include "chrome/browser/resource_coordinator/lifecycle_unit.h"Nit: to avoid another dependency on lifecycle_unit.h just for the typedef, I'd prefer to include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom-forward.h" and use `::mojom::LifecycleUnitDiscardReason` in this file. (I'm gradually migrating away from lifecycle_unit.h.)
If you do that, include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom.h" in the .cc file to get the full enum definition.
#include <stdint.h>Nit: this include should be in the .cc file.
content::RenderFrameHost* main_frame = web_contents->GetPrimaryMainFrame();Nit: please #include "content/public/browser/render_frame_host.h" for IWYU.
CHECK(main_frame);Nit: please #include "base/check.h" for IWYU.
#if BUILDFLAG(IS_CHROMEOS)Nit: please #include "build/build_config.h" for IWYU.
Related details
- Vovo Yang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vovo Yang (Gerrit)
Vovo Yang added 13 comments![]( "Open in Gerrit")
Nit: it's hard to keep track of the same object created in two spots in the same file. How about moving the existing PageDiscardingHelper outside the #if and changing it to something like:
```
#if BUILDFLAG(IS_ANDROID)
const bool need_page_discarding_helper =
base::FeatureList::IsEnabled(features::kWebContentsDiscard);
#else
const bool need_page_discarding_helper = true;
#endif
if (need_page_discarding_helper) {
graph->PassToGraph(...);
}
```
Done
#include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom-forward.h"Nit: this isn't needed because page_discarder.h file already includes `lifecycle_unit_state.mojom.h`.
Technically the .h should include `lifecycle_unit_state.mojom-forward.h` and the .cc should include `lifecycle_unit_state.mojom.h` so if you want to make that cleanup while you're here, feel free.
Done
Nit: instead of this, please just change `PageDiscarder::DiscardPageNode` to use `::mojom::LifecycleUnitDiscardReason`. The header file already uses the mojom symbol, leaving it in the .cc file was an oversight.
Done
if (contents && base::FeatureList::IsEnabled(features::kWebContentsDiscard)) {Done
Nit: this is also unused now.
Done
// This function is exposed for performance manager PageDiscarder in AndroidNit: I think this comment isn't needed. This is a good place for this function in all cases.
Done
#include "chrome/browser/resource_coordinator/lifecycle_unit.h"Nit: to avoid another dependency on lifecycle_unit.h just for the typedef, I'd prefer to include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom-forward.h" and use `::mojom::LifecycleUnitDiscardReason` in this file. (I'm gradually migrating away from lifecycle_unit.h.)
If you do that, include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom.h" in the .cc file to get the full enum definition.
Done
Nit: this include should be in the .cc file.
Removed unused include.
content::RenderFrameHost* main_frame = web_contents->GetPrimaryMainFrame();Nit: please #include "content/public/browser/render_frame_host.h" for IWYU.
Done
Nit: please #include "base/check.h" for IWYU.
Done
Nit: please #include "build/build_config.h" for IWYU.
Done
return;This is necessary to ensure PageDiscarder::DiscardPageNode() is not called when kWebContentsDiscard is disabled.
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
| 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")
Votes added by Joe Mason
| Code-Review | +1 |
This is necessary to ensure PageDiscarder::DiscardPageNode() is not called when kWebContentsDiscard is disabled.
Ack. It would be great to remove the "Discard" button when it's disabled, but this is just a testing UI so it's not a big deal. That can be another CL if you think it's worth it.
Related details
- Vovo Yang
Code-Coverage
Code-Owners
Code-Review
| 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
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Enable PageDiscardingHelper on Android
Only enabled when feature WebContentsDiscard is enabled.
PageDiscardingHelper is only used by chrome://discards on Android.
NO_IFTTT=move LINT.IfChange block to another file
- M chrome/browser/BUILD.gn
- M chrome/browser/performance_manager/chrome_browser_main_extra_parts_performance_manager.cc
- M chrome/browser/performance_manager/mechanisms/page_discarder.cc
- M chrome/browser/performance_manager/mechanisms/page_discarder.h
- M chrome/browser/performance_manager/user_tuning/user_tuning_utils.cc
- M chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
- M chrome/browser/resource_coordinator/tab_lifecycle_unit.h
- M chrome/browser/resource_coordinator/utils.cc
- M chrome/browser/resource_coordinator/utils.h
- M chrome/browser/resources/discards/discards_tab.html.ts
- M chrome/browser/resources/discards/discards_tab.ts
- M chrome/browser/ui/webui/discards/discards_ui.cc
- M tools/metrics/histograms/metadata/tab/enums.xml
Code-Review: +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |