WaaP: Fix PageLoadMetrics tests for InitialWebUI [chromium/src : main]
Eriko Kurimoto (Gerrit)
Eriko Kurimoto added 1 comment![]( "Open in Gerrit")
Related details
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted Commit-Queue+0![]( "Open in Gerrit")
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto added 1 comment![]( "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. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 6 comments![]( "Open in Gerrit")
Let me add toyoshim@ for another look. I need some thoughts on accepting/filtering metrics from WebUI/WaaP pages and UI components.
This CL fixes PageLoadMetrics integration tests and crashes that
occur when the `InitialWebUI` feature is enabled. TheIs `InitialWebUI` enabled by the field trial config? If not, we should have the entry to ensure the CL fixes the test failures.
(`chrome://webui-toolbar`) that records metrics (UKM and Histograms),I can't see this page on my Stable Chrome. Is this a correct URL?
us to keep the production code in [WebUIPageLoadMetricsObserver](cci:1://file:///usr/local/google/home/elkurin/2-chromium/src/chrome/browser/page_load_metrics/observers/webui_page_load_metrics_observer.cc:21:0-21:71)This path is your local environment, it shouldn't be part of the description.
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {Is there any way to avoid hard-coding scheme names for chrome and chrome-untrusted?
size_t valid_count = 0;
for (const auto& kv : entries) {
auto* entry = kv.second.get();
auto* source = test_ukm_recorder_->GetSourceForSourceId(entry->source_id);
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {
continue;
}
valid_count++;I'm less confident about just having this logic to filter unexpected logs here is generally a good idea. Also we should avoid avoid the code duplication.
Moreover, I'm wondering how the current UKM records events in the WebUI page. Are WebUI pages recorded to UKM normally? If so, do you have a plan to use it?
Related details
- Eriko Kurimoto
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
Just in case, do you audit what PLMOs should run for the WaaP's renderer, and others are correctly guarded at the registration steps?
IN_PROC_BROWSER_TEST_F(WebUIPageLoadMetricsObserverBrowserTest,If expectation depends on the experiment, it's better to make the test parameterized, or have an explicit ScopedFeatureList to flip the flag to a desired case? Our CIs run with several flags, and the fix will work or not work, test by test?
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {Is there any way to avoid hard-coding scheme names for chrome and chrome-untrusted?
content/public/common/url_constants.h
Related details
- Eriko Kurimoto
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Let me add toyoshim@ for another look. I need some thoughts on accepting/filtering metrics from WebUI/WaaP pages and UI components.
I want to double check if existing PLMOs mistakenly run also for the WaaP renderer, and that inject noises for existing metrics.
Maybe we want to clarify the long-term plan on PLMOs/WaaP in general, and added some short explanations in the components README? (but this does not block this change. TODO is fine)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto removed Jerome Jiang from this change![]( "Open in Gerrit")
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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto removed Mirko Bonadei from this change![]( "Open in Gerrit")
- Mirko Bonadei
Eriko Kurimoto (Gerrit)
Eriko Kurimoto removed asvitkine...@chromium.org from this change![]( "Open in Gerrit")
Related details
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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto (Gerrit)
Eriko Kurimoto added 8 comments![]( "Open in Gerrit")
Takashi ToyoshimaLet me add toyoshim@ for another look. I need some thoughts on accepting/filtering metrics from WebUI/WaaP pages and UI components.
I want to double check if existing PLMOs mistakenly run also for the WaaP renderer, and that inject noises for existing metrics.
Maybe we want to clarify the long-term plan on PLMOs/WaaP in general, and added some short explanations in the components README? (but this does not block this change. TODO is fine)
Yes, existing PLMOs (PageLoadMetricsObservers, correct?) do run for the InitialWebUI page because it is handled as a regular WebUI navigation.
This is what caused the noise and test failures we addressed in this CL (e.g., extra samples in WebUI histograms and extra UKM entries for background loads).
Just in case, do you audit what PLMOs should run for the WaaP's renderer, and others are correctly guarded at the registration steps?
Most PageLoadMetricsObserver already early-out or skip WebUI pages by default through `ShouldObserveScheme`.
`InitialWebUIPageLoadMetricsObserver` is explicitly audited to only continue observing when the source url is for initial webui.
But we have InitialWebUIPageLoadMetricsObserver which aims to collect the metrics for InitialWebUI, and that is also counted in the test since they all record the same glbal PageLoad UKM entry name.
fyi:
For UMA metrics, we have implemented generic MetricsNameMapper which intercepts and mapps subprocess metrics before they are merged into the browser process. This is filter InitialWebUI metrics out. (Note: we will map the metrics name in the future, but right now we are just dropping all of them).
We are separating UKM by the source URL based filtering.
This CL fixes PageLoadMetrics integration tests and crashes that
occur when the `InitialWebUI` feature is enabled. TheEriko KurimotoIs `InitialWebUI` enabled by the field trial config? If not, we should have the entry to ensure the CL fixes the test failures.
We don't have the filed trial config since there are many failures with Initial WebUI. We are resolving all the failures to enable the entry.
(failure list: https://chromium-review.git.corp.google.com/c/chromium/src/+/7733782)
(`chrome://webui-toolbar`) that records metrics (UKM and Histograms),I can't see this page on my Stable Chrome. Is this a correct URL?
The full url is chrome://webui-toolbar.top-chrome, and it's not enabled on stable yet.
us to keep the production code in [WebUIPageLoadMetricsObserver](cci:1://file:///usr/local/google/home/elkurin/2-chromium/src/chrome/browser/page_load_metrics/observers/webui_page_load_metrics_observer.cc:21:0-21:71)This path is your local environment, it shouldn't be part of the description.
Done
IN_PROC_BROWSER_TEST_F(WebUIPageLoadMetricsObserverBrowserTest,If expectation depends on the experiment, it's better to make the test parameterized, or have an explicit ScopedFeatureList to flip the flag to a desired case? Our CIs run with several flags, and the fix will work or not work, test by test?
sg. Let me separate the cl for webui_page_load_metrics_observer_browsertest.cc.
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {Takashi ToyoshimaIs there any way to avoid hard-coding scheme names for chrome and chrome-untrusted?
content/public/common/url_constants.h
Thanks! updated
size_t valid_count = 0;
for (const auto& kv : entries) {
auto* entry = kv.second.get();
auto* source = test_ukm_recorder_->GetSourceForSourceId(entry->source_id);
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {
continue;
}
valid_count++;I'm less confident about just having this logic to filter unexpected logs here is generally a good idea. Also we should avoid avoid the code duplication.
Moreover, I'm wondering how the current UKM records events in the WebUI page. Are WebUI pages recorded to UKM normally? If so, do you have a plan to use it?
The manual filtering is currently necessary because InitialWebUI spawns background pages during startup, which record metrics that pollute the expectations of the tests assuming a clean single-navigation state. (introduced `IsWebUISource` to avoid dup code)
For UMA histograms, we implemented [WebiumMetricsMapper](https://source.chromium.org/chromium/chromium/src/+/main:components/metrics/mapping/metrics_mapping_features.h;l=18;drc=b51c5e383220013e44e1e9e611d8ccdd1fb5b573) to prefix or drop metrics from Webium renderers based on configuration.
Currently, we are only handling metrics coming from WebUI renderer process, but we will work on the Initial WebUI related metrics which recorded in browser process as well.
Whiel many PageLoadMetricsObserver skip WebUI Pages by default, tehy can be recorded normally if opted-in. For Initial WebUI, we do have a plan to use PageLoad UKM
Related details
- Shunya Shishido
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
size_t valid_count = 0;
for (const auto& kv : entries) {
auto* entry = kv.second.get();
auto* source = test_ukm_recorder_->GetSourceForSourceId(entry->source_id);
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {
continue;
}
valid_count++;Eriko KurimotoI'm less confident about just having this logic to filter unexpected logs here is generally a good idea. Also we should avoid avoid the code duplication.
Moreover, I'm wondering how the current UKM records events in the WebUI page. Are WebUI pages recorded to UKM normally? If so, do you have a plan to use it?
The manual filtering is currently necessary because InitialWebUI spawns background pages during startup, which record metrics that pollute the expectations of the tests assuming a clean single-navigation state. (introduced `IsWebUISource` to avoid dup code)
For UMA histograms, we implemented [WebiumMetricsMapper](https://source.chromium.org/chromium/chromium/src/+/main:components/metrics/mapping/metrics_mapping_features.h;l=18;drc=b51c5e383220013e44e1e9e611d8ccdd1fb5b573) to prefix or drop metrics from Webium renderers based on configuration.
Currently, we are only handling metrics coming from WebUI renderer process, but we will work on the Initial WebUI related metrics which recorded in browser process as well.Whiel many PageLoadMetricsObserver skip WebUI Pages by default, tehy can be recorded normally if opted-in. For Initial WebUI, we do have a plan to use PageLoad UKM
So, we intentionally enable UKM for Waap here; https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/page_load_metrics_initialize.cc;l=186;drc=9a98849abd453b7472fb87b971c19162dfe3ea19;bpv=1;bpt=1
As UKM is bound with each URL, it would not make noise for existing data.
Related details
- Eriko Kurimoto
- Shunya Shishido
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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto added 1 comment![]( "Open in Gerrit")
@sisid...@chromium.org friendly ping :) If toyoshima-san's lgtm is enough, that's ok too.
Related details
- Shunya Shishido
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. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 4 comments![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
4 comments
LGTM w/ nit
This CL fixes PageLoadMetrics integration tests and crashes that
occur when the `InitialWebUI` feature is enabled. TheEriko KurimotoIs `InitialWebUI` enabled by the field trial config? If not, we should have the entry to ensure the CL fixes the test failures.
We don't have the filed trial config since there are many failures with Initial WebUI. We are resolving all the failures to enable the entry.
(failure list: https://chromium-review.git.corp.google.com/c/chromium/src/+/7733782)
In that case, how about parameterizing browser tests so that we can ensure both feature is enabled/disabled?
(`chrome://webui-toolbar`) that records metrics (UKM and Histograms),Eriko KurimotoI can't see this page on my Stable Chrome. Is this a correct URL?
The full url is chrome://webui-toolbar.top-chrome, and it's not enabled on stable yet.
Acknowledged
size_t valid_count = 0;
for (const auto& kv : entries) {
auto* entry = kv.second.get();
auto* source = test_ukm_recorder_->GetSourceForSourceId(entry->source_id);
if (!source || source->url().SchemeIs("chrome") ||
source->url().SchemeIs("chrome-untrusted")) {
continue;
}
valid_count++;Eriko KurimotoI'm less confident about just having this logic to filter unexpected logs here is generally a good idea. Also we should avoid avoid the code duplication.
Moreover, I'm wondering how the current UKM records events in the WebUI page. Are WebUI pages recorded to UKM normally? If so, do you have a plan to use it?
Takashi ToyoshimaThe manual filtering is currently necessary because InitialWebUI spawns background pages during startup, which record metrics that pollute the expectations of the tests assuming a clean single-navigation state. (introduced `IsWebUISource` to avoid dup code)
For UMA histograms, we implemented [WebiumMetricsMapper](https://source.chromium.org/chromium/chromium/src/+/main:components/metrics/mapping/metrics_mapping_features.h;l=18;drc=b51c5e383220013e44e1e9e611d8ccdd1fb5b573) to prefix or drop metrics from Webium renderers based on configuration.
Currently, we are only handling metrics coming from WebUI renderer process, but we will work on the Initial WebUI related metrics which recorded in browser process as well.Whiel many PageLoadMetricsObserver skip WebUI Pages by default, tehy can be recorded normally if opted-in. For Initial WebUI, we do have a plan to use PageLoad UKM
So, we intentionally enable UKM for Waap here; https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/page_load_metrics_initialize.cc;l=186;drc=9a98849abd453b7472fb87b971c19162dfe3ea19;bpv=1;bpt=1
As UKM is bound with each URL, it would not make noise for existing data.
- Eriko Kurimoto
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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eriko Kurimoto
| Commit-Queue | +2 |
1 comment
This CL fixes PageLoadMetrics integration tests and crashes that
occur when the `InitialWebUI` feature is enabled. TheEriko KurimotoIs `InitialWebUI` enabled by the field trial config? If not, we should have the entry to ensure the CL fixes the test failures.
Shunya ShishidoWe don't have the filed trial config since there are many failures with Initial WebUI. We are resolving all the failures to enable the entry.
(failure list: https://chromium-review.git.corp.google.com/c/chromium/src/+/7733782)
In that case, how about parameterizing browser tests so that we can ensure both feature is enabled/disabled?
OK.
We will enable the field trial this or next week if things go smoothly, so will delete the parameter later.
Related details
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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto voted and added 2 comments![]( "Open in Gerrit")
Votes added by Eriko Kurimoto
| Commit-Queue | +2 |
Takashi ToyoshimaLet me add toyoshim@ for another look. I need some thoughts on accepting/filtering metrics from WebUI/WaaP pages and UI components.
Eriko KurimotoI want to double check if existing PLMOs mistakenly run also for the WaaP renderer, and that inject noises for existing metrics.
Maybe we want to clarify the long-term plan on PLMOs/WaaP in general, and added some short explanations in the components README? (but this does not block this change. TODO is fine)
Yes, existing PLMOs (PageLoadMetricsObservers, correct?) do run for the InitialWebUI page because it is handled as a regular WebUI navigation.
This is what caused the noise and test failures we addressed in this CL (e.g., extra samples in WebUI histograms and extra UKM entries for background loads).
Acknowledged
Eriko KurimotoJust in case, do you audit what PLMOs should run for the WaaP's renderer, and others are correctly guarded at the registration steps?
Most PageLoadMetricsObserver already early-out or skip WebUI pages by default through `ShouldObserveScheme`.
`InitialWebUIPageLoadMetricsObserver` is explicitly audited to only continue observing when the source url is for initial webui.But we have InitialWebUIPageLoadMetricsObserver which aims to collect the metrics for InitialWebUI, and that is also counted in the test since they all record the same glbal PageLoad UKM entry name.
fyi:
For UMA metrics, we have implemented generic MetricsNameMapper which intercepts and mapps subprocess metrics before they are merged into the browser process. This is filter InitialWebUI metrics out. (Note: we will map the metrics name in the future, but right now we are just dropping all of them).We are separating UKM by the source URL based filtering.
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto 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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto 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. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto 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-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
chromiu...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
Insertions: 71, Deletions: 27.
@@ -1165,7 +1165,26 @@
VerifyNavigationMetrics({url});
}
-IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, CachedPage) {
+class PageLoadMetricsBrowserTestWithInitialWebUIParam
+ : public PageLoadMetricsBrowserTest,
+ public ::testing::WithParamInterface<bool> {
+ public:
+ PageLoadMetricsBrowserTestWithInitialWebUIParam() {
+ if (GetParam()) {
+ sub_feature_list_.InitWithFeatures(
+ {features::kInitialWebUI, features::kWebUIReloadButton}, {});
+ } else {
+ sub_feature_list_.InitWithFeatures(
+ {}, {features::kInitialWebUI, features::kWebUIReloadButton});
+ }
+ }
+
+ private:
+ base::test::ScopedFeatureList sub_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(PageLoadMetricsBrowserTestWithInitialWebUIParam,
+ CachedPage) {
ASSERT_TRUE(embedded_test_server()->Start());
GURL url = embedded_test_server()->GetURL(kCacheablePathPrefix);
@@ -1176,14 +1195,14 @@
auto entries =
test_ukm_recorder_->GetMergedEntriesByName(PageLoad::kEntryName);
- size_t expected_entries = 1;
- // TODO(crbug.com/501008370): Clarify how to handle global PageLoad UKM entries
- // recorded for InitialWebUI in tests. Currently they pollute generic tests
- // that check the total count of PageLoad entries.
- if (base::FeatureList::IsEnabled(features::kInitialWebUI)) {
- expected_entries = 2;
+ // TODO(crbug.com/501008370): Clarify how to handle global PageLoad UKM
+ // entries recorded for InitialWebUI in tests. Currently they pollute
+ // generic tests that check the total count of PageLoad entries.
+ if (GetParam()) {
+ EXPECT_GE(entries.size(), 1u);
+ } else {
+ EXPECT_EQ(1u, entries.size());
}
- EXPECT_EQ(expected_entries, entries.size());
for (const auto& kv : entries) {
EXPECT_FALSE(test_ukm_recorder_->EntryHasMetric(kv.second.get(),
@@ -1220,9 +1239,14 @@
VerifyNavigationMetrics({url});
}
+INSTANTIATE_TEST_SUITE_P(All,
+ PageLoadMetricsBrowserTestWithInitialWebUIParam,
+ ::testing::Bool());
+
// Test that we log kMainFrameResource_RequestHasNoStore when response has
// cache-control:no-store response header.
-IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, MainFrameHasNoStore) {
+IN_PROC_BROWSER_TEST_P(PageLoadMetricsBrowserTestWithInitialWebUIParam,
+ MainFrameHasNoStore) {
// Create a HTTP response to control main-frame navigation to send no-store
// response.
net::test_server::ControllableHttpResponse response(embedded_test_server(),
@@ -1254,11 +1278,11 @@
auto entries =
test_ukm_recorder_->GetMergedEntriesByName(PageLoad::kEntryName);
- size_t expected_entries = 1;
- if (base::FeatureList::IsEnabled(features::kInitialWebUI)) {
- expected_entries = 2;
+ if (GetParam()) {
+ EXPECT_GE(entries.size(), 1u);
+ } else {
+ EXPECT_EQ(1u, entries.size());
}
- EXPECT_EQ(expected_entries, entries.size());
size_t count = 0;
for (const auto& kv : entries) {
@@ -1274,7 +1298,7 @@
// Test that we set kMainFrameResource_RequestHasNoStore to false when response
// has no cache-control:no-store header.
-IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
+IN_PROC_BROWSER_TEST_P(PageLoadMetricsBrowserTestWithInitialWebUIParam,
MainFrameDoesnotHaveNoStore) {
ASSERT_TRUE(embedded_test_server()->Start());
@@ -1287,11 +1311,11 @@
auto entries =
test_ukm_recorder_->GetMergedEntriesByName(PageLoad::kEntryName);
- size_t expected_entries = 1;
- if (base::FeatureList::IsEnabled(features::kInitialWebUI)) {
- expected_entries = 2;
+ if (GetParam()) {
+ EXPECT_GE(entries.size(), 1u);
+ } else {
+ EXPECT_EQ(1u, entries.size());
}
- EXPECT_EQ(expected_entries, entries.size());
size_t count = 0;
for (const auto& kv : entries) {
@@ -4669,6 +4693,24 @@
content::test::PrerenderTestHelper prerender_helper_;
};
+class PrerenderPageLoadMetricsBrowserTestWithInitialWebUIParam
+ : public PrerenderPageLoadMetricsBrowserTest,
+ public ::testing::WithParamInterface<bool> {
+ public:
+ PrerenderPageLoadMetricsBrowserTestWithInitialWebUIParam() {
+ if (GetParam()) {
+ sub_feature_list_.InitWithFeatures(
+ {features::kInitialWebUI, features::kWebUIReloadButton}, {});
+ } else {
+ sub_feature_list_.InitWithFeatures(
+ {}, {features::kInitialWebUI, features::kWebUIReloadButton});
+ }
+ }
+
+ private:
+ base::test::ScopedFeatureList sub_feature_list_;
+};
+
IN_PROC_BROWSER_TEST_F(PrerenderPageLoadMetricsBrowserTest, PrerenderEvent) {
using page_load_metrics::internal::kPageLoadPrerender2Event;
using page_load_metrics::internal::PageLoadPrerenderEvent;
@@ -4708,7 +4750,7 @@
PageLoadPrerenderEvent::kPrerenderActivationNavigation, 1);
}
-IN_PROC_BROWSER_TEST_F(PrerenderPageLoadMetricsBrowserTest,
+IN_PROC_BROWSER_TEST_P(PrerenderPageLoadMetricsBrowserTestWithInitialWebUIParam,
PrerenderingDoNotRecordUKM) {
ASSERT_TRUE(embedded_test_server()->Start());
@@ -4724,23 +4766,25 @@
EXPECT_FALSE(host_observer.was_activated());
auto entries =
test_ukm_recorder_->GetMergedEntriesByName(PageLoad::kEntryName);
- size_t expected_entries = 0;
- if (base::FeatureList::IsEnabled(features::kInitialWebUI)) {
- expected_entries = 1;
- }
+ size_t expected_entries = GetParam() ? 1 : 0;
EXPECT_EQ(expected_entries, entries.size());
// Activate.
prerender_helper_.NavigatePrimaryPage(prerender_url);
EXPECT_TRUE(host_observer.was_activated());
entries = test_ukm_recorder_->GetMergedEntriesByName(PageLoad::kEntryName);
- expected_entries = 1;
- if (base::FeatureList::IsEnabled(features::kInitialWebUI)) {
- expected_entries = 2;
+ if (GetParam()) {
+ EXPECT_GE(entries.size(), 1u);
+ } else {
+ EXPECT_EQ(1u, entries.size());
}
- EXPECT_EQ(expected_entries, entries.size());
}
+INSTANTIATE_TEST_SUITE_P(
+ All,
+ PrerenderPageLoadMetricsBrowserTestWithInitialWebUIParam,
+ ::testing::Bool());
+
enum BackForwardCacheStatus { kDisabled = 0, kEnabled = 1 };
class PageLoadMetricsBackForwardCacheBrowserTest
```
Change information
WaaP: Fix PageLoadMetrics tests for InitialWebUI
This CL fixes PageLoadMetrics integration tests and crashes that
occur when the `InitialWebUI` feature is enabled.
When InitialWebUI is enabled, WebUI toolbar is loaded and it records
metrics that pollutes the test expectations in tests which assume only
the test-initizated navigation occurs.
- M chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
Code-Review: +1 by Takashi Toyoshima, +1 by Shunya Shishido
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |