Skip to content

Commit

Permalink
Set receive_header_start only for main frame document
Browse files Browse the repository at this point in the history
This is to fix a bug in a previous CL.
https://chromium-review.googlesource.com/c/chromium/src/+/4476487. The
receive_header_start_ of the PageLoadTracker object is set for each
resource. We want the field to hold the value of the main frame document
only.

(cherry picked from commit b682b61)

Bug: 1431906
Change-Id: Ifc76f40c1790f626b9aacbece785010c4d6e0710
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4562797
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Hao Liu <haoliuk@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1150323}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575844
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5790@{#188}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
haoliuk authored and Chromium LUCI CQ committed May 31, 2023
1 parent 22f78e7 commit bd1eb06
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 6 deletions.
16 changes: 12 additions & 4 deletions components/page_load_metrics/browser/page_load_tracker.cc
Expand Up @@ -691,8 +691,15 @@ void PageLoadTracker::FlushMetricsOnAppEnterBackground() {

void PageLoadTracker::OnLoadedResource(
const ExtraRequestCompleteInfo& extra_request_complete_info) {
receive_headers_start_ =
extra_request_complete_info.load_timing_info->receive_headers_start;
// The main_frame_receive_headers_start_ should be only set once during a
// page load. A new page load would have a new PageLoadTracker object.
if (extra_request_complete_info.request_destination ==
network::mojom::RequestDestination::kDocument &&
!main_frame_receive_headers_start_.has_value()) {
main_frame_receive_headers_start_ =
extra_request_complete_info.load_timing_info->receive_headers_start;
}

for (const auto& observer : observers_) {
observer->OnLoadedResource(extra_request_complete_info);
}
Expand Down Expand Up @@ -938,10 +945,11 @@ void PageLoadTracker::OnTimingChanged() {
.value());

if (largest_contentful_image_changed) {
if (receive_headers_start_.has_value() && !GetNavigationStart().is_null()) {
if (main_frame_receive_headers_start_.has_value() &&
!GetNavigationStart().is_null()) {
RecordLargestContentfulPaintImageLoadTiming(
*paint_timing->largest_contentful_paint,
receive_headers_start_.value() - GetNavigationStart());
main_frame_receive_headers_start_.value() - GetNavigationStart());
}
}

Expand Down
2 changes: 1 addition & 1 deletion components/page_load_metrics/browser/page_load_tracker.h
Expand Up @@ -572,7 +572,7 @@ class PageLoadTracker : public PageLoadMetricsUpdateDispatcher::Client,

uint32_t soft_navigation_count_ = 0;

absl::optional<base::TimeTicks> receive_headers_start_;
absl::optional<base::TimeTicks> main_frame_receive_headers_start_;

const internal::PageLoadTrackerPageType page_type_;

Expand Down
Expand Up @@ -494,10 +494,30 @@ TEST_F(PageLoadTrackerTest, LargestImageIncorrectLoadTimings) {
/*load_timing_info=*/
std::make_unique<net::LoadTimingInfo>(load_timing_info));

// Set document receive_headers_start.
// Set main frame document receive_headers_start. This field should be set
// only once.
const auto request_id = navigation_simulator->GetGlobalRequestID();
tester()->SimulateLoadedResource(request_info, request_id);

// Simulate the invocation of PageLoadTracker::OnLoadedResource() again with
// a ttfb earlier than the image load start and a request destination that is
// not of type Document. This should not overwrite the
// receive_headers_start that is already set.
load_timing_info.receive_headers_start =
reference_time + base::Milliseconds(29);

ExtraRequestCompleteInfo request_info1(
/*final_url=*/url::SchemeHostPort(GURL(kTestUrl)),
/*remote_endpoint=*/net::IPEndPoint(),
/*frame_tree_node_id=*/-1, /*was_cached=*/false, /*raw_body_bytes=*/0,
/*original_network_content_length=*/0,
/*request_destination=*/network::mojom::RequestDestination::kFrame,
/*net_error=*/0,
/*load_timing_info=*/
std::make_unique<net::LoadTimingInfo>(load_timing_info));

tester()->SimulateLoadedResource(request_info1, request_id);

// Set largest contentful paint timings.
tester()->SimulateTimingUpdate(timing);

Expand Down

0 comments on commit bd1eb06

Please sign in to comment.