predictors: Make ResourcePrefetchPredictor observable.

chrome/browser/predictors/resource_prefetch_predictor.cc

 // Copyright 2014 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 "chrome/browser/predictors/resource_prefetch_predictor.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
@@ skipping 21 matching lines @@
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/mime_util.h"
 #include "net/base/network_change_notifier.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context_getter.h"
 
 using content::BrowserThread;
 
+namespace predictors {
+
 namespace {
 
 // Sorted by decreasing likelihood according to HTTP archive.
 const char* kFontMimeTypes[] = {"font/woff2",
                                 "application/x-font-woff",
                                 "application/font-woff",
                                 "application/font-woff2",
                                 "font/x-woff",
                                 "application/x-font-ttf",
                                 "font/woff",
                                 "font/ttf",
                                 "application/x-font-otf",
                                 "x-font/woff",
                                 "application/font-sfnt",
                                 "application/font-ttf"};
 
 // For reporting events of interest that are not tied to any navigation.
 enum ReportingEvent {
   REPORTING_EVENT_ALL_HISTORY_CLEARED = 0,
   REPORTING_EVENT_PARTIAL_HISTORY_CLEARED = 1,
   REPORTING_EVENT_COUNT = 2
 };
 
 float ComputeRedirectConfidence(const predictors::RedirectStat& redirect) {
   return (redirect.number_of_hits() + 0.0) /
          (redirect.number_of_hits() + redirect.number_of_misses());
 }
 
+// Used to fetch the visit count for a URL from the History database.
+class GetUrlVisitCountTask : public history::HistoryDBTask {
+ public:
+  using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
+  using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
+  typedef base::Callback<void(size_t,  // URL visit count.
+                              const PageRequestSummary&)>
+      VisitInfoCallback;
+
+  GetUrlVisitCountTask(std::unique_ptr<PageRequestSummary> summary,
+                       VisitInfoCallback callback);
+
+  bool RunOnDBThread(history::HistoryBackend* backend,
+                     history::HistoryDatabase* db) override;
+
+  void DoneRunOnMainThread() override;
+
+ private:
+  ~GetUrlVisitCountTask() override;
+
+  int visit_count_;
+  std::unique_ptr<PageRequestSummary> summary_;
+  VisitInfoCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(GetUrlVisitCountTask);
+};
+
+GetUrlVisitCountTask::GetUrlVisitCountTask(
+    std::unique_ptr<PageRequestSummary> summary,
+    VisitInfoCallback callback)
+    : visit_count_(0), summary_(std::move(summary)), callback_(callback) {
+  DCHECK(summary_.get());
+}
+
+bool GetUrlVisitCountTask::RunOnDBThread(history::HistoryBackend* backend,
+                                         history::HistoryDatabase* db) {
+  history::URLRow url_row;
+  if (db->GetRowForURL(summary_->main_frame_url, &url_row))
+    visit_count_ = url_row.visit_count();
+  return true;
+}
+
+void GetUrlVisitCountTask::DoneRunOnMainThread() {
+  callback_.Run(visit_count_, *summary_);
+}
+
+GetUrlVisitCountTask::~GetUrlVisitCountTask() {}
+
 }  // namespace
 
-namespace predictors {
-
 ////////////////////////////////////////////////////////////////////////////////
 // ResourcePrefetchPredictor static functions.
 
 // static
 bool ResourcePrefetchPredictor::ShouldRecordRequest(
     net::URLRequest* request,
     content::ResourceType resource_type) {
   const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(request);
   if (!request_info)
@@ skipping 219 matching lines @@
     summary->has_validators = headers->HasValidators();
     // RFC 2616, section 14.9.
     summary->always_revalidate =
         headers->HasHeaderValue("cache-control", "no-cache") ||
         headers->HasHeaderValue("pragma", "no-cache") ||
         headers->HasHeaderValue("vary", "*");
   }
   return true;
 }
 
-ResourcePrefetchPredictor::GetUrlVisitCountTask::GetUrlVisitCountTask(
-    const NavigationID& navigation_id,
-    std::unique_ptr<PageRequestSummary> summary,
-    VisitInfoCallback callback)
-    : visit_count_(0),
-      navigation_id_(navigation_id),
-      summary_(std::move(summary)),
-      callback_(callback) {
-  DCHECK(summary_.get());
-}
-
-bool ResourcePrefetchPredictor::GetUrlVisitCountTask::RunOnDBThread(
-    history::HistoryBackend* backend,
-    history::HistoryDatabase* db) {
-  history::URLRow url_row;
-  if (db->GetRowForURL(navigation_id_.main_frame_url, &url_row))
-    visit_count_ = url_row.visit_count();
-  return true;
-}
-
-void ResourcePrefetchPredictor::GetUrlVisitCountTask::DoneRunOnMainThread() {
-  callback_.Run(visit_count_, navigation_id_, *summary_);
-}
-
-ResourcePrefetchPredictor::GetUrlVisitCountTask::~GetUrlVisitCountTask() {}
+ResourcePrefetchPredictor::PageRequestSummary::PageRequestSummary(
+    const GURL& i_main_frame_url)
+    : main_frame_url(i_main_frame_url), initial_url(i_main_frame_url) {}
 
 ResourcePrefetchPredictor::PageRequestSummary::PageRequestSummary(
-    const GURL& i_initial_url)
-    : initial_url(i_initial_url) {}
+    const PageRequestSummary& other)
+    : main_frame_url(other.main_frame_url),
+      initial_url(other.initial_url),
+      subresource_requests(other.subresource_requests) {}
 
 ResourcePrefetchPredictor::PageRequestSummary::~PageRequestSummary() {}
 
+ResourcePrefetchPredictor::TestObserver::~TestObserver() {
+  ResetResourcePrefetchPredictor();
+}
+
+ResourcePrefetchPredictor::TestObserver::TestObserver(
+    ResourcePrefetchPredictor* predictor)
+    : predictor_(predictor) {
+  predictor_->SetObserverForTesting(this);
+}
+
+void ResourcePrefetchPredictor::TestObserver::ResetResourcePrefetchPredictor() {
+  if (predictor_) {
+    predictor_->observer_ = nullptr;

[pasko, 2016/10/24 17:59:38]

yuck! does this compile because the inner class is a friend to the outer? Let's
try to avoid such tight coupling.

[alexilin, 2016/10/25 11:38:09]

Then we have to make SetObserverForTesting method public.

[pasko, 2016/10/25 14:05:40]

I believe this is how it is usually done in Chromium. So that's what I wanted
initially, it was not super clear, sorry about that.

[alexilin, 2016/10/25 15:10:58]

Acknowledged.

+    predictor_ = nullptr;
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ResourcePrefetchPredictor.
 
 ResourcePrefetchPredictor::ResourcePrefetchPredictor(
     const ResourcePrefetchPredictorConfig& config,
     Profile* profile)
     : profile_(profile),
+      observer_(nullptr),
       config_(config),
       initialization_state_(NOT_INITIALIZED),
       tables_(PredictorDatabaseFactory::GetForProfile(profile)
                   ->resource_prefetch_tables()),
       history_service_observer_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Some form of learning has to be enabled.
   DCHECK(config_.IsLearningEnabled());
   if (config_.IsURLPrefetchingEnabled(profile_))
     DCHECK(config_.IsURLLearningEnabled());
   if (config_.IsHostPrefetchingEnabled(profile_))
     DCHECK(config_.IsHostLearningEnabled());
 }
 
 ResourcePrefetchPredictor::~ResourcePrefetchPredictor() {
+  if (observer_)
+    observer_->ResetResourcePrefetchPredictor();
 }
 
 void ResourcePrefetchPredictor::RecordURLRequest(
     const URLRequestSummary& request) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (initialization_state_ != INITIALIZED)
     return;
 
   CHECK_EQ(request.resource_type, content::RESOURCE_TYPE_MAIN_FRAME);
   OnMainFrameRequest(request);
@@ skipping 34 matching lines @@
       // corresponding to the navigation has not been created yet.
       if (!navigation_id.main_frame_url.is_empty())
         OnNavigationComplete(navigation_id);
       break;
     default:
       NOTREACHED() << "Unexpected initialization_state_: "
                    << initialization_state_;
   }
 }
 
+void ResourcePrefetchPredictor::StartPrefetching(const GURL& url) {
+  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StartPrefetching", "url",
+               url.spec());
+  if (!prefetch_manager_.get())  // Prefetching not enabled.
+    return;
+
+  std::vector<GURL> subresource_urls;
+  if (!GetPrefetchData(url, &subresource_urls)) {
+    // No prefetching data at host or URL level.
+    return;
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ResourcePrefetcherManager::MaybeAddPrefetch,
+                 prefetch_manager_, url, subresource_urls));
+}
+
+void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
+  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StopPrefetching", "url",
+               url.spec());
+  if (!prefetch_manager_.get())  // Not enabled.
+    return;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ResourcePrefetcherManager::MaybeRemovePrefetch,
+                 prefetch_manager_, url));
+}
+
 void ResourcePrefetchPredictor::Shutdown() {
   if (prefetch_manager_.get()) {
     prefetch_manager_->ShutdownOnUIThread();
     prefetch_manager_ = NULL;
   }
   history_service_observer_.RemoveAll();
 }
 
 void ResourcePrefetchPredictor::OnMainFrameRequest(
     const URLRequestSummary& request) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(INITIALIZED, initialization_state_);
 
-  StartPrefetching(request.navigation_id.main_frame_url);
+  const GURL& main_frame_url = request.navigation_id.main_frame_url;
+  StartPrefetching(main_frame_url);
 
   // Cleanup older navigations.
   CleanupAbandonedNavigations(request.navigation_id);
 
   // New empty navigation entry.
-  const GURL& initial_url = request.navigation_id.main_frame_url;
   inflight_navigations_.insert(
       std::make_pair(request.navigation_id,
-                     base::MakeUnique<PageRequestSummary>(initial_url)));
+                     base::MakeUnique<PageRequestSummary>(main_frame_url)));
 }
 
 void ResourcePrefetchPredictor::OnMainFrameResponse(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (initialization_state_ != INITIALIZED)
     return;
 
   StopPrefetching(response.navigation_id.main_frame_url);
 }
 
 void ResourcePrefetchPredictor::OnMainFrameRedirect(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  const GURL& main_frame_url = response.navigation_id.main_frame_url;
   std::unique_ptr<PageRequestSummary> summary;
   NavigationMap::iterator nav_it =
       inflight_navigations_.find(response.navigation_id);
   if (nav_it != inflight_navigations_.end()) {
     summary.reset(nav_it->second.release());
     inflight_navigations_.erase(nav_it);
   }
 
   // The redirect url may be empty if the URL was invalid.
   if (response.redirect_url.is_empty())
     return;
 
   // If we lost the information about the first hop for some reason.
   if (!summary) {
-    const GURL& initial_url = response.navigation_id.main_frame_url;
-    summary = base::MakeUnique<PageRequestSummary>(initial_url);
+    summary = base::MakeUnique<PageRequestSummary>(main_frame_url);
   }
 
   // A redirect will not lead to another OnMainFrameRequest call, so record the
   // redirect url as a new navigation id and save the initial url.
   NavigationID navigation_id(response.navigation_id);
   navigation_id.main_frame_url = response.redirect_url;
+  summary->main_frame_url = response.redirect_url;
   inflight_navigations_.insert(
       std::make_pair(navigation_id, std::move(summary)));
 }
 
 void ResourcePrefetchPredictor::OnSubresourceResponse(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   NavigationMap::const_iterator nav_it =
         inflight_navigations_.find(response.navigation_id);
   if (nav_it == inflight_navigations_.end()) {
     return;
   }
 
   nav_it->second->subresource_requests.push_back(response);
 }
 
 void ResourcePrefetchPredictor::OnNavigationComplete(
     const NavigationID& nav_id_without_timing_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   NavigationMap::iterator nav_it =
       inflight_navigations_.find(nav_id_without_timing_info);
   if (nav_it == inflight_navigations_.end())
     return;
 
-  const NavigationID navigation_id(nav_it->first);
-
   // Remove the navigation from the inflight navigations.
   std::unique_ptr<PageRequestSummary> summary = std::move(nav_it->second);
   inflight_navigations_.erase(nav_it);
 
   // Kick off history lookup to determine if we should record the URL.
   history::HistoryService* history_service =
       HistoryServiceFactory::GetForProfile(profile_,
                                            ServiceAccessType::EXPLICIT_ACCESS);
   DCHECK(history_service);
   history_service->ScheduleDBTask(
       std::unique_ptr<history::HistoryDBTask>(new GetUrlVisitCountTask(
-          navigation_id, std::move(summary),
+          std::move(summary),
           base::Bind(&ResourcePrefetchPredictor::OnVisitCountLookup,
                      AsWeakPtr()))),
       &history_lookup_consumer_);
 }
 
 bool ResourcePrefetchPredictor::GetPrefetchData(const GURL& main_frame_url,
                                                 std::vector<GURL>* urls) {
   DCHECK(urls);
   DCHECK(urls->empty());
   bool use_url_data = config_.IsPrefetchingEnabled(profile_) ?
@@ skipping 47 matching lines @@
         resource.number_of_hits() <
             config_.min_resource_hits_to_trigger_prefetch)
       continue;
 
     urls->push_back(GURL(resource.resource_url()));
   }
 
   return urls->size() > initial_size;
 }
 
