predictors: Refactor resource_prefetch_predictor triggering.

chrome/browser/predictors/resource_prefetch_predictor.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
@@ skipping 650 matching lines @@
     const base::TimeTicks& first_contentful_paint) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (initialization_state_ != 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 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) {
   if (observer_)
     observer_->OnPrefetchingFinished(main_frame_url);
 
   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;
 
   return GetPrefetchData(main_frame_url, nullptr);
 }
 
 bool ResourcePrefetchPredictor::IsResourcePrefetchable(
     const ResourceData& resource) const {
   float confidence = static_cast<float>(resource.number_of_hits()) /
@@ skipping 13 matching lines @@
     prefetch_manager_ = NULL;
   }
   history_service_observer_.RemoveAll();
 }
 
 void ResourcePrefetchPredictor::OnMainFrameRequest(
     const URLRequestSummary& request) {
   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(
     const URLRequestSummary& response) {
   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);
   }
 }
 
 void ResourcePrefetchPredictor::OnMainFrameRedirect(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(INITIALIZED, initialization_state_);
 
@@ skipping 1043 matching lines @@
   if (!history_service)
     return;
   DCHECK(!history_service_observer_.IsObserving(history_service));
   history_service_observer_.Add(history_service);
   if (history_service->BackendLoaded()) {
     // HistoryService is already loaded. Continue with Initialization.
     OnHistoryAndCacheLoaded();
   }
 }
 
+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.
 
 TestObserver::~TestObserver() {
   predictor_->SetObserverForTesting(nullptr);
 }
 
 TestObserver::TestObserver(ResourcePrefetchPredictor* predictor)
     : predictor_(predictor) {
   predictor_->SetObserverForTesting(this);
 }
 
 }  // namespace predictors