Fix PrecacheFetcher behavior when reaching max_bytes.

components/precache/core/precache_fetcher.cc

Index: components/precache/core/precache_fetcher.cc
diff --git a/components/precache/core/precache_fetcher.cc b/components/precache/core/precache_fetcher.cc
index 44d4aa6c7e318e8c239bdd58e49fe8095aaa30e4..48c7a20af39d214b530279aa3659df3e7307a4ab 100644
--- a/components/precache/core/precache_fetcher.cc
+++ b/components/precache/core/precache_fetcher.cc
@@ -271,6 +271,11 @@ PrecacheFetcher::PrecacheFetcher(
 }
 
 PrecacheFetcher::~PrecacheFetcher() {
+  base::TimeDelta time_to_fetch = base::TimeTicks::Now() - start_time_;
+  UMA_HISTOGRAM_CUSTOM_TIMES("Precache.Fetch.TimeToComplete", time_to_fetch,
+                             base::TimeDelta::FromSeconds(1),
+                             base::TimeDelta::FromHours(4), 50);
+
   // Number of manifests for which we have downloaded all resources.
   int manifests_completed =
       num_manifest_urls_to_fetch_ - manifest_urls_to_fetch_.size();
@@ -305,6 +310,7 @@ void PrecacheFetcher::Start() {
 
   // Fetch the precache configuration settings from the server.
   DCHECK(pool_.IsEmpty()) << "All parallel requests should be available";
+  VLOG(3) << "Fetching " << config_url;
   pool_.Add(scoped_ptr<Fetcher>(new Fetcher(
       request_context_.get(), config_url,
       base::Bind(&PrecacheFetcher::OnConfigFetchComplete,
@@ -317,6 +323,7 @@ void PrecacheFetcher::StartNextResourceFetch() {
     const size_t max_bytes =
         std::min(config_->max_bytes_per_resource(),
                  config_->max_bytes_total() - total_response_bytes_);
+    VLOG(3) << "Fetching " << resource_urls_to_fetch_.front();
     pool_.Add(scoped_ptr<Fetcher>(
         new Fetcher(request_context_.get(), resource_urls_to_fetch_.front(),
                     base::Bind(&PrecacheFetcher::OnResourceFetchComplete,
@@ -336,6 +343,7 @@ void PrecacheFetcher::StartNextManifestFetch() {
   DCHECK(pool_.IsAvailable())
       << "There are no available parallel requests to fetch the next manifest. "
          "Did you forget to call Delete?";
+  VLOG(3) << "Fetching " << manifest_urls_to_fetch_.front();
   pool_.Add(scoped_ptr<Fetcher>(new Fetcher(
       request_context_.get(), manifest_urls_to_fetch_.front(),
       base::Bind(&PrecacheFetcher::OnManifestFetchComplete,
@@ -348,8 +356,8 @@ void PrecacheFetcher::StartNextManifestFetch() {
 void PrecacheFetcher::StartNextFetch() {
   // If over the precache total size cap, then stop prefetching.
   if (total_response_bytes_ > config_->max_bytes_total()) {
-    resource_urls_to_fetch_.clear();
-    manifest_urls_to_fetch_.clear();
+    precache_delegate_->OnDone();

[bengr, 2016/04/08 18:24:35]

You might want to also clear the lists, else the eventual pause/resume CL might
get confused.

[twifkak, 2016/04/08 19:09:05]

I can't clear the lists. That's what this CL is about. The lists are left in
tact so that PercentCompleted is computed correctly in the destructor.

As the code is currently architected, client code should construct a new
PrecacheFetcher each time it wants to fetch, and destroy immediately afterwards,
as the destructor logs all the relevant UMAs. I actually think that's a good fit
for the eventual pause/resume CL, but I could be wrong.

Otherwise, we can't clear the lists until after the UMAs are logged, or we'll
have to change the way PercentCompleted is computed.

[bengr, 2016/04/08 21:25:48]

I know that's what the CL is about. What you have is fine, but it might not
interact well with the pause/resume CL, but that's on my plate. :)

+    return;
   }
 
   StartNextResourceFetch();
@@ -357,11 +365,6 @@ void PrecacheFetcher::StartNextFetch() {
 
   if (pool_.IsEmpty()) {
     // There are no more URLs to fetch, so end the precache cycle.
-    base::TimeDelta time_to_fetch = base::TimeTicks::Now() - start_time_;
-    UMA_HISTOGRAM_CUSTOM_TIMES("Precache.Fetch.TimeToComplete", time_to_fetch,
-                               base::TimeDelta::FromSeconds(1),
-                               base::TimeDelta::FromHours(4), 50);
-
     precache_delegate_->OnDone();
     // OnDone may have deleted this PrecacheFetcher, so don't do anything after
     // it is called.
@@ -395,14 +398,19 @@ void PrecacheFetcher::OnConfigFetchComplete(const Fetcher& source) {
       ++rank;
       if (rank > config_->top_sites_count())
         break;
-      unique_manifest_urls.insert(ConstructManifestURL(prefix, host));
+      const std::string manifest_url = ConstructManifestURL(prefix, host);

[bengr, 2016/04/08 18:24:35]

I wonder if it makes sense to have a helper function in an anonymous namespace,
e.g.:

void AddUniqueURLsToList(const std::vector<std::string>& urls_or_hosts,
                         std::list<GURL>* url_list);

[twifkak, 2016/04/08 19:09:05]

Yeah, I was on the fence about that, but your comment pushes me over the fence.
Done.

+      bool first_seen = unique_manifest_urls.insert(manifest_url).second;
+      if (first_seen)
+        manifest_urls_to_fetch_.push_back(GURL(manifest_url));
     }
 
-    for (const std::string& url : config_->forced_site())
-      unique_manifest_urls.insert(ConstructManifestURL(prefix, url));
+    for (const std::string& url : config_->forced_site()) {
+      const std::string manifest_url = ConstructManifestURL(prefix, url);
+      bool first_seen = unique_manifest_urls.insert(manifest_url).second;
+      if (first_seen)
+        manifest_urls_to_fetch_.push_back(GURL(manifest_url));
+    }
 
-    for (const std::string& manifest_url : unique_manifest_urls)
-      manifest_urls_to_fetch_.push_back(GURL(manifest_url));
     num_manifest_urls_to_fetch_ = manifest_urls_to_fetch_.size();
   }
   pool_.Delete(source);