Commit 6415e8a1 authored by Robert Ogden's avatar Robert Ogden Committed by Commit Bot

Update Prefetch Status on Ineligible Isolated Prerender Redirects

When Isolated Prerender encounters an ineligible redirect that is not
the final URL, the after srp metrics do not get correctly updated. This
CL fixes that by checking the whole redirect chain from the end to the
beginning for metrics.

Note that in practice this is only likely to affect the metrics on
AMP pages and ads, since the Google SRP shortcuts redirect chains that
Google doesn't otherwise create.

Bug: 1106942
Change-Id: I26499291a4e8039d78524daf0d34dd4bc383af69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2307535Reviewed-by: default avatarTarun Bansal <tbansal@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790084}
parent 8b6808ba
...@@ -457,20 +457,39 @@ void IsolatedPrerenderTabHelper::DidFinishNavigation( ...@@ -457,20 +457,39 @@ void IsolatedPrerenderTabHelper::DidFinishNavigation(
new_page->after_srp_metrics_->probe_latency_ = page_->probe_latency_; new_page->after_srp_metrics_->probe_latency_ = page_->probe_latency_;
auto status_iter = page_->prefetch_status_by_url_.find(url); // Check every url in the redirect chain for a status, starting at the end
if (status_iter != page_->prefetch_status_by_url_.end()) { // and working backwards. Note: When a redirect chain is eligible all the
new_page->after_srp_metrics_->prefetch_status_ = // way to the end, the status is already propagated. But if a redirect was
MaybeUpdatePrefetchStatusWithNSPContext(url, status_iter->second); // not eligible then this will find its last known status.
DCHECK(!navigation_handle->GetRedirectChain().empty());
base::Optional<PrefetchStatus> status;
base::Optional<size_t> prediction_position;
for (auto back_iter = navigation_handle->GetRedirectChain().rbegin();
back_iter != navigation_handle->GetRedirectChain().rend();
++back_iter) {
GURL chain_url = *back_iter;
auto status_iter = page_->prefetch_status_by_url_.find(chain_url);
if (!status && status_iter != page_->prefetch_status_by_url_.end()) {
status = MaybeUpdatePrefetchStatusWithNSPContext(chain_url,
status_iter->second);
}
// Same check for the original prediction ordering.
auto position_iter = page_->original_prediction_ordering_.find(chain_url);
if (!prediction_position &&
position_iter != page_->original_prediction_ordering_.end()) {
prediction_position = position_iter->second;
}
}
if (status) {
new_page->after_srp_metrics_->prefetch_status_ = *status;
} else { } else {
new_page->after_srp_metrics_->prefetch_status_ = new_page->after_srp_metrics_->prefetch_status_ =
PrefetchStatus::kNavigatedToLinkNotOnSRP; PrefetchStatus::kNavigatedToLinkNotOnSRP;
} }
new_page->after_srp_metrics_->clicked_link_srp_position_ =
auto position_iter = page_->original_prediction_ordering_.find(url); prediction_position;
if (position_iter != page_->original_prediction_ordering_.end()) {
new_page->after_srp_metrics_->clicked_link_srp_position_ =
position_iter->second;
}
// See if the page being navigated to was prerendered. If so, copy over its // See if the page being navigated to was prerendered. If so, copy over its
// subresource manager and networking pipes. // subresource manager and networking pipes.
......
...@@ -192,6 +192,7 @@ class IsolatedPrerenderTabHelperTest : public ChromeRenderViewHostTestHarness { ...@@ -192,6 +192,7 @@ class IsolatedPrerenderTabHelperTest : public ChromeRenderViewHostTestHarness {
handle.set_url(url); handle.set_url(url);
tab_helper_->DidStartNavigation(&handle); tab_helper_->DidStartNavigation(&handle);
handle.set_has_committed(true); handle.set_has_committed(true);
handle.set_redirect_chain({url});
tab_helper_->DidFinishNavigation(&handle); tab_helper_->DidFinishNavigation(&handle);
} }
...@@ -203,6 +204,7 @@ class IsolatedPrerenderTabHelperTest : public ChromeRenderViewHostTestHarness { ...@@ -203,6 +204,7 @@ class IsolatedPrerenderTabHelperTest : public ChromeRenderViewHostTestHarness {
handle.set_is_same_document(true); handle.set_is_same_document(true);
tab_helper_->DidStartNavigation(&handle); tab_helper_->DidStartNavigation(&handle);
handle.set_has_committed(true); handle.set_has_committed(true);
handle.set_redirect_chain({GURL("https://test.com")});
tab_helper_->DidFinishNavigation(&handle); tab_helper_->DidFinishNavigation(&handle);
} }
...@@ -1561,6 +1563,43 @@ TEST_F(IsolatedPrerenderTabHelperRedirectTest, NoRedirect_Insecure) { ...@@ -1561,6 +1563,43 @@ TEST_F(IsolatedPrerenderTabHelperRedirectTest, NoRedirect_Insecure) {
EXPECT_EQ(base::Optional<size_t>(0), after_srp_clicked_link_srp_position()); EXPECT_EQ(base::Optional<size_t>(0), after_srp_clicked_link_srp_position());
} }
TEST_F(IsolatedPrerenderTabHelperRedirectTest, NoRedirect_Insecure_Continued) {
NavigateSomewhere();
GURL url("http://insecure.com");
RunNoRedirectTest(url);
EXPECT_EQ(predicted_urls_count(), 1U);
EXPECT_EQ(prefetch_eligible_count(), 1U);
EXPECT_EQ(prefetch_attempted_count(), 1U);
EXPECT_EQ(prefetch_successful_count(), 0U);
EXPECT_EQ(prefetch_total_redirect_count(), 1U);
EXPECT_TRUE(navigation_to_prefetch_start().has_value());
GURL final_url("http://final.com/");
content::MockNavigationHandle handle(web_contents());
handle.set_url(final_url);
tab_helper()->DidStartNavigation(&handle);
handle.set_has_committed(true);
handle.set_redirect_chain({
GURL("https://start.test.com"),
url,
final_url,
});
tab_helper()->DidFinishNavigation(&handle);
ASSERT_TRUE(tab_helper()->after_srp_metrics().has_value());
ASSERT_TRUE(tab_helper()->after_srp_metrics()->prefetch_status_.has_value());
EXPECT_EQ(IsolatedPrerenderTabHelper::PrefetchStatus::
kPrefetchNotEligibleSchemeIsNotHttps,
tab_helper()->after_srp_metrics()->prefetch_status_.value());
EXPECT_EQ(after_srp_prefetch_eligible_count(), 1U);
EXPECT_EQ(base::Optional<size_t>(0), after_srp_clicked_link_srp_position());
}
TEST_F(IsolatedPrerenderTabHelperRedirectTest, NoRedirect_Google) { TEST_F(IsolatedPrerenderTabHelperRedirectTest, NoRedirect_Google) {
NavigateSomewhere(); NavigateSomewhere();
......
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