predictors: Refactor resource_prefetch_predictor triggering.

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 d5bd264009427aead4df1a4926e85807fee38f60..9f19112c52596dbb4a831daf55442b34a1a9ea08 100644
--- a/chrome/browser/predictors/loading_predictor.cc
+++ b/chrome/browser/predictors/loading_predictor.cc
@@ -5,24 +5,39 @@
 #include "chrome/browser/predictors/loading_predictor.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "chrome/browser/predictors/resource_prefetch_common.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 
 namespace predictors {
 
+using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
+
 LoadingPredictor::LoadingPredictor(const LoadingPredictorConfig& config,
-                                   Profile* profile) {
-  resource_prefetch_predictor_ =
-      base::MakeUnique<ResourcePrefetchPredictor>(config, profile);
-}
+                                   Profile* profile)
+    : config_(config),
+      profile_(profile),
+      resource_prefetch_predictor_(
+          base::MakeUnique<ResourcePrefetchPredictor>(config, profile)) {}
 
 LoadingPredictor::~LoadingPredictor() = default;
 
 void LoadingPredictor::PrepareForPageLoad(const GURL& url, HintOrigin origin) {
-  resource_prefetch_predictor_->StartPrefetching(url, origin);
+  // To report hint durations.
+  if (active_hints_.find(url) == active_hints_.end())
+    active_hints_.emplace(url, base::TimeTicks::Now());

[alexilin, 2017/05/30 16:19:14]

We used to report a hint duration only for prefetchable urls. Is this change
intended?

[Benoit L, 2017/05/31 14:34:41]

Done.

+
+  if (!config_.IsPrefetchingEnabledForOrigin(profile_, origin) ||
+      !resource_prefetch_predictor_->IsUrlPrefetchable(url)) {

[alexilin, 2017/05/30 16:19:14]

nit:
There is no real need to check for IsUrlPrefetchable here because if it's not
prefetchable, ResourcePrefetchPredictor just doesn't get a prediction.

[Benoit L, 2017/05/31 14:34:42]

Kept the check anyway, both for tracking and also to centralize triggering here.

+    return;
+  }
+
+  resource_prefetch_predictor_->StartPrefetching(url);

[alexilin, 2017/05/30 16:19:14]

Should we do something if the url is already in active_hints? We have
deduplication on ResourcePrefetcher level but we need to move it here. It's ok
for me to do it later.

[Benoit L, 2017/05/31 14:34:41]

Done.

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

[alexilin, 2017/05/30 16:19:14]

tiny tiny nit:
Why not in one line?
CancelActiveHint(active_hints_.find(url));

[Benoit L, 2017/05/31 14:34:41]

Done.

 }
 
 void LoadingPredictor::StartInitialization() {
@@ -38,4 +53,80 @@ void LoadingPredictor::Shutdown() {
   resource_prefetch_predictor_->Shutdown();
 }
 
+void LoadingPredictor::OnMainFrameRequest(const URLRequestSummary& summary) {
+  const NavigationID& navigation_id = summary.navigation_id;
+  CleanupAbandonedHintsAndNavigations(navigation_id);
+  active_navigations_.emplace(

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

Shouldn't it be a part of LoadingDataCollector? I'd prefer to avoid to have the
same functionality in different components.
It's fine to store active_hints here but active_navigations (page_loads?) should
live inside LoadingDataCollector.
Then, observers could just call CancelPageLoadHint with a url they have and
LoadingPredictor will consult LoadingDataCollector to obtain initial_url. WDYT?
For now page loads tracking is located inside ResourcePrefetchPredictor, you
could bind your code with it.

[Benoit L, 2017/05/31 14:34:42]

After offline discussion, decided to keep it this way.

+      navigation_id,
+      std::make_pair(navigation_id.main_frame_url, base::TimeTicks::Now()));

[alexilin, 2017/05/30 16:19:14]

The second member of the pair is unused.
You're using navigation_id.creation_time instead.

[Benoit L, 2017/05/31 14:34:41]

Done.

+  PrepareForPageLoad(navigation_id.main_frame_url, HintOrigin::NAVIGATION);
+}
+
+void LoadingPredictor::OnMainFrameRedirect(const URLRequestSummary& summary) {
+  auto it = active_navigations_.find(summary.navigation_id);
+  if (it != active_navigations_.end()) {
+    NavigationID navigation_id = summary.navigation_id;
+    navigation_id.main_frame_url = summary.redirect_url;

[alexilin, 2017/05/30 16:19:14]

What if redirect_url == main_frame_url?
I know redirecting to itself is silly but shouldn't we check that the insertion
really happened?

[Benoit L, 2017/05/31 14:34:42]

That's likely filtered lower in the stack (as unless the 302 is no-store, this
is an infinite loop), but more paranoia doesn't hurt indeed.
Done.

[alexilin, 2017/05/31 16:02:03]

Well, a redirect response could set a new cookie so it could have some sense.

+    active_navigations_.emplace(navigation_id, it->second);
+    active_navigations_.erase(it);
+  }
+}
+
+void LoadingPredictor::OnMainFrameResponse(const URLRequestSummary& summary) {
+  const NavigationID& navigation_id = summary.navigation_id;
+  auto it = active_navigations_.find(navigation_id);
+  if (it != active_navigations_.end()) {
+    const auto& initial_url = it->second.first;

[alexilin, 2017/05/30 16:19:14]

nit:
s/auto/GURL/ since it's the same number of chars but GURL is easier to read

[Benoit L, 2017/05/31 14:34:42]

Done.

+    CancelPageLoadHint(initial_url);
+    active_navigations_.erase(it);
+  } else {
+    CancelPageLoadHint(navigation_id.main_frame_url);
+  }
+}
+
+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);
+
+  UMA_HISTOGRAM_TIMES(
+      internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
+      base::TimeTicks::Now() - hint_it->second);
+  return active_hints_.erase(hint_it);
+}
+
+void LoadingPredictor::CleanupAbandonedHintsAndNavigations(
+    const NavigationID& navigation_id) {
+  base::TimeTicks time_now = base::TimeTicks::Now();
+  const base::TimeDelta max_navigation_age =
+      base::TimeDelta::FromSeconds(config_.max_navigation_lifetime_seconds);
+
+  // Hints.
+  for (auto it = active_hints_.begin(); it != active_hints_.end();) {
+    base::TimeDelta prefetch_age = time_now - it->second;
+    if (prefetch_age > max_navigation_age) {
+      // Will to the last bucket meaning that the duration was unlimited.

[alexilin, 2017/05/30 15:06:00]

nit:
Delete the comment because it isn't relevant.

[Benoit L, 2017/05/31 14:34:41]

I actually wanted to point that out, clarified the comment.

[alexilin, 2017/05/31 16:02:03]

s/Will to/Will go to/
Initially I was confused by this comment because I didn't understand it :)

+      it = CancelActiveHint(it);
+    } else {
+      ++it;
+    }
+  }
+
+  // Navigations.
+  for (auto it = active_navigations_.begin();
+       it != active_navigations_.end();) {
+    if ((it->first.tab_id == navigation_id.tab_id) ||
+        (time_now - it->first.creation_time > max_navigation_age)) {
+      const GURL& initial_url = it->second.first;
+      CancelActiveHint(active_hints_.find(initial_url));

[alexilin, 2017/05/30 16:19:14]

nit:
CancelPageLoadHint(initial_url) since we don't care about the return value and
to be consistent with OnMainFrameResponse()?

[Benoit L, 2017/05/31 14:34:41]

I would prefer to keep it as is to be consistent with the call above in the same
function.

+      it = active_navigations_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+}
+
 }  // namespace predictors