predictors: Prefetch resources from manifest.

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 65f3f90a68533dc541111e060820f3b160dcd2b4..a8f9f64f211f3ab05468a5075bb6159e5398043d 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -100,6 +100,12 @@ void InitializeOriginStatFromOriginRequestSummary(
   origin->set_accessed_network(summary.accessed_network);
 }
 
+bool IsManifestTooOld(const precache::PrecacheManifest& manifest) {
+  const base::TimeDelta kMaxManifestAge = base::TimeDelta::FromDays(5);

[alexilin, 2017/04/18 07:05:03]

I don't feel strongly about this. 
Probably, it would be better to add the timestamp of the last fetch instead of
using manifest id.

+  return base::Time::Now() - base::Time::FromDoubleT(manifest.id().id()) >
+         kMaxManifestAge;
+}
+
 // Used to fetch the visit count for a URL from the History database.
 class GetUrlVisitCountTask : public history::HistoryDBTask {
  public:
@@ -898,16 +904,33 @@ bool ResourcePrefetchPredictor::GetPrefetchData(
   // Use host data if the URL-based prediction isn't available.
   std::string main_frame_url_host = main_frame_url.host();
   if (GetRedirectEndpoint(main_frame_url_host, *host_redirect_table_cache_,
-                          &redirect_endpoint) &&
-      PopulatePrefetcherRequest(redirect_endpoint, *host_table_cache_, urls)) {
-    if (prediction) {
-      prediction->is_host = true;
-      prediction->main_frame_key = redirect_endpoint;
-      prediction->is_redirected = (redirect_endpoint != main_frame_url_host);
+                          &redirect_endpoint)) {
+    if (PopulatePrefetcherRequest(redirect_endpoint, *host_table_cache_,
+                                  urls)) {
+      if (prediction) {
+        prediction->is_host = true;
+        prediction->main_frame_key = redirect_endpoint;
+        prediction->is_redirected = (redirect_endpoint != main_frame_url_host);
+      }
+      return true;
     }
-    return true;
-  }
 
+    if (config_.is_manifests_enabled) {
+      // Use manifest data for host if available.
+      std::string manifest_host = redirect_endpoint;
+      if (base::StartsWith(manifest_host, "www.", base::CompareCase::SENSITIVE))

[Benoit L, 2017/04/18 16:28:39]

is www systematically stripped from the manifest keys?

[alexilin, 2017/04/19 07:59:25]

Yes. (See history::HostForTopHosts and how it's used for manifests)

+        manifest_host.assign(manifest_host, 4, std::string::npos);
+      if (PopulatePrefetcherRequestByManifest(manifest_host, urls)) {
+        if (prediction) {
+          prediction->is_host = true;
+          prediction->main_frame_key = redirect_endpoint;
+          prediction->is_redirected =
+              (redirect_endpoint != main_frame_url_host);
+        }
+        return true;
+      }
+    }
+  }
   return false;
 }
 
@@ -931,6 +954,45 @@ bool ResourcePrefetchPredictor::PopulatePrefetcherRequest(
   return has_prefetchable_resource;
 }
 
+bool ResourcePrefetchPredictor::PopulatePrefetcherRequestByManifest(
+    const std::string& manifest_host,
+    std::vector<GURL>* urls) const {
+  auto it = manifest_table_cache_->find(manifest_host);
+  if (it == manifest_table_cache_->end())
+    return false;
+
+  const precache::PrecacheManifest& manifest = it->second;
+
+  if (IsManifestTooOld(manifest))
+    return false;
+
+  const float kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;

[Benoit L, 2017/04/18 16:28:39]

Have you looked at the practical meaning of this threshold?


Also, tiny nit: since the name is really local, no need to make it that long :-)

[alexilin, 2017/04/19 07:59:25]

Roughly speaking, it's percentage of host pages where the resource is used. In
addition, pages are also weighted.
 
Done.

[Benoit L, 2017/04/19 14:48:54]

Ok, so can you add a comment saying that this is roughly in line with the
threshold we use in the rest of the code?

[alexilin, 2017/04/19 15:35:11]

Done.

+
+  base::Optional<std::vector<bool>> unused_bitset =
+      precache::GetResourceBitset(manifest, internal::kUnusedRemovedExperiment);
+  base::Optional<std::vector<bool>> versioned_bitset =
+      precache::GetResourceBitset(manifest,
+                                  internal::kVersionedRemovedExperiment);
+  base::Optional<std::vector<bool>> no_store_bitset =
+      precache::GetResourceBitset(manifest,
+                                  internal::kNoStoreRemovedExperiment);
+
+  bool has_prefetchable_resource = false;
+  for (int i = 0; i < manifest.resource_size(); ++i) {
+    const precache::PrecacheResource& resource = manifest.resource(i);
+    if (resource.weight_ratio() > kMinPrecacheResourceWeightToTriggerPrefetch &&
+        (!unused_bitset.has_value() || unused_bitset.value()[i]) &&

[Benoit L, 2017/04/18 16:28:39]

what is the meaning of the bits?

if unused, versioned and no_store are all set, is the resource prefetchable?


Update: after offline discussion, the logic is correct, but the naming weird.
Can you add a comment?

[alexilin, 2017/04/19 07:59:25]

Renamed and commented.
Done.

+        (!versioned_bitset.has_value() || versioned_bitset.value()[i]) &&
+        (!no_store_bitset.has_value() || no_store_bitset.value()[i])) {
+      has_prefetchable_resource = true;
+      if (urls)
+        urls->push_back(GURL(resource.url()));

[Benoit L, 2017/04/18 16:28:39]

useless_nit: emplace_back?

[alexilin, 2017/04/19 07:59:25]

Done.

+    }
+  }
+
+  return has_prefetchable_resource;
+}
+
 void ResourcePrefetchPredictor::CreateCaches(
     std::unique_ptr<PrefetchDataMap> url_data_map,
     std::unique_ptr<PrefetchDataMap> host_data_map,
@@ -1637,7 +1699,7 @@ void ResourcePrefetchPredictor::OnManifestFetched(
   if (initialization_state_ != INITIALIZED)
     return;
 
-  if (!config_.is_manifests_enabled)
+  if (!config_.is_manifests_enabled || IsManifestTooOld(manifest))
     return;
 
   // The manifest host has "www." prefix stripped, the manifest host could
@@ -1688,9 +1750,10 @@ void ResourcePrefetchPredictor::UpdatePrefetchDataByManifest(
   for (int i = 0; i < manifest.resource_size(); ++i)
     manifest_index.insert({manifest.resource(i).url(), i});
 
-  bool was_updated = false;
   base::Optional<std::vector<bool>> unused_bitset =
       precache::GetResourceBitset(manifest, internal::kUnusedRemovedExperiment);
+
+  bool was_updated = false;
   if (unused_bitset.has_value()) {
     // Remove unused resources from |data|.
     auto new_end = std::remove_if(