predictors: Make ResourcePrefetchPredictor observable.

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 b42d0243cada4466d95a4d58ed76bdcbf37a211e..058323202ae7e35551c0cf12b860b54fb3f1f797 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -39,6 +39,8 @@
 
 using content::BrowserThread;
 
+namespace predictors {
+
 namespace {
 
 // Sorted by decreasing likelihood according to HTTP archive.
@@ -67,9 +69,55 @@ float ComputeRedirectConfidence(const predictors::RedirectStat& redirect) {
          (redirect.number_of_hits() + redirect.number_of_misses());
 }
 
-}  // namespace
+// 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;
 
-namespace predictors {
+  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
 
 ////////////////////////////////////////////////////////////////////////////////
 // ResourcePrefetchPredictor static functions.
@@ -310,37 +358,34 @@ bool ResourcePrefetchPredictor::URLRequestSummary::SummarizeResponse(
   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());
-}
+ResourcePrefetchPredictor::PageRequestSummary::PageRequestSummary(
+    const GURL& i_main_frame_url)
+    : main_frame_url(i_main_frame_url), initial_url(i_main_frame_url) {}
 
-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;
-}
+ResourcePrefetchPredictor::PageRequestSummary::PageRequestSummary(
+    const PageRequestSummary& other)
+    : main_frame_url(other.main_frame_url),
+      initial_url(other.initial_url),
+      subresource_requests(other.subresource_requests) {}
 
-void ResourcePrefetchPredictor::GetUrlVisitCountTask::DoneRunOnMainThread() {
-  callback_.Run(visit_count_, navigation_id_, *summary_);
-}
+ResourcePrefetchPredictor::PageRequestSummary::~PageRequestSummary() {}
 
-ResourcePrefetchPredictor::GetUrlVisitCountTask::~GetUrlVisitCountTask() {}
+ResourcePrefetchPredictor::TestObserver::~TestObserver() {
+  ResetResourcePrefetchPredictor();
+}
 
-ResourcePrefetchPredictor::PageRequestSummary::PageRequestSummary(
-    const GURL& i_initial_url)
-    : initial_url(i_initial_url) {}
+ResourcePrefetchPredictor::TestObserver::TestObserver(
+    ResourcePrefetchPredictor* predictor)
+    : predictor_(predictor) {
+  predictor_->SetObserverForTesting(this);
+}
 
-ResourcePrefetchPredictor::PageRequestSummary::~PageRequestSummary() {}
+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.
@@ -349,6 +394,7 @@ ResourcePrefetchPredictor::ResourcePrefetchPredictor(
     const ResourcePrefetchPredictorConfig& config,
     Profile* profile)
     : profile_(profile),
+      observer_(nullptr),
       config_(config),
       initialization_state_(NOT_INITIALIZED),
       tables_(PredictorDatabaseFactory::GetForProfile(profile)
@@ -365,6 +411,8 @@ ResourcePrefetchPredictor::ResourcePrefetchPredictor(
 }
 
 ResourcePrefetchPredictor::~ResourcePrefetchPredictor() {
+  if (observer_)
+    observer_->ResetResourcePrefetchPredictor();
 }
 
 void ResourcePrefetchPredictor::RecordURLRequest(
@@ -419,6 +467,36 @@ void ResourcePrefetchPredictor::RecordMainFrameLoadComplete(
   }
 }
 
+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();
@@ -432,16 +510,16 @@ void ResourcePrefetchPredictor::OnMainFrameRequest(
   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(
@@ -457,6 +535,7 @@ 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);
@@ -471,14 +550,14 @@ void ResourcePrefetchPredictor::OnMainFrameRedirect(
 
   // 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)));
 }
@@ -505,8 +584,6 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
   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);
@@ -518,7 +595,7 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
   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_);
@@ -586,36 +663,6 @@ bool ResourcePrefetchPredictor::PopulatePrefetcherRequest(
   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);
 
@@ -809,23 +856,22 @@ void ResourcePrefetchPredictor::RemoveOldestEntryInRedirectDataMap(
 }
 
 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,
@@ -836,12 +882,15 @@ void ResourcePrefetchPredictor::OnVisitCountLookup(
 
   // 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(
@@ -1130,4 +1179,13 @@ void ResourcePrefetchPredictor::ConnectToHistoryService() {
   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