Get correct PLT measurement for Resource Prefetching for Mobile Web

The starting point of the PLT measurement for Resource Prefetching for Mobile Web is when the main resource request is created and sent out. Current implementation actually did not store the start time in the right data structure, leading to incorrect PLT measurement. This CL fixes this.

inflight_navigations_ stores the NavigationID. When measuring PLT, we need to use the navigation ID stored in inflight_navigations_, instead of using the newly created ones.

BUG=405690

Review URL: https://codereview.chromium.org/1133493002

Cr-Commit-Position: refs/heads/master@{#329009}

4 files changed, 86 insertions, 31 deletions

diff --git a/chrome/browser/net/resource_prefetch_predictor_observer.cc b/chrome/browser/net/resource_prefetch_predictor_observer.cc
index 5a3f4a1..81d9b07 100644
--- a/chrome/browser/net/resource_prefetch_predictor_observer.cc
+++ b/chrome/browser/net/resource_prefetch_predictor_observer.cc
@@ -118,6 +118,7 @@ void ResourcePrefetchPredictorObserver::OnRequestStarted(
   summary.navigation_id.render_process_id = child_id;
   summary.navigation_id.render_frame_id = frame_id;
   summary.navigation_id.main_frame_url = request->first_party_for_cookies();
+  summary.navigation_id.creation_time = request->creation_time();
   summary.resource_url = request->original_url();
   summary.resource_type = resource_type;
 
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index 3e1055e..616028b 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -513,18 +513,22 @@ void ResourcePrefetchPredictor::OnSubresourceResponse(
   nav_it->second->push_back(response);
 }
 
-void ResourcePrefetchPredictor::OnNavigationComplete(
-    const NavigationID& navigation_id) {
+base::TimeDelta ResourcePrefetchPredictor::OnNavigationComplete(
+    const NavigationID& nav_id_without_timing_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   NavigationMap::iterator nav_it =
-      inflight_navigations_.find(navigation_id);
+      inflight_navigations_.find(nav_id_without_timing_info);
   if (nav_it == inflight_navigations_.end()) {
     RecordNavigationEvent(NAVIGATION_EVENT_ONLOAD_UNTRACKED_URL);
-    return;
+    return base::TimeDelta();
   }
   RecordNavigationEvent(NAVIGATION_EVENT_ONLOAD_TRACKED_URL);
 
+  // Get and use the navigation ID stored in |inflight_navigations_| because it
+  // has the timing infomation.
+  const NavigationID navigation_id(nav_it->first);
+
   // Report any stats.
   base::TimeDelta plt = base::TimeTicks::Now() - navigation_id.creation_time;
   ReportPageLoadTimeStats(plt);
@@ -580,6 +584,8 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
               base::Bind(&ResourcePrefetchPredictor::OnVisitCountLookup,
                          AsWeakPtr()))),
       &history_lookup_consumer_);
+
+  return plt;
 }
 
 bool ResourcePrefetchPredictor::GetPrefetchData(
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.h b/chrome/browser/predictors/resource_prefetch_predictor.h
index f446a40..d6a0053 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor.h
@@ -141,6 +141,7 @@ class ResourcePrefetchPredictor
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnMainFrameRedirect);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            OnSubresourceResponse);
+  FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetCorrectPLT);
 
   enum InitializationState {
     NOT_INITIALIZED = 0,
@@ -190,9 +191,11 @@ class ResourcePrefetchPredictor
   void OnSubresourceResponse(const URLRequestSummary& response);
 
   // Called when onload completes for a navigation. We treat this point as the
-  // "completion" of the navigation. The resources requested by the page upto
-  // this point are the only ones considered for prefetching.
-  void OnNavigationComplete(const NavigationID& navigation_id);
+  // "completion" of the navigation. The resources requested by the page up to
+  // this point are the only ones considered for prefetching. Return the page
+  // load time for testing.
+  base::TimeDelta OnNavigationComplete(
+      const NavigationID& nav_id_without_timing_info);
 
   // Returns true if there is PrefetchData that can be used for the
   // navigation and fills in the |prefetch_data| to resources that need to be
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
index a4e8165..ae8f29e 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
@@ -382,7 +382,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationNotRecorded) {
                               std::string(),
                               false);
   predictor_->RecordURLRequest(main_frame);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
   // Now add a few subresources.
   URLRequestSummary resource1 = CreateURLRequestSummary(
@@ -440,7 +440,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDB) {
                               std::string(),
                               false);
   predictor_->RecordURLRequest(main_frame);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
   URLRequestSummary resource1 = CreateURLRequestSummary(
       1, 1, "http://www.google.com",  "http://google.com/style1.css",
@@ -522,8 +522,8 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) {
                       SetArgPointee<1>(test_host_data_)));
   ResetPredictor();
   InitializePredictor();
