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 614d93a3bf4747e41f1816dff5f47841464e6e3e..1a4ca313b7d27d00c71ed10a9dcaf5fb09445c5f 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -54,6 +54,47 @@ using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
 
+#if defined(OS_ANDROID)
+namespace {
+
+#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/18 00:51:34]

nit: no need to for static, now that you're in the anonymous namespace.

[bengr, 2013/06/18 18:43:37]

Done.

+#endif
+
+// 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) {
+  DCHECK_LE(top_urls.size(), kPageVisitStatsMaxTopSites);
+  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|.
+// 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/18 00:51:34]

Why is this now passed as an argument, rather than consistently using
kPageVisitStatsMaxTopSites?  Using the constant ensure that we are safe in our
use of the histogram macro (args are constants, other than sample).

[bengr, 2013/06/18 18:43:37]

Done.

+  int rank = GetRankOfUrl(top_urls, url);
+  if (rank == -1)
+    rank = max_top_url_count;
+  UMA_HISTOGRAM_ENUMERATION("History.TopSitesVisitsByRank",
+                            rank, max_top_url_count + 1);
+}
+
+}  // namespace
+#endif
+
 /* The HistoryBackend consists of a number of components:
 
     HistoryDatabase (stores past 3 months of history)
@@ -103,6 +144,7 @@ static const int kMaxRedirectCount = 32;
 // and is archived.
 static const int kArchiveDaysThreshold = 90;
 
+

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

nit: How about coming these constants up to the top of the file in an anonymous
namespace?  You can then have your #ifdef android within that space, and the
constants would be nicely gathered together.

[bengr, 2013/06/18 18:43:37]

The histogram code I've added is temporary and once it's gone, all the remaining
constants will again be cleanly at the top of the file. I'd prefer not to move
the previously existing constants into the anonymous namespace that I added.

On 2013/06/18 00:51:34, jar wrote:

[jar (doing other things), 2013/06/19 00:49:58]

When editing files, it is always good to clean (mildly... *NOT* major risky
refactors, etc., or drastic style changes) while changing what should be
adjacent code.

If you landed local functions, your code should actually be landed between lines
121 and 123 of this file, rather than above the file level comments.  If you
land as methods, they should be aligned with their header declarations.

It is a bad idea to drop code into the "wrong place" and plan to "remove it
RSN," as too often, it stays much longer than thought :-/.

 // 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,
@@ -256,6 +298,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_);

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

Is this done exactly once per process run?  If so, it sure seems like it would
be nicer to put this into a map, and then have a quick single scan on each
navigation.

If this is updated constantly, as the "top URLs" change.... then it would not be
such a clear win.

[bengr, 2013/06/18 18:43:37]

Done.

+#endif
 }
 
 void HistoryBackend::SetOnBackendDestroyTask(base::MessageLoop* message_loop,
@@ -800,6 +846,16 @@ 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

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

Please indicate this in your histograms.xml prose.

[bengr, 2013/06/18 18:43:37]

Done.

+  // once when cycling through a redirect chain.
+  if (visit_source == SOURCE_BROWSED &&
+      (transition & content::PAGE_TRANSITION_CHAIN_END) != 0) {

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

It is interesting that you *only* check if we're at the end of a redirect
chain... but when scanning the URL list, you appear to (wastefully??) compare
with all entries in the redirect list.  Why is that?

Why not (for instance) only search if PAGE_TRANSITION_CHAIN_START, and then only
look at the first URL (and not any redirections??

[bengr, 2013/06/18 18:43:37]

Let's say the user commonly follows a redirect chain: a->b->c->d, but today
follows x->a->b->c->d. If I look for x in the top sites redirect chains, I won't
find it, even though x really means a visit to d, which is a top site. Therefore
I use the end of the chain for comparison.

And why not just look at the last entry in each top site's redirect chain?
Because afaik, the top sites chains do not guarantee an order afaik.


On 2013/06/18 00:51:34, jar wrote:

[jar (doing other things), 2013/06/19 00:49:58]

If there is a redirect chain... I sure hope it is consistent... and all roads
lead to the final link (with no redirects).  

Are you suggesting that sometimes there is an intentional redirect loop that
hopes across a bunch of sites, and then settles, after visiting (fetching)
several/all intermediaries?  That seems strange... but I'll leave it to you to
judge.

Thanks for your explanation.

[brettw, 2013/06/19 19:10:27]

Redirect chains should be ordered by time, so if it's in a vector, you should be
fine.

[bengr, 2013/06/19 19:42:58]

Good to know. I've switched over to using a map, so there aren't any wasteful
comparisons.

On 2013/06/19 19:10:27, brettw wrote:

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