predictors: move ResourcePrefetcher handling to LoadingPredictor.

chrome/browser/predictors/loading_predictor.cc

Index: chrome/browser/predictors/loading_predictor.cc
diff --git a/chrome/browser/predictors/loading_predictor.cc b/chrome/browser/predictors/loading_predictor.cc
index 5d1ced2c4b7ae1e5da06e56b220a3b1383e2ff53..f88402b789866c6bfc20c045c6d7dd31eaed0368 100644
--- a/chrome/browser/predictors/loading_predictor.cc
+++ b/chrome/browser/predictors/loading_predictor.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/predictors/loading_predictor.h"
 
+#include <vector>
+
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/predictors/loading_data_collector.h"
@@ -26,6 +28,7 @@ LoadingPredictor::LoadingPredictor(const LoadingPredictorConfig& config,
           config)),
       loading_data_collector_(base::MakeUnique<LoadingDataCollector>(
           resource_prefetch_predictor())),
+      observer_(nullptr),
       weak_factory_(this) {
   resource_prefetch_predictor_->SetStatsCollector(stats_collector_.get());
 }
@@ -41,9 +44,7 @@ void LoadingPredictor::PrepareForPageLoad(const GURL& url, HintOrigin origin) {
 
   // To report hint durations.
   active_hints_.emplace(url, base::TimeTicks::Now());
-
-  if (config_.IsPrefetchingEnabledForOrigin(profile_, origin))
-    resource_prefetch_predictor_->StartPrefetching(url, prediction);
+  MaybeAddPrefetch(url, prediction, origin);
 }
 
 void LoadingPredictor::CancelPageLoadHint(const GURL& url) {
@@ -64,6 +65,17 @@ ResourcePrefetchPredictor* LoadingPredictor::resource_prefetch_predictor() {
 
 void LoadingPredictor::Shutdown() {

[alexilin, 2017/07/03 14:55:01]

Could LoadingPredictor be destructed without a call to Shutdown()? Probably
it'll mean something bad has already happened anyway, so it's okay to destruct
prefetches on UI thread.

[Benoit L, 2017/07/03 15:43:14]

No, it can't.
Unless we crashed, in which case no destructor is called.

   resource_prefetch_predictor_->Shutdown();
+
+  std::vector<std::unique_ptr<ResourcePrefetcher>> prefetchers;
+  for (auto& kv : prefetches_)
+    prefetchers.push_back(std::move(kv.second.first));
+
+  // prefetchers must be destroyed on the IO thread.

[alexilin, 2017/07/03 14:55:01]

nit:
s/prefetchers/|prefetchers|/

[Benoit L, 2017/07/03 15:43:14]

Done.

+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(
+          [](std::vector<std::unique_ptr<ResourcePrefetcher>> prefetchers) {},
+          std::move(prefetchers)));
 }
 
 void LoadingPredictor::OnMainFrameRequest(const URLRequestSummary& summary) {
@@ -103,13 +115,17 @@ void LoadingPredictor::OnMainFrameResponse(const URLRequestSummary& summary) {
   }
 }
 
+void LoadingPredictor::SetObserverForTesting(TestLoadingObserver* observer) {
+  observer_ = observer;
+}
+
 std::map<GURL, base::TimeTicks>::iterator LoadingPredictor::CancelActiveHint(
     std::map<GURL, base::TimeTicks>::iterator hint_it) {
   if (hint_it == active_hints_.end())
     return hint_it;
 
   const GURL& url = hint_it->first;
-  resource_prefetch_predictor_->StopPrefetching(url);
+  MaybeRemovePrefetch(url);
 
   UMA_HISTOGRAM_TIMES(
       internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
@@ -149,4 +165,82 @@ void LoadingPredictor::CleanupAbandonedHintsAndNavigations(
   }
 }
 
+void LoadingPredictor::MaybeAddPrefetch(
+    const GURL& url,
+    const ResourcePrefetchPredictor::Prediction& prediction,
+    HintOrigin origin) {
+  if (!config_.IsPrefetchingEnabledForOrigin(profile_, origin))
+    return;
+  std::string host = url.host();
+  if (prefetches_.find(host) != prefetches_.end())
+    return;
+
+  auto prefetcher = base::MakeUnique<ResourcePrefetcher>(
+      GetWeakPtr(), profile_->GetRequestContext(),
+      config_.max_prefetches_inflight_per_navigation,
+      config_.max_prefetches_inflight_per_host_per_navigation, url,
+      prediction.subresource_urls);

[alexilin, 2017/07/03 14:55:01]

std::move(prediction.subresource_urls)
|prediction| can be moved in this function as well.

[Benoit L, 2017/07/03 15:43:14]

Thanks!

Done.

[alexilin, 2017/07/03 15:57:14]

I'm very suspicious about moving an object (or its part) that was passed by a
const reference. Could you rewrite the code to avoid that? Probably, it would be
better to pass |prediction| here as
unique_ptr<ResourcePrefetchPredictor::Prediction>

+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&ResourcePrefetcher::Start,
+                     base::Unretained(prefetcher.get())));

[alexilin, 2017/07/03 14:55:01]

Can you add a comment explaining that passing unretained pointer is okay in this
case because a prefetcher destruction will be scheduled on the IO thread after
that always?

[Benoit L, 2017/07/03 15:43:14]

Done.

+  prefetches_.emplace(host, std::make_pair(std::move(prefetcher), false));
+  if (observer_)
+    observer_->OnPrefetchingStarted(url);
+}
+
+void LoadingPredictor::MaybeRemovePrefetch(const GURL& url) {
+  std::string host = url.host();
+  auto it = prefetches_.find(host);
+  if (it == prefetches_.end())
+    return;
+
+  auto& prefetcher_and_deleted = it->second;
+  if (prefetcher_and_deleted.second)
+    return;
+
+  // Avoid duplicates.

[alexilin, 2017/07/03 14:55:01]

Can you make this comment more detailed? Something like
"Avoid to stop the same prefetcher twice."

[Benoit L, 2017/07/03 15:43:14]

Done.

+  prefetcher_and_deleted.second = true;
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&ResourcePrefetcher::Stop,
+                     base::Unretained(prefetcher_and_deleted.first.get())));
+  if (observer_)
+    observer_->OnPrefetchingStopped(url);
+}
+
+void LoadingPredictor::ResourcePrefetcherFinished(
+    ResourcePrefetcher* prefetcher,

[alexilin, 2017/07/03 14:55:01]

nit:
Do we really need to pass a pointer to ResourcePrefetcher? It seems unnecessary
and makes me more worried about passing raw ptrs between threads.
PrefetcherStats have a |url| field. On the other hand it could be not right to
wire business logic to stats-reporting logic.

[Benoit L, 2017/07/03 15:43:14]

The lifetime of a ResourcePrefetcher is entirely controlled by this object, that
is LoadingPredictor owns the ResourcePrefetcher, so I think that passing
pointers is fine here.

+    std::unique_ptr<ResourcePrefetcher::PrefetcherStats> stats) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  std::string host = prefetcher->main_frame_url().host();
+  auto it = prefetches_.find(host);
+  // Can happen after Shutdown() has been called, since it purges |prefetches_|.
+  if (it == prefetches_.end())
+    return;
+
+  DCHECK(it->second.first.get() == prefetcher);
+  stats_collector_->RecordPrefetcherStats(std::move(stats));
+
+  if (observer_)
+    observer_->OnPrefetchingFinished(prefetcher->main_frame_url());
+
+  // ResourcePrefetcher must be destroyed on the IO thread.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce([](std::unique_ptr<ResourcePrefetcher>) {},
+                     base::Passed(std::move(it->second.first))));

[alexilin, 2017/07/03 14:55:01]

Just curious, do you know whether we really need to use base::Passed()? I've
landed a CL recently where I used just std::move() without base::Passed() to
pass a unique_ptr to another thread.

[Benoit L, 2017/07/03 15:43:14]

Thanks for letting me know!
Done.

+
+  prefetches_.erase(it);
+}
+
+TestLoadingObserver::~TestLoadingObserver() {
+  predictor_->SetObserverForTesting(nullptr);
+}
+
+TestLoadingObserver::TestLoadingObserver(LoadingPredictor* predictor)
+    : predictor_(predictor) {
+  predictor_->SetObserverForTesting(this);
+}
+
 }  // namespace predictors