| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
feature_list.InitWithFeatures({},Shunya Shishidonit: InitAndDisableFeature(features::kThrottleSendingCustomUserTimings) is more common for such a simple case?
Done
base::test::ScopedFeatureList feature_list;Shunya Shishidoditto; InitAndEnableFeature
Or, can we just make the existing test parameterized?
I parameterized existing tests.
Shunya Shishidonit: We don't need to call clear()s for moved members?
Done. I removed other moved members calling clear().
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/page_load_metrics/renderer/page_timing_metrics_sender.cc
Insertions: 0, Deletions: 1.
@@ -374,7 +374,6 @@
last_cpu_timing_->task_time = base::TimeDelta();
modified_resources_.clear();
render_data_.new_layout_shifts.clear();
- custom_user_timings_.clear();
// As PageTimingMetricsSender is owned by MetricsRenderFrameObserver, which is
// instantiated for each frame, there's no need to make soft_navigation_count_
// zero here, as its value only increments through the lifetime of the frame.
```
```
The name of the file: components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
Insertions: 31, Deletions: 49.
@@ -82,7 +82,8 @@
} // namespace
class MetricsWebContentsObserverTest
- : public content::RenderViewHostTestHarness {
+ : public content::RenderViewHostTestHarness,
+ public testing::WithParamInterface<bool> {
public:
MetricsWebContentsObserverTest() {
mojom::PageLoadTiming timing;
@@ -287,7 +288,9 @@
raw_ptr<content::ContentBrowserClient> original_browser_client_ = nullptr;
};
-TEST_F(MetricsWebContentsObserverTest, SuccessfulMainFrameNavigation) {
+INSTANTIATE_TEST_SUITE_P(All, MetricsWebContentsObserverTest, testing::Bool());
+
+TEST_P(MetricsWebContentsObserverTest, SuccessfulMainFrameNavigation) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(1);
@@ -320,7 +323,7 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest,
+TEST_P(MetricsWebContentsObserverTest,
DISABLED_MainFrameNavigationInternalAbort) {
auto navigation = content::NavigationSimulator::CreateBrowserInitiated(
GURL(kDefaultTestUrl), web_contents());
@@ -329,7 +332,7 @@
ASSERT_EQ(kDefaultTestUrl, observed_aborted_urls().front().spec());
}
-TEST_F(MetricsWebContentsObserverTest, SubFrame) {
+TEST_P(MetricsWebContentsObserverTest, SubFrame) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(1);
@@ -381,7 +384,7 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, SameDocumentNoTrigger) {
+TEST_P(MetricsWebContentsObserverTest, SameDocumentNoTrigger) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(1);
@@ -413,7 +416,7 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, DontLogIrrelevantNavigation) {
+TEST_P(MetricsWebContentsObserverTest, DontLogIrrelevantNavigation) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(10);
@@ -438,7 +441,7 @@
CheckTotalErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, EmptyTimingError) {
+TEST_P(MetricsWebContentsObserverTest, EmptyTimingError) {
// Page load timing errors are not being reported when the error occurs for a
// page that gets preserved in the back/forward cache.
// TODO(crbug.com/40213776): Fix this.
@@ -466,7 +469,7 @@
page_load_metrics::internal::INVALID_EMPTY_TIMING, 1);
}
-TEST_F(MetricsWebContentsObserverTest, NullNavigationStartError) {
+TEST_P(MetricsWebContentsObserverTest, NullNavigationStartError) {
// Page load timing errors are not being reported when the error occurs for a
// page that gets preserved in the back/forward cache.
// TODO(crbug.com/40213776): Fix this.
@@ -495,7 +498,7 @@
page_load_metrics::internal::INVALID_NULL_NAVIGATION_START, 1);
}
-TEST_F(MetricsWebContentsObserverTest, TimingOrderError) {
+TEST_P(MetricsWebContentsObserverTest, TimingOrderError) {
// Page load timing errors are not being reported when the error occurs for a
// page that gets preserved in the back/forward cache.
// TODO(crbug.com/40213776): Fix this.
@@ -525,7 +528,7 @@
page_load_metrics::internal::INVALID_ORDER_PARSE_START_PARSE_STOP, 1);
}
-TEST_F(MetricsWebContentsObserverTest, BadIPC) {
+TEST_P(MetricsWebContentsObserverTest, BadIPC) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(10);
@@ -545,7 +548,7 @@
CheckTotalErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, ObservePartialNavigation) {
+TEST_P(MetricsWebContentsObserverTest, ObservePartialNavigation) {
// Reset the state of the tests, and attach the MetricsWebContentsObserver in
// the middle of a navigation. This tests that the class is robust to only
// observing some of a navigation.
@@ -576,7 +579,7 @@
CheckTotalErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, StopObservingOnCommit) {
+TEST_P(MetricsWebContentsObserverTest, StopObservingOnCommit) {
ASSERT_TRUE(completed_filtered_urls().empty());
content::NavigationSimulator::NavigateAndCommitFromBrowser(
@@ -601,7 +604,7 @@
completed_filtered_urls());
}
-TEST_F(MetricsWebContentsObserverTest, StopObservingOnStart) {
+TEST_P(MetricsWebContentsObserverTest, StopObservingOnStart) {
ASSERT_TRUE(completed_filtered_urls().empty());
content::NavigationSimulator::NavigateAndCommitFromBrowser(
@@ -628,7 +631,7 @@
// We buffer cross frame timings in order to provide a consistent view of
// timing data to observers. See crbug.com/722860 for more.
-TEST_F(MetricsWebContentsObserverTest, OutOfOrderCrossFrameTiming) {
+TEST_P(MetricsWebContentsObserverTest, OutOfOrderCrossFrameTiming) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromSecondsSinceUnixEpoch(1);
@@ -692,7 +695,7 @@
// We buffer cross-frame paint updates to account for paint timings from
// different frames arriving out of order.
-TEST_F(MetricsWebContentsObserverTest, OutOfOrderCrossFrameTiming2) {
+TEST_P(MetricsWebContentsObserverTest, OutOfOrderCrossFrameTiming2) {
// Dispatch a timing update for the main frame that includes a first
// paint. This should be buffered, with the dispatch timer running.
mojom::PageLoadTiming timing;
@@ -771,7 +774,7 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, FlushBufferOnAppBackground) {
+TEST_P(MetricsWebContentsObserverTest, FlushBufferOnAppBackground) {
mojom::PageLoadTiming timing;
PopulatePageLoadTiming(&timing);
timing.paint_timing->first_paint = base::Milliseconds(100000);
@@ -784,7 +787,7 @@
ASSERT_EQ(1, CountUpdatedTimingReported());
}
-TEST_F(MetricsWebContentsObserverTest,
+TEST_P(MetricsWebContentsObserverTest,
FirstInputDelayMissingFirstInputTimestamp) {
mojom::PageLoadTiming timing;
PopulatePageLoadTiming(&timing);
@@ -813,7 +816,7 @@
CheckTotalErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest,
+TEST_P(MetricsWebContentsObserverTest,
FirstInputTimestampMissingFirstInputDelay) {
mojom::PageLoadTiming timing;
PopulatePageLoadTiming(&timing);
@@ -845,7 +848,7 @@
// Main frame delivers an input notification. Subsequently, a subframe delivers
// an input notification, where the input occurred first. Verify that
// FirstInputDelay and FirstInputTimestamp come from the subframe.
-TEST_F(MetricsWebContentsObserverTest,
+TEST_P(MetricsWebContentsObserverTest,
FirstInputDelayAndTimingSubframeFirstDeliveredSecond) {
mojom::PageLoadTiming timing;
PopulatePageLoadTiming(&timing);
@@ -894,7 +897,7 @@
// A subframe delivers an input notification. Subsequently, the mainframe
// delivers an input notification, where the input occurred first. Verify that
// FirstInputDelay and FirstInputTimestamp come from the main frame.
-TEST_F(MetricsWebContentsObserverTest,
+TEST_P(MetricsWebContentsObserverTest,
FirstInputDelayAndTimingMainframeFirstDeliveredSecond) {
// We need to navigate before we can navigate the subframe.
content::NavigationSimulator::NavigateAndCommitFromBrowser(
@@ -945,7 +948,7 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, DispatchDelayedMetricsOnPageClose) {
+TEST_P(MetricsWebContentsObserverTest, DispatchDelayedMetricsOnPageClose) {
mojom::PageLoadTiming timing;
PopulatePageLoadTiming(&timing);
timing.paint_timing->first_paint = base::Milliseconds(1000);
@@ -977,7 +980,7 @@
}
// Make sure the dispatch of CPU occurs immediately.
-TEST_F(MetricsWebContentsObserverTest, DispatchCpuMetricsImmediately) {
+TEST_P(MetricsWebContentsObserverTest, DispatchCpuMetricsImmediately) {
content::NavigationSimulator::NavigateAndCommitFromBrowser(
web_contents(), GURL(kDefaultTestUrl));
@@ -997,7 +1000,7 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, OnLoadedResource_MainFrame) {
+TEST_P(MetricsWebContentsObserverTest, OnLoadedResource_MainFrame) {
GURL main_resource_url(kDefaultTestUrl);
content::NavigationSimulator::NavigateAndCommitFromBrowser(web_contents(),
main_resource_url);
@@ -1031,7 +1034,7 @@
loaded_resources().back().final_url);
}
-TEST_F(MetricsWebContentsObserverTest, OnLoadedResource_Subresource) {
+TEST_P(MetricsWebContentsObserverTest, OnLoadedResource_Subresource) {
content::NavigationSimulator::NavigateAndCommitFromBrowser(
web_contents(), GURL(kDefaultTestUrl));
GURL loaded_resource_url("http://www.other.com/");
@@ -1045,7 +1048,7 @@
loaded_resources().back().final_url);
}
-TEST_F(MetricsWebContentsObserverTest,
+TEST_P(MetricsWebContentsObserverTest,
OnLoadedResource_ResourceFromOtherRFHIgnored) {
content::NavigationSimulator::NavigateAndCommitFromBrowser(
web_contents(), GURL(kDefaultTestUrl));
@@ -1065,7 +1068,7 @@
EXPECT_TRUE(loaded_resources().empty());
}
-TEST_F(MetricsWebContentsObserverTest, RecordFeatureUsage) {
+TEST_P(MetricsWebContentsObserverTest, RecordFeatureUsage) {
content::NavigationSimulator::NavigateAndCommitFromBrowser(
web_contents(), GURL(kDefaultTestUrl));
ASSERT_EQ(main_rfh()->GetLastCommittedURL().spec(), GURL(kDefaultTestUrl));
@@ -1089,7 +1092,7 @@
EXPECT_EQ(observed_features()[1], feature2);
}
-TEST_F(MetricsWebContentsObserverTest, RecordFeatureUsageNoObserver) {
+TEST_P(MetricsWebContentsObserverTest, RecordFeatureUsageNoObserver) {
// Reset the state of the tests, and don't add an observer.
DeleteContents();
SetContents(CreateTestWebContents());
@@ -1101,10 +1104,7 @@
blink::mojom::WebFeature::kFormAttribute});
}
-TEST_F(MetricsWebContentsObserverTest, CustomUserTiming) {
- base::test::ScopedFeatureList feature_list;
- feature_list.InitWithFeatures({},
- {features::kThrottleSendingCustomUserTimings});
+TEST_P(MetricsWebContentsObserverTest, CustomUserTiming) {
content::NavigationSimulator::NavigateAndCommitFromBrowser(
web_contents(), GURL(kDefaultTestUrl));
content::RenderFrameHost* rfh = web_contents()->GetPrimaryMainFrame();
@@ -1118,24 +1118,6 @@
CheckNoErrorEvents();
}
-TEST_F(MetricsWebContentsObserverTest, CustomUserTimingWithThrottling) {
- base::test::ScopedFeatureList feature_list;
- feature_list.InitWithFeatures({features::kThrottleSendingCustomUserTimings},
- {});
- content::NavigationSimulator::NavigateAndCommitFromBrowser(
- web_contents(), GURL(kDefaultTestUrl));
- content::RenderFrameHost* rfh = web_contents()->GetPrimaryMainFrame();
-
- mojom::CustomUserTimingMark custom_timing;
- custom_timing.mark_name = "fake_custom_mark";
- custom_timing.start_time = base::Milliseconds(1000);
-
- SimulateCustomUserTimingUpdate(custom_timing, rfh);
- ASSERT_EQ(1, CountUpdatedCustomUserTimingReported());
- EXPECT_TRUE(custom_timing.Equals(*updated_custom_user_timings().back()));
- CheckNoErrorEvents();
-}
-
class MetricsWebContentsObserverBackForwardCacheTest
: public MetricsWebContentsObserverTest,
public content::WebContentsDelegate {
```
Stop sending performance timing mark with dedicated IPC
crrev.com/c/5626378 introduced `AddCustomUserTiming()` mojo call so that
the renderer immediately sends performance mark event to the browser. We
thought this is OK, as we limit sending events from the main frame only.
But now we want to send events from subframes as well. This may increase
the number of events, which directly increases the number of IPCs. To
prevent that, this CL updates the existing message to include
performance mark events, and make the IPC send timing throttled.
As a tradeoff, this may move the metrics a bit (as the send timing is
changed), but instead we can keep the performance impact minimal.
Bug: 467177770
Bundle custom user timings into existing throttled IPC
Previously, custom user timings were sent from the renderer to the
browser immediately using a dedicated Mojo call. This was acceptable
when limited only to the main frame, but extending support to subframes
risks a significant increase in IPC volume.
To address this, update the existing page load timing message to include
custom user timing marks. When the throttling feature is enabled, these
events are bundled and sent via the existing throttled IPC mechanism
instead of using the dedicated call.
While this change may result in slight shifts in reported metrics due to
the delay in transmission, it ensures that the performance impact of
reporting these events remains minimal as subframe support is
introduced.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |