Fix PrecacheFetcher behavior when reaching max_bytes.

components/precache/core/precache_fetcher.cc

 // Copyright 2013 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 "components/precache/core/precache_fetcher.h"
 
 #include <algorithm>
 #include <limits>
 #include <string>
 #include <utility>
@@ skipping 253 matching lines @@
   DCHECK(request_context_.get());  // Request context must be non-NULL.
   DCHECK(precache_delegate_);  // Precache delegate must be non-NULL.
 
   DCHECK_NE(GURL(), GetDefaultConfigURL())
       << "Could not determine the precache config settings URL.";
   DCHECK_NE(std::string(), GetDefaultManifestURLPrefix())
       << "Could not determine the default precache manifest URL prefix.";
 }
 
 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();
 
   // If there are resource URLs left to fetch, the last manifest is not yet
   // completed.
   if (!resource_urls_to_fetch_.empty())
     --manifests_completed;
 
   DCHECK_GE(manifests_completed, 0);
@@ skipping 14 matching lines @@
   GURL config_url =
       config_url_.is_empty() ? GetDefaultConfigURL() : config_url_;
 
   DCHECK(config_url.is_valid()) << "Config URL not valid: "
                                 << config_url.possibly_invalid_spec();
 
   start_time_ = base::TimeTicks::Now();
 
   // 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,
                  base::Unretained(this)),
       false /* is_resource_request */, std::numeric_limits<int32_t>::max())));
 }
 
 void PrecacheFetcher::StartNextResourceFetch() {
   while (!resource_urls_to_fetch_.empty() && pool_.IsAvailable()) {
     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,
                                base::Unretained(this)),
                     true /* is_resource_request */, max_bytes)));
 
     resource_urls_to_fetch_.pop_front();
   }
 }
 
 void PrecacheFetcher::StartNextManifestFetch() {
   if (manifest_urls_to_fetch_.empty())
     return;
 
   // We only fetch one manifest at a time to keep the size of
   // resource_urls_to_fetch_ as small as possible.
   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,
                  base::Unretained(this)),
       false /* is_resource_request */, std::numeric_limits<int32_t>::max())));
 
   manifest_urls_to_fetch_.pop_front();
 }
 
 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();
   StartNextManifestFetch();
 
   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.
   }
 }
 
 void PrecacheFetcher::OnConfigFetchComplete(const Fetcher& source) {
   UpdateStats(source.response_bytes(), source.network_response_bytes());
   if (source.network_url_fetcher() == nullptr) {
     pool_.DeleteAll();  // Cancel any other ongoing request.
@@ skipping 13 matching lines @@
     base::hash_set<std::string> unique_manifest_urls;
 
     // Attempt to fetch manifests for starting hosts up to the maximum top sites
     // count. If a manifest does not exist for a particular starting host, then
     // the fetch will fail, and that starting host will be ignored.
     int64_t rank = 0;
     for (const std::string& host : starting_hosts_) {
       ++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);
 
   StartNextFetch();
 }
 
 void PrecacheFetcher::OnManifestFetchComplete(const Fetcher& source) {
   UpdateStats(source.response_bytes(), source.network_response_bytes());
   if (source.network_url_fetcher() == nullptr) {
@@ skipping 24 matching lines @@
   StartNextFetch();
 }
 
 void PrecacheFetcher::UpdateStats(int64_t response_bytes,
                                   int64_t network_response_bytes) {
   total_response_bytes_ += response_bytes;
   network_response_bytes_ += network_response_bytes;
 }
 
 }  // namespace precache