Track fraction of visits to top URLs.

chrome/browser/history/history_backend.cc

 // Copyright (c) 2012 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/history/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 36 matching lines @@
 #include "sql/error_delegate_util.h"
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/history/android/android_provider_backend.h"
 #endif
 
 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)
       URLDatabase (stores a list of URLs)
       DownloadDatabase (stores a list of downloads)
       VisitDatabase (stores a list of visits for the URLs)
       VisitSegmentDatabase (stores groups of URLs for the most visited view).
 
     ArchivedDatabase (stores history older than 3 months)
       URLDatabase (stores a list of URLs)
@@ skipping 29 matching lines @@
 static const int kSessionCloseTimeWindowSecs = 10;
 
 // The maximum number of items we'll allow in the redirect list before
 // deleting some.
 static const int kMaxRedirectCount = 32;
 
 // The number of days old a history entry can be before it is considered "old"
 // 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,
                                   const RedirectList& redirects) {
   MostVisitedURL mv;
   mv.url = page_data.GetURL();
   mv.title = page_data.GetTitle();
   if (redirects.empty()) {
     // Redirects must contain at least the target url.
     mv.redirects.push_back(mv.url);
@@ skipping 177 matching lines @@
 #if defined(OS_ANDROID)
   file_util::Delete(GetAndroidCacheFileName(), false);
 #endif
 }
 
 void HistoryBackend::Init(const std::string& languages, bool force_fail) {
   if (!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,
                                              const base::Closure& task) {
   if (!backend_destroy_task_.is_null())
     DLOG(WARNING) << "Setting more than one destroy task, overriding";
   backend_destroy_message_loop_ = message_loop;
   backend_destroy_task_ = task;
 }
 
@@ skipping 524 matching lines @@
   // TODO(pkasting): http://b/1148304 We shouldn't be marking so many URLs as
   // typed, which would eliminate the need for this code.
   int typed_increment = 0;
   content::PageTransition transition_type =
       content::PageTransitionStripQualifier(transition);
   if ((transition_type == content::PAGE_TRANSITION_TYPED &&
       !content::PageTransitionIsRedirect(transition)) ||
       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);
   if (url_id) {
     // Update of an existing row.
     if (content::PageTransitionStripQualifier(transition) !=
         content::PAGE_TRANSITION_RELOAD)
       url_info.set_visit_count(url_info.visit_count() + 1);
     if (typed_increment)
       url_info.set_typed_count(url_info.typed_count() + typed_increment);
@@ skipping 2241 matching lines @@
   info.url_id = visit.url_id;
   info.time = visit.visit_time;
   info.transition = visit.transition;
   // If we don't have a delegate yet during setup or shutdown, we will drop
   // these notifications.
   if (delegate_)
     delegate_->NotifyVisitDBObserversOnAddVisit(info);
 }
 
 }  // namespace history