predictors: Move more methods from ResourcePrefetchPredictor into LoadingDataCollector.

chrome/browser/predictors/loading_data_collector.cc

Index: chrome/browser/predictors/loading_data_collector.cc
diff --git a/chrome/browser/predictors/loading_data_collector.cc b/chrome/browser/predictors/loading_data_collector.cc
index 92705ae7773ae5784ed029e6412408dd01969fcc..b477bce5ce0c7a96fd0fa141f5ead7caa1e63327 100644
--- a/chrome/browser/predictors/loading_data_collector.cc
+++ b/chrome/browser/predictors/loading_data_collector.cc
@@ -5,18 +5,47 @@
 #include <string>
 
 #include "chrome/browser/predictors/loading_data_collector.h"
+#include "chrome/browser/predictors/loading_stats_collector.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/history/core/browser/history_service.h"
 #include "components/mime_util/mime_util.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_type.h"
 #include "net/url_request/url_request.h"
 
+using content::BrowserThread;
+
 namespace predictors {
 
 namespace {
 
+using PageRequestSummary =
+    predictors::ResourcePrefetchPredictor::PageRequestSummary;

[trevordixon, 2017/06/14 08:04:34]

Might make sense to move PageRequestSummary into loading_predictor.h or
loading_data_collector.h?

[alexilin, 2017/06/14 11:41:54]

I'd suggest you to move all of OriginRequestSummary, URLRequestSummary and
PageRequestSummary into loading_data_collector.h. And I think it'd be even
better to make them non-nested.

[trevordixon, 2017/06/16 05:11:28]

Done.

+
 bool g_allow_port_in_urls = false;
 
+void UpdateOrAddToOrigins(
+    std::map<GURL, ResourcePrefetchPredictor::OriginRequestSummary>* summaries,
+    const ResourcePrefetchPredictor::URLRequestSummary& request_summary) {
+  const GURL& request_url = request_summary.request_url;
+  DCHECK(request_url.is_valid());
+  if (!request_url.is_valid())
+    return;
+
+  GURL origin = request_url.GetOrigin();
+  auto it = summaries->find(origin);
+  if (it == summaries->end()) {
+    ResourcePrefetchPredictor::OriginRequestSummary summary;
+    summary.origin = origin;
+    summary.first_occurrence = summaries->size();
+    it = summaries->insert({origin, summary}).first;
+  }
+
+  it->second.always_access_network |=
+      request_summary.always_revalidate || request_summary.is_no_store;
+  it->second.accessed_network |= request_summary.network_accessed;
+}
+
 }  // namespace
 
 // static
@@ -110,35 +139,212 @@ void LoadingDataCollector::SetAllowPortInUrlsForTesting(bool state) {
   g_allow_port_in_urls = state;
 }
 
-LoadingDataCollector::LoadingDataCollector(ResourcePrefetchPredictor* predictor)
-    : predictor_(predictor) {}
+LoadingDataCollector::LoadingDataCollector(
+    ResourcePrefetchPredictor* predictor,
+    predictors::LoadingStatsCollector* stats_collector,
+    const LoadingPredictorConfig& config)
+    : predictor_(predictor),
+      stats_collector_(stats_collector),
+      config_(config) {}
 
 LoadingDataCollector::~LoadingDataCollector() {}
 
 void LoadingDataCollector::RecordURLRequest(
     const ResourcePrefetchPredictor::URLRequestSummary& request) {
-  predictor_->RecordURLRequest(request);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (predictor_->initialization_state() !=
+      ResourcePrefetchPredictor::INITIALIZED)

[trevordixon, 2017/06/14 08:04:34]

I copied all of the initialization_state checks over, so the logic hasn't
changed at all, but I think it might not be necessary. I think the only place
where the predictor's initialization_state matters is in
LoadingDataCollector::OnNavigationComplete, before calling
ResourcePrefetchPredictor::HandlePageRequestSummary. Can this check be moved
into either LoadingDataCollector::OnNavigationComplete or
ResourcePrefetchPredictor::HandlePageRequestSummary?

[alexilin, 2017/06/14 11:41:54]

Yeah, initialization is only needed for ResourcePrefetchPredictor.
LoadingDataCollector has nothing to initialize. So I think it'll be clearer to
delete all initialization-related logic from this class and keep it inside
ResourcePrefetchPredictor. LoadingDataCollector shouldn't care about what's
initialization state of ResourcePrefetchPredictor, that's my opinion.

[trevordixon, 2017/06/16 05:11:28]

Done.

+    return;
+
+  CHECK_EQ(request.resource_type, content::RESOURCE_TYPE_MAIN_FRAME);
+  OnMainFrameRequest(request);
 }
 
 void LoadingDataCollector::RecordURLResponse(
     const ResourcePrefetchPredictor::URLRequestSummary& response) {
-  predictor_->RecordURLResponse(response);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (predictor_->initialization_state() !=
+      ResourcePrefetchPredictor::INITIALIZED)
+    return;
+
+  if (response.resource_type != content::RESOURCE_TYPE_MAIN_FRAME)
+    OnSubresourceResponse(response);
 }
 
 void LoadingDataCollector::RecordURLRedirect(
     const ResourcePrefetchPredictor::URLRequestSummary& response) {
-  predictor_->RecordURLRedirect(response);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (predictor_->initialization_state() !=
+      ResourcePrefetchPredictor::INITIALIZED)
+    return;
+
+  if (response.resource_type == content::RESOURCE_TYPE_MAIN_FRAME)
+    OnMainFrameRedirect(response);
+  else
+    OnSubresourceRedirect(response);
 }
 
 void LoadingDataCollector::RecordMainFrameLoadComplete(
     const NavigationID& navigation_id) {
-  predictor_->RecordMainFrameLoadComplete(navigation_id);
+  switch (predictor_->initialization_state()) {
+    case ResourcePrefetchPredictor::NOT_INITIALIZED:
+      predictor_->StartInitialization();
+      break;
+    case ResourcePrefetchPredictor::INITIALIZING:
+      break;
+    case ResourcePrefetchPredictor::INITIALIZED:
+      // WebContents can return an empty URL if the navigation entry
+      // corresponding to the navigation has not been created yet.
+      if (!navigation_id.main_frame_url.is_empty())
+        OnNavigationComplete(navigation_id);
+      break;
+    default:
+      NOTREACHED() << "Unexpected initialization_state_: "
+                   << predictor_->initialization_state();
+  }
 }
 
 void LoadingDataCollector::RecordFirstContentfulPaint(
     const NavigationID& navigation_id,
     const base::TimeTicks& first_contentful_paint) {
-  predictor_->RecordFirstContentfulPaint(navigation_id, first_contentful_paint);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (predictor_->initialization_state() !=
+      ResourcePrefetchPredictor::INITIALIZED)
+    return;
+
+  NavigationMap::iterator nav_it = inflight_navigations_.find(navigation_id);
+  if (nav_it != inflight_navigations_.end())
+    nav_it->second->first_contentful_paint = first_contentful_paint;
+}
+
+void LoadingDataCollector::OnMainFrameRequest(
+    const ResourcePrefetchPredictor::URLRequestSummary& request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(ResourcePrefetchPredictor::INITIALIZED,
+            predictor_->initialization_state());
+
+  CleanupAbandonedNavigations(request.navigation_id);
+
+  // New empty navigation entry.
+  const GURL& main_frame_url = request.navigation_id.main_frame_url;
+  inflight_navigations_.emplace(
+      request.navigation_id,
+      base::MakeUnique<PageRequestSummary>(main_frame_url));
+}
+
+void LoadingDataCollector::OnMainFrameRedirect(
+    const ResourcePrefetchPredictor::URLRequestSummary& response) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(ResourcePrefetchPredictor::INITIALIZED,
+            predictor_->initialization_state());
+
+  const GURL& main_frame_url = response.navigation_id.main_frame_url;
+  std::unique_ptr<PageRequestSummary> summary;
+  NavigationMap::iterator nav_it =
+      inflight_navigations_.find(response.navigation_id);
+  if (nav_it != inflight_navigations_.end()) {
+    summary = std::move(nav_it->second);
+    inflight_navigations_.erase(nav_it);
+  }
+
+  // The redirect url may be empty if the URL was invalid.
+  if (response.redirect_url.is_empty())
+    return;
+
+  // If we lost the information about the first hop for some reason.
+  if (!summary) {
+    summary = base::MakeUnique<PageRequestSummary>(main_frame_url);
+  }
+
+  // A redirect will not lead to another OnMainFrameRequest call, so record the
+  // redirect url as a new navigation id and save the initial url.
+  NavigationID navigation_id(response.navigation_id);
+  navigation_id.main_frame_url = response.redirect_url;
+  summary->main_frame_url = response.redirect_url;
+  inflight_navigations_.emplace(navigation_id, std::move(summary));
+}
+
+void LoadingDataCollector::OnSubresourceResponse(
+    const ResourcePrefetchPredictor::URLRequestSummary& response) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(ResourcePrefetchPredictor::INITIALIZED,
+            predictor_->initialization_state());
+
+  NavigationMap::const_iterator nav_it =
+      inflight_navigations_.find(response.navigation_id);
+  if (nav_it == inflight_navigations_.end())
+    return;
+  auto& page_request_summary = *nav_it->second;
+
+  if (!response.is_no_store)
+    page_request_summary.subresource_requests.push_back(response);
+
+  if (config_.is_origin_learning_enabled)
+    UpdateOrAddToOrigins(&page_request_summary.origins, response);
+}
+
+void LoadingDataCollector::OnSubresourceRedirect(
+    const ResourcePrefetchPredictor::URLRequestSummary& response) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(ResourcePrefetchPredictor::INITIALIZED,
+            predictor_->initialization_state());
+
+  if (!config_.is_origin_learning_enabled)
+    return;
+
+  NavigationMap::const_iterator nav_it =
+      inflight_navigations_.find(response.navigation_id);
+  if (nav_it == inflight_navigations_.end())
+    return;
+  auto& page_request_summary = *nav_it->second;
+  UpdateOrAddToOrigins(&page_request_summary.origins, response);
+}
+
+void LoadingDataCollector::OnNavigationComplete(
+    const NavigationID& nav_id_without_timing_info) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(ResourcePrefetchPredictor::INITIALIZED,
+            predictor_->initialization_state());
+
+  NavigationMap::iterator nav_it =
+      inflight_navigations_.find(nav_id_without_timing_info);
+  if (nav_it == inflight_navigations_.end())
+    return;
+
+  // Remove the navigation from the inflight navigations.
+  std::unique_ptr<PageRequestSummary> summary = std::move(nav_it->second);
+  inflight_navigations_.erase(nav_it);
+
+  // Set before_first_contentful paint for each resource.
+  for (auto& request_summary : summary->subresource_requests) {
+    request_summary.before_first_contentful_paint =
+        request_summary.response_time < summary->first_contentful_paint;
+  }
+
+  if (stats_collector_)
+    stats_collector_->RecordPageRequestSummary(*summary);
+
+  predictor_->HandlePageRequestSummary(std::move(summary));
+}
+
+void LoadingDataCollector::CleanupAbandonedNavigations(
+    const NavigationID& navigation_id) {
+  if (stats_collector_)
+    stats_collector_->CleanupAbandonedStats();
+
+  static const base::TimeDelta max_navigation_age =
+      base::TimeDelta::FromSeconds(config_.max_navigation_lifetime_seconds);
+
+  base::TimeTicks time_now = base::TimeTicks::Now();
+  for (NavigationMap::iterator it = inflight_navigations_.begin();
+       it != inflight_navigations_.end();) {
+    if ((it->first.tab_id == navigation_id.tab_id) ||
+        (time_now - it->first.creation_time > max_navigation_age)) {
+      inflight_navigations_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
 }
 
 }  // namespace predictors