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..6a8ffb0b967ee54733663a89b26592bf5d888e31 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -54,6 +54,39 @@ 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
+// 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) {

[jar (doing other things), 2013/06/17 22:58:14]

nit: How about at least not searching beyond max_top_url_count?

...or DCHECK that size() is always under it??

[bengr, 2013/06/17 23:46:59]

Done.

+    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])

[jar (doing other things), 2013/06/17 22:58:14]

I'm not sure how long these chains are.... but are you concerned about writing
code that does these linear searches?

This seems like it would be done a lot...

Could you mind histogramming how many URL compares we do here?  ... and do we do
this for every navigation to every URL??

[bengr, 2013/06/17 23:46:59]

From a random sampling of top sites, I'm seeing chains of size 1-4. This occurs
once per page load, but on the db thread. I considered using a hash, but this
seemed to be overkill for a metric that will be removed in M31. If you feel
otherwise, I could build a hash table. Also, do you really want me to histogram
the operation of code that supports a histogram? That seems like overkill to me,
but if you really want it, I'm ok with adding it.

On 2013/06/17 22:58:14, jar wrote:

[jar (doing other things), 2013/06/18 00:51:34]

This histogram will be hardly anything in performance compared with the scanning
you're doing.... assuming you call it only with the sum of the counts traversed
in a failure.

I just want to be sure we're not blowing performance out big time... and we have
a way to see what is happening.

If you change to using a map (as per later comments), I wouldn't be concerned
about perf here.

+        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|.
+// This can be removed for M31. (See issue 248761.)
+void RecordTopPageVisitStats(const history::MostVisitedURLList& top_urls,
+                             const GURL& url, int max_top_url_count) {

[jar (doing other things), 2013/06/17 22:58:14]

nit: one arg per line in declaration and definitions.

[bengr, 2013/06/17 23:46:59]

Done.

+  int rank = GetRankOfUrl(top_urls, url);
+  if (rank == -1)
+    rank = max_top_url_count;
+  UMA_HISTOGRAM_COUNTS("History.TopSitesVisitsByRank", rank);

[jar (doing other things), 2013/06/17 22:58:14]

Better would be to use:
UMA_HISTOGRAM_ENUMERATION("...", rank, max_top_url_count + 1);

[bengr, 2013/06/17 23:46:59]

Done.

+}
+
+}  // namespace
+#endif
+
 /* The HistoryBackend consists of a number of components:
 
     HistoryDatabase (stores past 3 months of history)
@@ -103,6 +136,11 @@ static const int kMaxRedirectCount = 32;
 // and is archived.
 static const int kArchiveDaysThreshold = 90;
 
+#if defined(OS_ANDROID)
+// The maximum number of top sites to track when recording top page visit stats.
+static const size_t kPageVisitStatsMaxTopSites = 50;

[jar (doing other things), 2013/06/17 22:58:14]

nit: (even though I like statics at file scope...) Anonymous namespaces are
preferred. 

This list of constants should probably be pushed higher in the file, (or your
new code later in the file), so that the new code can refer to these constants.

[bengr, 2013/06/17 23:46:59]

I moved just my constant to the top. 

On 2013/06/17 22:58:14, jar wrote:

+#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 +338,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(kPageVisitStatsMaxTopSites, kSegmentDataRetention,
+                           &most_visited_urls_);
+#endif
 }
 
 void HistoryBackend::SetOnBackendDestroyTask(base::MessageLoop* message_loop,
@@ -844,6 +886,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)
+    RecordTopPageVisitStats(most_visited_urls_, url,

[jar (doing other things), 2013/06/17 22:58:14]

nit: use curlies around any if action that runs onto more than a line.

[bengr, 2013/06/17 23:46:59]

Done.

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