predictors: Refactor resource_prefetch_predictor triggering.

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index 67f84ed92d885c4bc8cfd5525535693212118eae..9439f35c8174e234b50fd3de8b013fa9fe6693ff 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -668,56 +668,6 @@ void ResourcePrefetchPredictor::RecordFirstContentfulPaint(
     nav_it->second->first_contentful_paint = first_contentful_paint;
 }
 
-void ResourcePrefetchPredictor::StartPrefetching(const GURL& url,

[alexilin, 2017/05/23 11:01:39]

Why moving code?

-                                                 HintOrigin origin) {
-  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StartPrefetching", "url",
-               url.spec());
-  // Save prefetch start time to report prefetching duration.
-  if (inflight_prefetches_.find(url) == inflight_prefetches_.end() &&
-      IsUrlPrefetchable(url)) {
-    inflight_prefetches_.insert(std::make_pair(url, base::TimeTicks::Now()));
-  }
-
-  if (!prefetch_manager_.get())  // Prefetching not enabled.
-    return;
-  if (!config_.IsPrefetchingEnabledForOrigin(profile_, origin))
-    return;
-
-  ResourcePrefetchPredictor::Prediction prediction;
-  if (!GetPrefetchData(url, &prediction))
-    return;
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ResourcePrefetcherManager::MaybeAddPrefetch,
-                     prefetch_manager_, url, prediction.subresource_urls));
-
-  if (observer_)
-    observer_->OnPrefetchingStarted(url);
-}
-
-void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
-  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StopPrefetching", "url",
-               url.spec());
-  auto it = inflight_prefetches_.find(url);
-  if (it != inflight_prefetches_.end()) {
-    UMA_HISTOGRAM_TIMES(
-        internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
-        base::TimeTicks::Now() - it->second);
-    inflight_prefetches_.erase(it);
-  }
-  if (!prefetch_manager_.get())  // Not enabled.
-    return;
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ResourcePrefetcherManager::MaybeRemovePrefetch,
-                     prefetch_manager_, url));
-
-  if (observer_)
-    observer_->OnPrefetchingStopped(url);
-}
-
 void ResourcePrefetchPredictor::OnPrefetchingFinished(
     const GURL& main_frame_url,
     std::unique_ptr<ResourcePrefetcher::PrefetcherStats> stats) {
@@ -727,7 +677,8 @@ void ResourcePrefetchPredictor::OnPrefetchingFinished(
   prefetcher_stats_.insert(std::make_pair(main_frame_url, std::move(stats)));
 }
 
-bool ResourcePrefetchPredictor::IsUrlPrefetchable(const GURL& main_frame_url) {
+bool ResourcePrefetchPredictor::IsUrlPrefetchable(
+    const GURL& main_frame_url) const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (initialization_state_ != INITIALIZED)
     return false;
@@ -761,15 +712,13 @@ void ResourcePrefetchPredictor::OnMainFrameRequest(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(INITIALIZED, initialization_state_);
 
-  const GURL& main_frame_url = request.navigation_id.main_frame_url;
-  StartPrefetching(main_frame_url, HintOrigin::NAVIGATION);
-
   CleanupAbandonedNavigations(request.navigation_id);
 
   // New empty navigation entry.
-  inflight_navigations_.insert(
-      std::make_pair(request.navigation_id,
-                     base::MakeUnique<PageRequestSummary>(main_frame_url)));
+  const GURL& main_frame_url = request.navigation_id.main_frame_url;
+  inflight_navigations_.emplace(
+      request.navigation_id,
+      base::MakeUnique<PageRequestSummary>(main_frame_url));
 }
 
 void ResourcePrefetchPredictor::OnMainFrameResponse(
@@ -777,10 +726,9 @@ void ResourcePrefetchPredictor::OnMainFrameResponse(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(INITIALIZED, initialization_state_);
 
-  NavigationMap::iterator nav_it =
-      inflight_navigations_.find(response.navigation_id);
+  auto nav_it = inflight_navigations_.find(response.navigation_id);
   if (nav_it != inflight_navigations_.end()) {
-    // To match an URL in StartPrefetching().
+    // To match a URL in StartPrefetching().
     StopPrefetching(nav_it->second->initial_url);

[alexilin, 2017/05/23 11:01:39]

Shouldn't we move StopPrefetching outside of ResourcePrefetchPredictor as well?

[Benoit L, 2017/05/29 16:45:14]

The unfortunate part is that without navigation tracking in LoadingPredictor, we
cannot know which prefetch to stop.
That's why the navigation prefetch is "fire and forget" for now.

Any suggestion to make it simpler in one step?

[alexilin, 2017/05/30 11:55:27]

Answered in other comment.
TL;DR
Expose navigation tracking from ResourcePrefetchPredictor now, later this logic
will move to LoadingDataCollector.

void LoadingPredictor::CancelPageLoadHint(const GURL& url) {
  const auto& summary = resource_prefetch_predictor_->GetPageLoadSummary(url);
  auto it = active_hints_.find(summary.initial_url);
  CancelActiveHint(it);
}

   } else {
     StopPrefetching(response.navigation_id.main_frame_url);
@@ -1845,6 +1793,59 @@ void ResourcePrefetchPredictor::ConnectToHistoryService() {
   }
 }
 
+bool ResourcePrefetchPredictor::CanPrefetchUrlForOrigin(
+    const GURL& url,
+    HintOrigin origin) const {
+  return prefetch_manager_.get() &&  // Prefetching not enabled.
+         config_.IsPrefetchingEnabledForOrigin(profile_, origin) &&

[alexilin, 2017/05/23 11:01:38]

I think the second condition is always true if the first one is fulfilled.
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...

[Benoit L, 2017/05/29 16:45:14]

Nope, the first one is true if prefetching is enabled for some origin, not for
the current one.

[alexilin, 2017/05/30 11:55:27]

Yup, you're right.

The following paragraph is just observation that could be useful when we'll be
deleting PrefetchManager:
The opposite direction almost works as well: the first condition is true if the
second one is fulfilled except the case when ResourcePrefetchPredictor is
non-initialized or was shut down. 
So we indirectly check the initialization state of ResourcePrefetchPredictor by
checking prefetch_manager_. The class shouldn't be used after shutdown so we can
replace the first check by initialization_state_ == INITIALIZED and it should
work.

+         IsUrlPrefetchable(url);

[alexilin, 2017/05/23 11:01:39]

TODO:
Look at the cost of IsUrlPrefetchable() method call. We call it many times for a
single navigation (with producing the same results) and it's not trivial.

+}
+
+void ResourcePrefetchPredictor::StartPrefetching(const GURL& url,
+                                                 HintOrigin origin) {
+  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StartPrefetching", "url",
+               url.spec());
+  DCHECK(CanPrefetchUrlForOrigin(url, origin));
+
+  // Save prefetch start time to report prefetching duration.
+  if (inflight_prefetches_.find(url) == inflight_prefetches_.end())
+    inflight_prefetches_.emplace(url, base::TimeTicks::Now());
+
+  ResourcePrefetchPredictor::Prediction prediction;
+  bool has_data = GetPrefetchData(url, &prediction);
+  DCHECK(has_data);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&ResourcePrefetcherManager::MaybeAddPrefetch,
+                     prefetch_manager_, url, prediction.subresource_urls));
+
+  if (observer_)
+    observer_->OnPrefetchingStarted(url);
+}
+
+void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
+  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StopPrefetching", "url",
+               url.spec());
+  auto it = inflight_prefetches_.find(url);
+  if (it != inflight_prefetches_.end()) {
+    UMA_HISTOGRAM_TIMES(
+        internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
+        base::TimeTicks::Now() - it->second);
+    inflight_prefetches_.erase(it);
+  }
+  if (!prefetch_manager_.get())  // Not enabled.
+    return;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&ResourcePrefetcherManager::MaybeRemovePrefetch,
+                     prefetch_manager_, url));
+
+  if (observer_)
+    observer_->OnPrefetchingStopped(url);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // TestObserver.