Commit bd7285b8 authored by Robert Ogden's avatar Robert Ogden Committed by Commit Bot

Partial revert of Report navigation restart penalty to PLM

Reverts all of the PLM changes in
https://chromium-review.googlesource.com/c/chromium/src/+/1334944
and a partial revert of the previews code because of merge conflicts.

git checkout 967d3a9b \
  chrome/browser/page_load_metrics/page* \
  chrome/browser/page_load_metrics/metrics*

Bug: 874150
Change-Id: Ic943baa1c8945c3dd033de5890ba0d70b2efd5bc
Reviewed-on: https://chromium-review.googlesource.com/c/1355331Reviewed-by: default avatarTarun Bansal <tbansal@chromium.org>
Reviewed-by: default avatarBryan McQuade <bmcquade@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612305}
parent af67b64d
......@@ -685,20 +685,6 @@ void MetricsWebContentsObserver::OnTimingUpdated(
}
}
bool MetricsWebContentsObserver::ReportNavigationRestartPenalty(
content::NavigationHandle* navigation_handle,
const base::TimeDelta mainframe_navigation_restart_penalty) {
DCHECK(!navigation_handle->HasCommitted());
auto it = provisional_loads_.find(navigation_handle);
if (it == provisional_loads_.end())
return false;
it->second->metrics_update_dispatcher()
->ReportMainFrameNavigationRestartPenalty(
mainframe_navigation_restart_penalty);
return true;
}
void MetricsWebContentsObserver::UpdateTiming(
mojom::PageLoadTimingPtr timing,
mojom::PageLoadMetadataPtr metadata,
......
......@@ -154,18 +154,6 @@ class MetricsWebContentsObserver
const std::vector<mojom::ResourceDataUpdatePtr>& resources,
mojom::PageRenderDataPtr render_data);
// Used to report that a NavigationThrottle or other browser intervention
// canceled a navigation and replaced it with a new navigation.
// |mainframe_navigation_restart_penalty| will be applied across all relevant
// timing deltas within the page for the page load tracker corresponding to
// |navigation_handle|. Returns whether or not there was an applicable
// navigation to update. This must be called before commit. Note: This method
// should not be called from a WebContentsObserver's DidFinishNavigation or
// DidStartNavigation since this would cause a race condition.
bool ReportNavigationRestartPenalty(
content::NavigationHandle* navigation_handle,
const base::TimeDelta mainframe_navigation_restart_penalty);
// Informs the observers of the currently committed load that the event
// corresponding to |event_key| has occurred. This should not be called within
// WebContentsObserver::DidFinishNavigation methods.
......
......@@ -459,35 +459,6 @@ TEST_F(MetricsWebContentsObserverTest, SubFrame) {
CheckNoErrorEvents();
}
TEST_F(MetricsWebContentsObserverTest, ReportNavigationRestartPenalty) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
timing.navigation_start = base::Time::FromDoubleT(1);
timing.response_start = base::TimeDelta::FromMilliseconds(10);
std::unique_ptr<content::NavigationSimulator> navigation_simulator =
content::NavigationSimulator::CreateRendererInitiated(
GURL(kDefaultTestUrl), main_rfh());
navigation_simulator->Start();
const base::TimeDelta penalty = base::TimeDelta::FromMilliseconds(5);
observer()->ReportNavigationRestartPenalty(
navigation_simulator->GetNavigationHandle(), penalty);
navigation_simulator->Commit();
SimulateTimingUpdate(timing);
NavigateToUntrackedUrl();
CheckNoErrorEvents();
ASSERT_EQ(1, CountCompleteTimingReported());
mojom::PageLoadTimingPtr got_timing = complete_timings()[0].Clone();
// TODO(crbug.com/906718): Test all updated metrics.
EXPECT_EQ(got_timing->navigation_start, timing.navigation_start - penalty);
EXPECT_EQ(got_timing->response_start.value(),
timing.response_start.value() - penalty);
}
TEST_F(MetricsWebContentsObserverTest, SameDocumentNoTrigger) {
mojom::PageLoadTiming timing;
page_load_metrics::InitPageLoadTimingForTest(&timing);
......
......@@ -261,34 +261,15 @@ class PageLoadTimingMerger {
: target_(target) {}
// Merge timing values from |new_page_load_timing| into the target
// PageLoadTiming, applying the |navigation_start_offset| to applicable
// metrics with respect to |is_main_frame|.
void MergeAndApplyOffset(base::TimeDelta navigation_start_offset,
const mojom::PageLoadTiming& new_page_load_timing,
bool is_main_frame) {
// TODO(crbug.com/906718): This approach is hard to maintain and likely to
// get stale over time.
// PageLoadTiming;
void Merge(base::TimeDelta navigation_start_offset,
const mojom::PageLoadTiming& new_page_load_timing,
bool is_main_frame) {
MergePaintTiming(navigation_start_offset,
*new_page_load_timing.paint_timing, is_main_frame);
MergeInteractiveTiming(navigation_start_offset,
*new_page_load_timing.interactive_timing,
is_main_frame);
// Don't need to merge these main-frame only metrics: merely apply the
// offset.
if (is_main_frame) {
target_->navigation_start =
new_page_load_timing.navigation_start + navigation_start_offset;
MaybeUpdateTimeDelta(&target_->response_start, navigation_start_offset,
new_page_load_timing.response_start);
MaybeUpdateTimeDelta(&target_->input_to_navigation_start,
navigation_start_offset,
new_page_load_timing.input_to_navigation_start);
ApplyDocumentTimingOffset(navigation_start_offset,
*new_page_load_timing.document_timing);
ApplyParseTimingOffset(navigation_start_offset,
*new_page_load_timing.parse_timing);
}
}
// Whether we merged a new value.
......@@ -362,25 +343,10 @@ class PageLoadTimingMerger {
MaybeUpdateTimeDelta(&target_paint_timing->first_contentful_paint,
navigation_start_offset,
new_paint_timing.first_contentful_paint);
// These are currently a no-op on main frames.
MaybeUpdateTimeDelta(&target_paint_timing->largest_image_paint,
navigation_start_offset,
new_paint_timing.largest_image_paint);
MaybeUpdateTimeDelta(&target_paint_timing->last_image_paint,
navigation_start_offset,
new_paint_timing.last_image_paint);
MaybeUpdateTimeDelta(&target_paint_timing->largest_text_paint,
navigation_start_offset,
new_paint_timing.largest_text_paint);
MaybeUpdateTimeDelta(&target_paint_timing->last_text_paint,
navigation_start_offset,
new_paint_timing.last_text_paint);
if (is_main_frame) {
// First meaningful paint is only tracked in the main frame.
MaybeUpdateTimeDelta(&target_paint_timing->first_meaningful_paint,
navigation_start_offset,
new_paint_timing.first_meaningful_paint);
target_paint_timing->first_meaningful_paint =
new_paint_timing.first_meaningful_paint;
}
}
......@@ -425,32 +391,6 @@ class PageLoadTimingMerger {
}
}
void ApplyDocumentTimingOffset(
base::TimeDelta navigation_start_offset,
const mojom::DocumentTiming& new_document_timing) {
mojom::DocumentTiming* target_document_timing =
target_->document_timing.get();
MaybeUpdateTimeDelta(
&target_document_timing->dom_content_loaded_event_start,
navigation_start_offset,
new_document_timing.dom_content_loaded_event_start);
MaybeUpdateTimeDelta(&target_document_timing->load_event_start,
navigation_start_offset,
new_document_timing.load_event_start);
MaybeUpdateTimeDelta(&target_document_timing->first_layout,
navigation_start_offset,
new_document_timing.first_layout);
}
void ApplyParseTimingOffset(base::TimeDelta navigation_start_offset,
const mojom::ParseTiming& new_parse_timing) {
mojom::ParseTiming* target_parse_timing = target_->parse_timing.get();
MaybeUpdateTimeDelta(&target_parse_timing->parse_start,
navigation_start_offset, new_parse_timing.parse_start);
MaybeUpdateTimeDelta(&target_parse_timing->parse_stop,
navigation_start_offset, new_parse_timing.parse_stop);
}
// The target PageLoadTiming we are merging values into.
mojom::PageLoadTiming* const target_;
......@@ -522,11 +462,6 @@ void PageLoadMetricsUpdateDispatcher::ShutDown() {
}
}
void PageLoadMetricsUpdateDispatcher::ReportMainFrameNavigationRestartPenalty(
base::TimeDelta penalty) {
total_main_frame_navigation_restart_penalty_ += penalty;
}
void PageLoadMetricsUpdateDispatcher::UpdateMetrics(
content::RenderFrameHost* render_frame_host,
mojom::PageLoadTimingPtr new_timing,
......@@ -600,13 +535,9 @@ void PageLoadMetricsUpdateDispatcher::UpdateSubFrameTiming(
client_->OnSubFrameTimingChanged(render_frame_host, *new_timing);
// |total_main_frame_navigation_restart_penalty_| should also be applied to
// subframes since this penalty is associated with the main frame.
base::TimeDelta navigation_start_offset =
it->second - total_main_frame_navigation_restart_penalty_;
base::TimeDelta navigation_start_offset = it->second;
PageLoadTimingMerger merger(pending_merged_page_timing_.get());
merger.MergeAndApplyOffset(navigation_start_offset, *new_timing,
false /* is_main_frame */);
merger.Merge(navigation_start_offset, *new_timing, false /* is_main_frame */);
MaybeDispatchTimingUpdates(merger.should_buffer_timing_update_callback());
}
......@@ -654,33 +585,9 @@ void PageLoadMetricsUpdateDispatcher::UpdateMainFrameTiming(
mojom::InteractiveTimingPtr last_interactive_timing =
std::move(pending_merged_page_timing_->interactive_timing);
base::TimeDelta navigation_start_offset =
-total_main_frame_navigation_restart_penalty_;
// Update the latest candidate to the corresponding buffers. We will dispatch
// the last candidate at the page load end. Because we don't want to dispatch
// the non-last candidate here, we clear it from |new_timing|.
// Since this also means that these timings won't get
// |navigation_start_offset| applied in the merger, apply it here.
if (new_timing->paint_timing->largest_image_paint.has_value()) {
new_timing->paint_timing->largest_image_paint =
new_timing->paint_timing->largest_image_paint.value() +
navigation_start_offset;
}
if (new_timing->paint_timing->last_image_paint.has_value()) {
new_timing->paint_timing->last_image_paint =
new_timing->paint_timing->last_image_paint.value() +
navigation_start_offset;
}
if (new_timing->paint_timing->largest_text_paint.has_value()) {
new_timing->paint_timing->largest_text_paint =
new_timing->paint_timing->largest_text_paint.value() +
navigation_start_offset;
}
if (new_timing->paint_timing->last_text_paint.has_value()) {
new_timing->paint_timing->last_text_paint =
new_timing->paint_timing->last_text_paint.value() +
navigation_start_offset;
}
largest_image_paint_.swap(new_timing->paint_timing->largest_image_paint);
new_timing->paint_timing->largest_image_paint.reset();
last_image_paint_.swap(new_timing->paint_timing->last_image_paint);
......@@ -699,8 +606,7 @@ void PageLoadMetricsUpdateDispatcher::UpdateMainFrameTiming(
std::move(last_interactive_timing);
PageLoadTimingMerger merger(pending_merged_page_timing_.get());
merger.MergeAndApplyOffset(navigation_start_offset, *new_timing,
true /* is_main_frame */);
merger.Merge(base::TimeDelta(), *new_timing, true /* is_main_frame */);
MaybeDispatchTimingUpdates(merger.should_buffer_timing_update_callback());
}
......
......@@ -136,11 +136,6 @@ class PageLoadMetricsUpdateDispatcher {
void DidFinishSubFrameNavigation(
content::NavigationHandle* navigation_handle);
// Increases |total_main_frame_navigation_restart_penalty_| by the given
// delta. This should be fully updated before commit. See the comment on
// |total_main_frame_navigation_restart_penalty_|.
void ReportMainFrameNavigationRestartPenalty(base::TimeDelta penalty);
void ShutDown();
const mojom::PageLoadTiming& timing() const {
......@@ -204,13 +199,6 @@ class PageLoadMetricsUpdateDispatcher {
mojom::PageLoadMetadataPtr subframe_metadata_;
mojom::PageRenderDataPtr main_frame_render_data_;
// This time delta is an otherwise unknown penalty where the final navigation
// to a page was not the original (i.e.: user started) navigation. This can be
// caused by client redirects or other browser-based interventions like
// previews. Applying this delta will reconcile this page load metric with the
// time when the original navigation started. This will be applied to all
// metrics just before they are sent to the client.
base::TimeDelta total_main_frame_navigation_restart_penalty_;
// Navigation start offsets for the most recently committed document in each
// frame.
......
......@@ -636,10 +636,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
}
{
......@@ -649,10 +645,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
VerifyPreviewLoaded();
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
true, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
}
{
......@@ -668,10 +660,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
}
{
......@@ -693,10 +681,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
PreviewsLitePageNavigationThrottle::IneligibleReason::kHttpPost, 1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
}
{
......@@ -712,10 +696,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
}
{
......@@ -729,10 +709,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
VerifyErrorPageLoaded();
}
......@@ -749,10 +725,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
}
{
......@@ -772,10 +744,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
1);
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
false, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
// Reset ECT for future tests.
g_browser_process->network_quality_tracker()
......@@ -884,10 +852,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
ClearDeciderState();
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
true, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.HttpOnlyFallbackPenalty", 1);
histogram_tester.ExpectBucketCount(
......@@ -907,10 +871,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
true, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.HttpOnlyFallbackPenalty", 1);
histogram_tester.ExpectBucketCount(
......@@ -940,10 +900,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
ClearDeciderState();
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
true, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 2);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.HttpOnlyFallbackPenalty", 1);
histogram_tester.ExpectBucketCount(
......@@ -959,10 +915,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBrowserTest,
ClearDeciderState();
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
true, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 2);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.HttpOnlyFallbackPenalty", 1);
histogram_tester.ExpectBucketCount(
......@@ -1088,29 +1040,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerTimeoutBrowserTest,
histogram_tester.ExpectBucketCount(
"Previews.ServerLitePage.ServerResponse",
PreviewsLitePageNavigationThrottle::ServerResponse::kTimeout, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 2);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
// Since this test already has a delay baked in, make sure the reported
// penalty is at least the length of the delay.
int max_penalty = 0;
for (const base::Bucket& bucket : histogram_tester.GetAllSamples(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty")) {
if (bucket.min > max_penalty) {
max_penalty = bucket.min;
}
}
// Expecting |max_penalty| > |kTimeoutMs| is flaky in release builds because
// of histogram bucketing. Since HistogramTester::Bucket doesn't provide a
// bucket max, if |max_penalty| < |kTimeoutMs|, check that a sample exists
// in the |kTimeoutMs| bucket.
if (max_penalty <= kTimeoutMs) {
EXPECT_GE(histogram_tester.GetBucketCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty",
kTimeoutMs),
1);
} // else, test passes
}
{
......@@ -1154,10 +1083,6 @@ IN_PROC_BROWSER_TEST_F(PreviewsLitePageServerBadServerBrowserTest,
histogram_tester.ExpectBucketCount("Previews.ServerLitePage.Triggered",
true, 1);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", 2);
histogram_tester.ExpectTotalCount(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", 0);
histogram_tester.ExpectBucketCount(
"Previews.ServerLitePage.ServerResponse",
PreviewsLitePageNavigationThrottle::ServerResponse::kFailed, 1);
......
......@@ -24,7 +24,6 @@
#include "chrome/browser/content_settings/cookie_settings_factory.h"
#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
#include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
#include "chrome/browser/previews/previews_lite_page_decider.h"
#include "chrome/browser/previews/previews_service.h"
#include "chrome/browser/previews/previews_service_factory.h"
......@@ -93,43 +92,6 @@ content::OpenURLParams MakeOpenURLParams(content::NavigationHandle* handle,
return url_params;
}
// This is called on |WillStartRequest| to check whether the given |handle| is
// for a navigation that was started by this preview. If it was, the penalty
// (time between the original navigation start and the navigation start of
// |handle|) is reported to PLM.
void MaybeReportNavigationRestartPenalty(content::NavigationHandle* handle) {
PreviewsUITabHelper* ui_tab_helper =
PreviewsUITabHelper::FromWebContents(handle->GetWebContents());
if (!ui_tab_helper)
return;
previews::PreviewsUserData* previews_data =
ui_tab_helper->GetPreviewsUserData(handle);
if (!previews_data)
return;
previews::PreviewsUserData::ServerLitePageInfo* info =
previews_data->server_lite_page_info();
if (!info)
return;
DCHECK_GT(info->original_navigation_start, base::TimeTicks());
base::TimeDelta penalty =
handle->NavigationStart() - info->original_navigation_start;
bool updated = page_load_metrics::MetricsWebContentsObserver::FromWebContents(
handle->GetWebContents())
->ReportNavigationRestartPenalty(handle, penalty);
DCHECK(updated);
if (updated) {
UMA_HISTOGRAM_MEDIUM_TIMES(
"Previews.ServerLitePage.ReportedNavigationRestartPenalty", penalty);
} else {
UMA_HISTOGRAM_MEDIUM_TIMES(
"Previews.ServerLitePage.NotReportedNavigationRestartPenalty", penalty);
}
}
// Gets the ServerLitePageInfo struct from an existing attempted lite page
// navigation, if there is one. If not, returns nullptr.
std::unique_ptr<previews::PreviewsUserData::ServerLitePageInfo>
......@@ -581,7 +543,6 @@ PreviewsLitePageNavigationThrottle::WillStartRequest() {
return content::NavigationThrottle::CANCEL;
}
MaybeReportNavigationRestartPenalty(navigation_handle());
return MaybeNavigateToPreview();
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment