Track fraction of visits to top URLs.

chrome/browser/history/history_backend.cc

Index: chrome/browser/history/history_backend.cc
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 26a51d0ab59e2f7b82fcefe85aced76798561b01..f8dffe21b4deeb06f2963eca25eb4e6b8ba2df8c 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -54,6 +54,36 @@ using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
 
+#if defined(OS_ANDROID)
+namespace {
+  // Return the rank of the url if it appears in |top_urls| (including within

[brettw, 2013/06/11 18:28:16]

Style: blank line before here, don't indent namespace contents.

[bengr, 2013/06/11 23:47:57]

Done.

+  // the associated redirect chains) and -1 if not.
+  int GetRankOfUrl(const history::MostVisitedURLList& top_urls,
+                   const GURL& url) {
+    for (size_t i = 0; i < top_urls.size(); ++i) {
+      if (url == top_urls[i].url)
+        return i;
+      for (size_t j = 0; j < top_urls[i].redirects.size(); ++j) {
+        if (url == top_urls[i].redirects[j])
+          return i;
+      }
+    }
+    return -1;
+  }
+
+  // Record counts of page visits by rank. If a url is not ranked, record the
+  // page visit in a slot corresponding to |max_top_url_count|, which should
+  // be one greater than the largest rank of any url in |top_urls|.
+  void RecordTopPageVisitStats(const history::MostVisitedURLList& top_urls,
+                               const GURL& url, int max_top_url_count) {
+    int rank = GetRankOfUrl(top_urls, url);
+    if (rank == -1)
+      rank = max_top_url_count;
+    UMA_HISTOGRAM_COUNTS("History.TopSitesVisitsByRank", rank);
+  }
+}  // namespace

[brettw, 2013/06/11 18:28:16]

Blank line before here.

[bengr, 2013/06/11 23:47:57]

Done.

+#endif
+
 /* The HistoryBackend consists of a number of components:
 
     HistoryDatabase (stores past 3 months of history)
@@ -103,6 +133,10 @@ static const int kMaxRedirectCount = 32;
 // and is archived.
 static const int kArchiveDaysThreshold = 90;
 
+#if defined(OS_ANDROID)
+static const size_t kMaxTopSites = 50;

[brettw, 2013/06/11 18:28:16]

Can this have a name more specific to your metrics? Otherwise this looks like a
general "how many top sites we have" thing.

[bengr, 2013/06/11 23:47:57]

Done.

+#endif
+
 // Converts from PageUsageData to MostVisitedURL. |redirects| is a
 // list of redirects for this URL. Empty list means no redirects.
 MostVisitedURL MakeMostVisitedURL(const PageUsageData& page_data,
@@ -300,6 +334,10 @@ void HistoryBackend::Init(const std::string& languages, bool force_fail) {
     InitImpl(languages);
   delegate_->DBLoaded(id_);
   typed_url_syncable_service_.reset(new TypedUrlSyncableService(this));
+#if defined(OS_ANDROID)
+  QueryMostVisitedURLsImpl(kMaxTopSites, kSegmentDataRetention,
+                           &most_visited_urls_);
+#endif
 }
 
 void HistoryBackend::SetOnBackendDestroyTask(base::MessageLoop* message_loop,
@@ -844,6 +882,15 @@ std::pair<URLID, VisitID> HistoryBackend::AddPageVisit(
       transition_type == content::PAGE_TRANSITION_KEYWORD_GENERATED)
     typed_increment = 1;
 
+#if defined(OS_ANDROID)
+  // Only count the page visit if it came from user browsing and only count it
+  // once when cycling through a redirect chain.
+  if (visit_source == SOURCE_BROWSED &&
+      (transition & content::PAGE_TRANSITION_CHAIN_END) != 0)

[brettw, 2013/06/11 18:28:16]

If you want only the last one, then this code should go in AddPage which calls
this function once for each redirect in the chain. Then you can remove the END
check.

[bengr, 2013/06/11 23:47:57]

If I understand the code, AddPage calls this AddPageVisit in two locations, one
for the no redirect case and on for the redirect case. If I move this code
there, I'd have to duplicate it. What do you prefer?

On 2013/06/11 18:28:16, brettw wrote:

+    RecordTopPageVisitStats(most_visited_urls_, url, kMaxTopSites);
+  // TODO(bengr): check referrer as well.

[brettw, 2013/06/11 18:28:16]

This TODO doesn't make any sense to me. Either remove it or expand on it for
somebody else reading can understand what you meant.

[bengr, 2013/06/11 23:47:57]

Done.

+#endif
+
   // See if this URL is already in the DB.
   URLRow url_info(url);
   URLID url_id = db_->GetRowForURL(url, &url_info);