-  EXPECT_EQ(3, static_cast<int>(predictor_->url_table_cache_->size()));
-  EXPECT_EQ(2, static_cast<int>(predictor_->host_table_cache_->size()));
+  EXPECT_EQ(3U, predictor_->url_table_cache_->size());
+  EXPECT_EQ(2U, predictor_->host_table_cache_->size());
 
   URLRequestSummary main_frame =
       CreateURLRequestSummary(1,
@@ -534,7 +534,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) {
                               std::string(),
                               false);
   predictor_->RecordURLRequest(main_frame);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
   URLRequestSummary resource1 = CreateURLRequestSummary(
       1, 1, "http://www.google.com",  "http://google.com/style1.css",
@@ -646,8 +646,8 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) {
                       SetArgPointee<1>(test_host_data_)));
   ResetPredictor();
   InitializePredictor();
-  EXPECT_EQ(3, static_cast<int>(predictor_->url_table_cache_->size()));
-  EXPECT_EQ(2, static_cast<int>(predictor_->host_table_cache_->size()));
+  EXPECT_EQ(3U, predictor_->url_table_cache_->size());
+  EXPECT_EQ(2U, predictor_->host_table_cache_->size());
 
   URLRequestSummary main_frame =
       CreateURLRequestSummary(1,
@@ -658,7 +658,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) {
                               std::string(),
                               false);
   predictor_->RecordURLRequest(main_frame);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
   URLRequestSummary resource1 = CreateURLRequestSummary(
       1, 1, "http://www.nike.com",  "http://nike.com/style1.css",
@@ -746,8 +746,8 @@ TEST_F(ResourcePrefetchPredictorTest, DeleteUrls) {
       DeleteData(ContainerEq(urls_to_delete), ContainerEq(hosts_to_delete)));
 
   predictor_->DeleteUrls(rows);
-  EXPECT_EQ(2, static_cast<int>(predictor_->url_table_cache_->size()));
-  EXPECT_EQ(1, static_cast<int>(predictor_->host_table_cache_->size()));
+  EXPECT_EQ(2U, predictor_->url_table_cache_->size());
+  EXPECT_EQ(1U, predictor_->host_table_cache_->size());
 
   EXPECT_CALL(*mock_tables_.get(), DeleteAllData());
 
@@ -783,11 +783,11 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRequest) {
                               false);
 
   predictor_->OnMainFrameRequest(summary1);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
   predictor_->OnMainFrameRequest(summary2);