-void ResourcePrefetchPredictor::StartPrefetching(const GURL& url) {
-  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StartPrefetching", "url",
-               url.spec());
-  if (!prefetch_manager_.get())  // Prefetching not enabled.
-    return;
-
-  std::vector<GURL> subresource_urls;
-  if (!GetPrefetchData(url, &subresource_urls)) {
-    // No prefetching data at host or URL level.
-    return;
-  }
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&ResourcePrefetcherManager::MaybeAddPrefetch,
-                 prefetch_manager_, url, subresource_urls));
-}
-
-void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
-  TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StopPrefetching", "url",
-               url.spec());
-  if (!prefetch_manager_.get())  // Not enabled.
-    return;
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&ResourcePrefetcherManager::MaybeRemovePrefetch,
-                 prefetch_manager_, url));
-}
-
 void ResourcePrefetchPredictor::StartInitialization() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   DCHECK_EQ(NOT_INITIALIZED, initialization_state_);
   initialization_state_ = INITIALIZING;
 
   // Create local caches using the database as loaded.
   auto url_data_map = base::MakeUnique<PrefetchDataMap>();
   auto host_data_map = base::MakeUnique<PrefetchDataMap>();
   auto url_redirect_data_map = base::MakeUnique<RedirectDataMap>();
@@ skipping 173 matching lines @@
 
   data_map->erase(key_to_delete);
   BrowserThread::PostTask(
       BrowserThread::DB, FROM_HERE,
       base::Bind(
           &ResourcePrefetchPredictorTables::DeleteSingleRedirectDataPoint,
           tables_, key_to_delete, key_type));
 }
 
 void ResourcePrefetchPredictor::OnVisitCountLookup(
-    size_t visit_count,
-    const NavigationID& navigation_id,
+    size_t url_visit_count,
     const PageRequestSummary& summary) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   UMA_HISTOGRAM_COUNTS("ResourcePrefetchPredictor.HistoryVisitCountForUrl",
-                       visit_count);
+                       url_visit_count);
 
   // TODO(alexilin): make only one request to DB thread.
 
   // URL level data - merge only if we already saved the data, or it
   // meets the cutoff requirement.
