Add and report LayoutStability.JankScore UKM.
We prolong the lifetime of UkmPageLoadMetricsObserver to permit jank
score reporting at session end for tabs that start or become hidden,
but avoid extra reporting of other metrics by checking was_hidden_.
The OnFinalLayoutStabilityUpdate hook is removed in favor of passing
PageRenderData to existing hooks through PageLoadExtraInfo. This lets
the observer decide when to report.
UKM collection review doc: go/lsukmrev
Bug: 581518
Change-Id: I2983dff155872f3080474e09df784d20b3cb08d3
Reviewed-on: https://chromium-review.googlesource.com/c/1299917
Commit-Queue: Steve Kobes <skobes@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606877}diff --git a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
index 72bf521..68a6999 100644
--- a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
@@ -16,6 +16,7 @@
#include "services/metrics/public/cpp/ukm_builders.h"
#include "services/metrics/public/cpp/ukm_recorder.h"
#include "services/network/public/cpp/network_quality_tracker.h"
+#include "third_party/blink/public/common/features.h"
#include "third_party/metrics_proto/system_profile.pb.h"
// static
@@ -40,8 +41,10 @@
content::NavigationHandle* navigation_handle,
const GURL& currently_committed_url,
bool started_in_foreground) {
- if (!started_in_foreground)
- return STOP_OBSERVING;
+ if (!started_in_foreground) {
+ was_hidden_ = true;
+ return CONTINUE_OBSERVING;
+ }
// When OnStart is invoked, we don't yet know whether we're observing a web
// page load, vs another kind of load (e.g. a download or a PDF). Thus,
@@ -76,23 +79,31 @@
UkmPageLoadMetricsObserver::FlushMetricsOnAppEnterBackground(
const page_load_metrics::mojom::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
- RecordPageLoadExtraInfoMetrics(info, base::TimeTicks::Now());
- RecordTimingMetrics(timing, info);
+ if (!was_hidden_) {
+ RecordPageLoadExtraInfoMetrics(info, base::TimeTicks::Now());
+ RecordTimingMetrics(timing, info);
+ }
+ ReportLayoutStability(info);
return STOP_OBSERVING;
}
UkmPageLoadMetricsObserver::ObservePolicy UkmPageLoadMetricsObserver::OnHidden(
const page_load_metrics::mojom::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
- RecordPageLoadExtraInfoMetrics(
- info, base::TimeTicks() /* no app_background_time */);
- RecordTimingMetrics(timing, info);
- return STOP_OBSERVING;
+ if (!was_hidden_) {
+ RecordPageLoadExtraInfoMetrics(
+ info, base::TimeTicks() /* no app_background_time */);
+ RecordTimingMetrics(timing, info);
+ was_hidden_ = true;
+ }
+ return CONTINUE_OBSERVING;
}
void UkmPageLoadMetricsObserver::OnFailedProvisionalLoad(
const page_load_metrics::FailedProvisionalLoadInfo& failed_load_info,
const page_load_metrics::PageLoadExtraInfo& extra_info) {
+ if (was_hidden_)
+ return;
RecordPageLoadExtraInfoMetrics(
extra_info, base::TimeTicks() /* no app_background_time */);
@@ -111,14 +122,19 @@
void UkmPageLoadMetricsObserver::OnComplete(
const page_load_metrics::mojom::PageLoadTiming& timing,
const page_load_metrics::PageLoadExtraInfo& info) {
- RecordPageLoadExtraInfoMetrics(
- info, base::TimeTicks() /* no app_background_time */);
- RecordTimingMetrics(timing, info);
+ if (!was_hidden_) {
+ RecordPageLoadExtraInfoMetrics(
+ info, base::TimeTicks() /* no app_background_time */);
+ RecordTimingMetrics(timing, info);
+ }
+ ReportLayoutStability(info);
}
void UkmPageLoadMetricsObserver::OnLoadedResource(
const page_load_metrics::ExtraRequestCompleteInfo&
extra_request_complete_info) {
+ if (was_hidden_)
+ return;
if (extra_request_complete_info.was_cached) {
cache_bytes_ += extra_request_complete_info.raw_body_bytes;
} else {
@@ -331,3 +347,20 @@
builder->SetMainFrameResource_RequestStartToReceiveHeadersEnd(
request_start_to_receive_headers_end_ms);
}
+
+void UkmPageLoadMetricsObserver::ReportLayoutStability(
+ const page_load_metrics::PageLoadExtraInfo& info) {
+ // Avoid reporting when the feature is disabled. This ensures that a report
+ // with score == 0 only occurs for a page that was actually jank-free.
+ if (!base::FeatureList::IsEnabled(blink::features::kJankTracking))
+ return;
+
+ // Report (jank_score * 100) as an int in the range [0, 1000].
+ float jank_score = info.main_frame_render_data.layout_jank_score;
+ int64_t ukm_value =
+ static_cast<int>(roundf(std::min(jank_score, 10.0f) * 100.0f));
+
+ ukm::builders::PageLoad builder(info.source_id);
+ builder.SetLayoutStability_JankScore(ukm_value);
+ builder.Record(ukm::UkmRecorder::Get());
+}
diff --git a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
index 770caf5..0d02ead2 100644
--- a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
+++ b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
@@ -78,6 +78,8 @@
// Adds main resource timing metrics to |builder|.
void ReportMainResourceTimingMetrics(ukm::builders::PageLoad* builder);
+ void ReportLayoutStability(const page_load_metrics::PageLoadExtraInfo& info);
+
// Guaranteed to be non-null during the lifetime of |this|.
network::NetworkQualityTracker* network_quality_tracker_;
@@ -100,6 +102,9 @@
// PAGE_TRANSITION_LINK is the default PageTransition value.
ui::PageTransition page_transition_ = ui::PAGE_TRANSITION_LINK;
+ // True if the page started hidden, or ever became hidden.
+ bool was_hidden_ = false;
+
DISALLOW_COPY_AND_ASSIGN(UkmPageLoadMetricsObserver);
};
diff --git a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
index 4bfc962..836ec8e 100644
--- a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
+++ b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
@@ -8,6 +8,7 @@
#include "base/metrics/metrics_hashes.h"
#include "base/optional.h"
+#include "base/test/scoped_feature_list.h"
#include "base/test/simple_test_clock.h"
#include "base/time/time.h"
#include "chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h"
@@ -21,6 +22,7 @@
#include "services/metrics/public/cpp/ukm_source.h"
#include "services/network/public/cpp/network_quality_tracker.h"
#include "testing/gmock/include/gmock/gmock.h"
+#include "third_party/blink/public/common/features.h"
#include "third_party/metrics_proto/system_profile.pb.h"
using testing::AnyNumber;
@@ -762,3 +764,34 @@
bucketed_cache_bytes);
}
}
+
+TEST_F(UkmPageLoadMetricsObserverTest, LayoutStability) {
+ base::test::ScopedFeatureList feature_list;
+ feature_list.InitAndEnableFeature(blink::features::kJankTracking);
+
+ NavigateAndCommit(GURL(kTestUrl1));
+
+ page_load_metrics::mojom::PageRenderData render_data(1.0);
+ SimulateRenderDataUpdate(render_data);
+
+ // Simulate hiding the tab (the report should include jank after hide).
+ web_contents()->WasHidden();
+
+ render_data.layout_jank_score = 2.5;
+ SimulateRenderDataUpdate(render_data);
+
+ // Simulate closing the tab.
+ DeleteContents();
+
+ const auto& ukm_recorder = test_ukm_recorder();
+ std::map<ukm::SourceId, ukm::mojom::UkmEntryPtr> merged_entries =
+ ukm_recorder.GetMergedEntriesByName(PageLoad::kEntryName);
+ EXPECT_EQ(1ul, merged_entries.size());
+
+ for (const auto& kv : merged_entries) {
+ const ukm::mojom::UkmEntry* ukm_entry = kv.second.get();
+ ukm_recorder.ExpectEntrySourceHasUrl(ukm_entry, GURL(kTestUrl1));
+ ukm_recorder.ExpectEntryMetric(
+ ukm_entry, PageLoad::kLayoutStability_JankScoreName, 250);
+ }
+}
diff --git a/chrome/browser/page_load_metrics/page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/page_load_metrics_observer.cc
index 69f7ffe..fdd4629 100644
--- a/chrome/browser/page_load_metrics/page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/page_load_metrics_observer.cc
@@ -22,6 +22,7 @@
const base::Optional<base::TimeDelta>& page_end_time,
const mojom::PageLoadMetadata& main_frame_metadata,
const mojom::PageLoadMetadata& subframe_metadata,
+ const mojom::PageRenderData& main_frame_render_data,
ukm::SourceId source_id)
: navigation_start(navigation_start),
first_background_time(first_background_time),
@@ -36,6 +37,7 @@
page_end_time(page_end_time),
main_frame_metadata(main_frame_metadata),
subframe_metadata(subframe_metadata),
+ main_frame_render_data(main_frame_render_data),
source_id(source_id) {}
PageLoadExtraInfo::PageLoadExtraInfo(const PageLoadExtraInfo& other) = default;
@@ -55,7 +57,8 @@
page_load_metrics::END_NONE,
page_load_metrics::UserInitiatedInfo::NotUserInitiated(),
base::TimeDelta(), page_load_metrics::mojom::PageLoadMetadata(),
- page_load_metrics::mojom::PageLoadMetadata(), 0 /* source_id */);
+ page_load_metrics::mojom::PageLoadMetadata(),
+ page_load_metrics::mojom::PageRenderData(), 0 /* source_id */);
}
ExtraRequestCompleteInfo::ExtraRequestCompleteInfo(
diff --git a/chrome/browser/page_load_metrics/page_load_metrics_observer.h b/chrome/browser/page_load_metrics/page_load_metrics_observer.h
index 08800244..39a13cc 100644
--- a/chrome/browser/page_load_metrics/page_load_metrics_observer.h
+++ b/chrome/browser/page_load_metrics/page_load_metrics_observer.h
@@ -136,6 +136,7 @@
const base::Optional<base::TimeDelta>& page_end_time,
const mojom::PageLoadMetadata& main_frame_metadata,
const mojom::PageLoadMetadata& subframe_metadata,
+ const mojom::PageRenderData& main_frame_render_data,
ukm::SourceId source_id);
// Simplified version of the constructor, intended for use in tests.
@@ -210,6 +211,8 @@
// PageLoadMetadata for subframes of the current page load.
const mojom::PageLoadMetadata subframe_metadata;
+ const mojom::PageRenderData main_frame_render_data;
+
// UKM SourceId for the current page load.
const ukm::SourceId source_id;
};
@@ -502,10 +505,6 @@
// Called when the event corresponding to |event_key| occurs in this page
// load.
virtual void OnEventOccurred(const void* const event_key) {}
-
- // Called when the final layout jank score for the session is ready to be
- // reported (immediately before OnComplete).
- virtual void OnFinalLayoutStabilityUpdate(float jank_score) {}
};
} // namespace page_load_metrics
diff --git a/chrome/browser/page_load_metrics/page_load_tracker.cc b/chrome/browser/page_load_metrics/page_load_tracker.cc
index 4572a02..574798a 100644
--- a/chrome/browser/page_load_metrics/page_load_tracker.cc
+++ b/chrome/browser/page_load_metrics/page_load_tracker.cc
@@ -255,9 +255,6 @@
if (failed_provisional_load_info_) {
observer->OnFailedProvisionalLoad(*failed_provisional_load_info_, info);
} else if (did_commit_) {
- observer->OnFinalLayoutStabilityUpdate(
- metrics_update_dispatcher_.main_frame_render_data()
- .layout_jank_score);
observer->OnComplete(metrics_update_dispatcher_.timing(), info);
}
}
@@ -511,7 +508,8 @@
started_in_foreground_, user_initiated_info_, url(), start_url_,
did_commit_, page_end_reason_, page_end_user_initiated_info_,
page_end_time, metrics_update_dispatcher_.main_frame_metadata(),
- metrics_update_dispatcher_.subframe_metadata(), source_id_);
+ metrics_update_dispatcher_.subframe_metadata(),
+ metrics_update_dispatcher_.main_frame_render_data(), source_id_);
}
bool PageLoadTracker::HasMatchingNavigationRequestID(
diff --git a/tools/metrics/ukm/ukm.xml b/tools/metrics/ukm/ukm.xml
index 38fb63c..40e4d7ad 100644
--- a/tools/metrics/ukm/ukm.xml
+++ b/tools/metrics/ukm/ukm.xml
@@ -3233,6 +3233,12 @@
meaningful input with longest queuing delay per navigation. In ms.
</summary>
</metric>
+ <metric name="LayoutStability.JankScore">
+ <summary>
+ Measures the amount of layout jank (bit.ly/lsm-explainer) that has
+ occurred during the session.
+ </summary>
+ </metric>
<metric name="MainFrameResource.ConnectDelay">
<summary>
The duration between the start of the connection establishment to the end