-  EXPECT_EQ(2, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(2U, predictor_->inflight_navigations_.size());
   predictor_->OnMainFrameRequest(summary3);
-  EXPECT_EQ(3, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(3U, predictor_->inflight_navigations_.size());
 
   // Insert anther with same navigation id. It should replace.
   URLRequestSummary summary4 =
@@ -808,13 +808,13 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRequest) {
                               false);
 
   predictor_->OnMainFrameRequest(summary4);
-  EXPECT_EQ(3, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(3U, predictor_->inflight_navigations_.size());
 
   // Change this creation time so that it will go away on the next insert.
   summary5.navigation_id.creation_time = base::TimeTicks::Now() -
       base::TimeDelta::FromDays(1);
   predictor_->OnMainFrameRequest(summary5);
-  EXPECT_EQ(3, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(3U, predictor_->inflight_navigations_.size());
 
   URLRequestSummary summary6 =
       CreateURLRequestSummary(3,
@@ -825,7 +825,7 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRequest) {
                               std::string(),
                               false);
   predictor_->OnMainFrameRequest(summary6);
-  EXPECT_EQ(3, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(3U, predictor_->inflight_navigations_.size());
 
   EXPECT_TRUE(predictor_->inflight_navigations_.find(summary3.navigation_id) !=
               predictor_->inflight_navigations_.end());
@@ -865,14 +865,14 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRedirect) {
   EXPECT_TRUE(predictor_->inflight_navigations_.empty());
 
   predictor_->OnMainFrameRequest(summary1);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
   predictor_->OnMainFrameRequest(summary2);
-  EXPECT_EQ(2, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(2U, predictor_->inflight_navigations_.size());
 
   predictor_->OnMainFrameRedirect(summary3);
-  EXPECT_EQ(2, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(2U, predictor_->inflight_navigations_.size());
   predictor_->OnMainFrameRedirect(summary1);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
   predictor_->OnMainFrameRedirect(summary2);
   EXPECT_TRUE(predictor_->inflight_navigations_.empty());
 }
@@ -895,7 +895,7 @@ TEST_F(ResourcePrefetchPredictorTest, OnSubresourceResponse) {
                               std::string(),
                               false);
   predictor_->OnMainFrameRequest(main_frame1);
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
   // Now add a few subresources.
   URLRequestSummary resource2 = CreateURLRequestSummary(
@@ -908,9 +908,9 @@ TEST_F(ResourcePrefetchPredictorTest, OnSubresourceResponse) {
   predictor_->OnSubresourceResponse(resource2);
   predictor_->OnSubresourceResponse(resource3);
 
-  EXPECT_EQ(1, static_cast<int>(predictor_->inflight_navigations_.size()));
-  EXPECT_EQ(3, static_cast<int>(
-      predictor_->inflight_navigations_[main_frame1.navigation_id]->size()));
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
+  EXPECT_EQ(3U,
+      predictor_->inflight_navigations_[main_frame1.navigation_id]->size());
   EXPECT_TRUE(URLRequestSummaryAreEqual(
       resource1,
       predictor_->inflight_navigations_[main_frame1.navigation_id]->at(0)));
@@ -922,4 +922,49 @@ TEST_F(ResourcePrefetchPredictorTest, OnSubresourceResponse) {
       predictor_->inflight_navigations_[main_frame1.navigation_id]->at(2)));
 }
 
+TEST_F(ResourcePrefetchPredictorTest, GetCorrectPLT) {
+  // Single navigation but history count is low, so should not record.
+  AddUrlToHistory("http://www.google.com", 1);
+
+  URLRequestSummary main_frame =
+      CreateURLRequestSummary(1,
+                              1,
+                              "http://www.google.com",
+                              "http://www.google.com",
+                              content::RESOURCE_TYPE_MAIN_FRAME,
+                              std::string(),
+                              false);
+  predictor_->RecordURLRequest(main_frame);
+  EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
+
+  // Reset the creation time in |main_frame.navigation_id|. The correct creation
+  // time is stored in |inflight_navigations_| and should be used later.
+  main_frame.navigation_id.creation_time = base::TimeTicks();
+  EXPECT_TRUE(main_frame.navigation_id.creation_time.is_null());
+
+  // Now add a subresource.
+  URLRequestSummary resource1 = CreateURLRequestSummary(
+      1, 1, "http://www.google.com",  "http://google.com/style1.css",
+      content::RESOURCE_TYPE_STYLESHEET, "text/css", false);
+  predictor_->RecordURLResponse(resource1);
+
+  PrefetchData host_data(PREFETCH_KEY_TYPE_HOST, "www.google.com");
+  host_data.resources.push_back(ResourceRow(std::string(),
+                                            "http://google.com/style1.css",
+                                            content::RESOURCE_TYPE_STYLESHEET,
+                                            1,
+                                            0,
+                                            0,
+                                            1.0));
+  EXPECT_CALL(*mock_tables_.get(), UpdateData(empty_url_data_, host_data));
+
+  // The page load time will be collected by RPP_HISTOGRAM_MEDIUM_TIMES, which
+  // has a upper bound of 3 minutes.
+  base::TimeDelta plt =
+      predictor_->OnNavigationComplete(main_frame.navigation_id);
+  EXPECT_LT(plt, base::TimeDelta::FromSeconds(180));
+
+  profile_->BlockUntilHistoryProcessesPendingRequests();
+}
+
 }  // namespace predictors