-  const std::string url_spec = navigation_id.main_frame_url.spec();
+  const std::string url_spec = summary.main_frame_url.spec();
   bool already_tracking = url_table_cache_->find(url_spec) !=
       url_table_cache_->end();
-  bool should_track_url = already_tracking ||
-      (visit_count >= config_.min_url_visit_count);
+  bool should_track_url =
+      already_tracking || (url_visit_count >= config_.min_url_visit_count);
 
   if (should_track_url && config_.IsURLLearningEnabled()) {
     LearnNavigation(url_spec, PREFETCH_KEY_TYPE_URL,
                     summary.subresource_requests, config_.max_urls_to_track,
                     url_table_cache_.get(), summary.initial_url.spec(),
                     url_redirect_table_cache_.get());
   }
 
   // Host level data - no cutoff, always learn the navigation if enabled.
   if (config_.IsHostLearningEnabled()) {
-    const std::string host = navigation_id.main_frame_url.host();
+    const std::string host = summary.main_frame_url.host();
     LearnNavigation(host, PREFETCH_KEY_TYPE_HOST, summary.subresource_requests,
                     config_.max_hosts_to_track, host_table_cache_.get(),
                     summary.initial_url.host(),
                     host_redirect_table_cache_.get());
   }
+
+  if (observer_)
+    observer_->OnNavigationLearned(url_visit_count, summary);
 }
 
 void ResourcePrefetchPredictor::LearnNavigation(
     const std::string& key,
     PrefetchKeyType key_type,
     const std::vector<URLRequestSummary>& new_resources,
     size_t max_data_map_size,
     PrefetchDataMap* data_map,
     const std::string& key_before_redirects,
     RedirectDataMap* redirect_map) {
@@ skipping 268 matching lines @@
   if (history_service->BackendLoaded()) {
     // HistoryService is already loaded. Continue with Initialization.
     OnHistoryAndCacheLoaded();
     return;
   }
   DCHECK(!history_service_observer_.IsObserving(history_service));
   history_service_observer_.Add(history_service);
   return;
 }
 
+void ResourcePrefetchPredictor::SetObserverForTesting(TestObserver* observer) {
+  if (observer_)
+    observer_->ResetResourcePrefetchPredictor();
+
+  DCHECK(!observer_);
+  DCHECK(observer->predictor_ == this);

[pasko, 2016/10/24 17:59:38]

DCHECKs in tests should be avoided. Also, this check is done at the cost of
having, ResourcePrefetchPredictor as a friend to the observer, which complicates
understanding the interfaces of both.

[alexilin, 2016/10/25 11:38:09]

What's wrong with DCHECKs in tests? It's one way to make sure that code works
correctly no matter whether it's test code or production.

[pasko, 2016/10/25 14:05:40]

Style guide: Don't use these macros in tests, as they crash the test binary and
leave bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros,
which report failures gracefully and can continue running other tests.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

[alexilin, 2016/10/25 15:10:58]

Yes, you are right. 
Although I can't say that this code is truly "test" code. I've found some places
in the codebase where DCHECK is called inside *ForTesting function but not
inside test file.
Evidence:
https://cs.chromium.org/chromium/src/base/threading/thread.cc?rcl=1477379968&...

[pasko, 2016/10/26 12:05:40]

This is not too bad, but bad :)

Within 4 lines of definition of various ForTesting functions, I found this many
DCHECKs:
shell> git grep -A 4 'ForTesting(.* {' | grep 'DCHECK(' | wc -l
54

Compared to overall amount of ForTesting definitions:
shell> git grep 'ForTesting(.* {' -- '*.h' '*.cc' | wc -l
795

This fraction is not negligible, people do DCHECKs in test code, but I still
think that removing it here is more beneficial than keeping it, and certainly
more consistent with the codebase overall.

[alexilin, 2016/10/26 13:09:59]

Acknowledged.

+  observer_ = observer;
+}
+
 }  // namespace predictors