Change GetVisitCountToHost() to GetVisibleVisitCountToHost(), and restrict it...

Change GetVisitCountToHost() to GetVisibleVisitCountToHost(), and restrict it to returning "user-visible" visits.  This makes the Page Info bubble and the download system more restrictive about considering when someone "first visited" a site, so that they don't consider things like subframe navigations or redirects.

BUG=81741
TEST=Page Info bubble should not say you've visited a site before today if your only visits in previous days were subframe navigations.
Review URL: http://codereview.chromium.org/6990074

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@86695 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/download/download_history.cc

@@ -24,10 +24,10 @@ DownloadHistory::DownloadHistory(Profile* profile)
 }
 
 DownloadHistory::~DownloadHistory() {
-  // For any outstanding requests to HistoryService::GetVisitCountToHost(),
-  // since they'll be cancelled and thus not call back to
-  // OnGotVisitCountToHost(), we need to delete the associated
-  // VisitedBeforeDoneCallbacks.
+  // For any outstanding requests to
+  // HistoryService::GetVisibleVisitCountToHost(), since they'll be cancelled
+  // and thus not call back to OnGotVisitCountToHost(), we need to delete the
+  // associated VisitedBeforeDoneCallbacks.
   for (VisitedBeforeRequestsMap::iterator i(visited_before_requests_.begin());
        i != visited_before_requests_.end(); ++i)
     delete i->second.second;
@@ -55,9 +55,9 @@ void DownloadHistory::CheckVisitedReferrerBefore(
   if (referrer_url.is_valid()) {
     HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
     if (hs) {
-      HistoryService::Handle handle = hs->GetVisitCountToHost(referrer_url,
-          &history_consumer_,
-          NewCallback(this, &DownloadHistory::OnGotVisitCountToHost));
+      HistoryService::Handle handle =
+          hs->GetVisibleVisitCountToHost(referrer_url, &history_consumer_,
+              NewCallback(this, &DownloadHistory::OnGotVisitCountToHost));
       visited_before_requests_[handle] = std::make_pair(download_id, callback);
       return;
     }

chrome/browser/history/history.cc

@@ -593,12 +593,12 @@ HistoryService::Handle HistoryService::QueryRedirectsTo(
       new history::QueryRedirectsRequest(callback), to_url);
 }
 
-HistoryService::Handle HistoryService::GetVisitCountToHost(
+HistoryService::Handle HistoryService::GetVisibleVisitCountToHost(
     const GURL& url,
     CancelableRequestConsumerBase* consumer,
-    GetVisitCountToHostCallback* callback) {
-  return Schedule(PRIORITY_UI, &HistoryBackend::GetVisitCountToHost, consumer,
-      new history::GetVisitCountToHostRequest(callback), url);
+    GetVisibleVisitCountToHostCallback* callback) {
+  return Schedule(PRIORITY_UI, &HistoryBackend::GetVisibleVisitCountToHost,
+      consumer, new history::GetVisibleVisitCountToHostRequest(callback), url);
 }
 
 HistoryService::Handle HistoryService::QueryTopURLsAndRedirects(

chrome/browser/history/history.h

@@ -330,15 +330,17 @@ class HistoryService : public CancelableRequestProvider,
   typedef Callback4<Handle,
                     bool,        // Were we able to determine the # of visits?
                     int,         // Number of visits.
-                    base::Time>::Type  // Time of first visit. Only first bool
+                    base::Time>::Type  // Time of first visit. Only set if bool
                                        // is true and int is > 0.
-      GetVisitCountToHostCallback;
+      GetVisibleVisitCountToHostCallback;
 
-  // Requests the number of visits to all urls on the scheme/host/post
-  // identified by url. This is only valid for http and https urls.
-  Handle GetVisitCountToHost(const GURL& url,
-                             CancelableRequestConsumerBase* consumer,
-                             GetVisitCountToHostCallback* callback);
+  // Requests the number of user-visible visits (i.e. no redirects or subframes)
+  // to all urls on the same scheme/host/port as |url|.  This is only valid for
+  // HTTP and HTTPS URLs.
+  Handle GetVisibleVisitCountToHost(
+      const GURL& url,
+      CancelableRequestConsumerBase* consumer,
+      GetVisibleVisitCountToHostCallback* callback);
 
   // Called when QueryTopURLsAndRedirects completes. The vector contains a list
   // of the top |result_count| URLs.  For each of these URLs, there is an entry

chrome/browser/history/history_backend.cc

@@ -1312,16 +1312,16 @@ void HistoryBackend::QueryRedirectsTo(
       request->handle(), url, success, &request->value));
 }
 
-void HistoryBackend::GetVisitCountToHost(
-    scoped_refptr<GetVisitCountToHostRequest> request,
+void HistoryBackend::GetVisibleVisitCountToHost(
+    scoped_refptr<GetVisibleVisitCountToHostRequest> request,
     const GURL& url) {
   if (request->canceled())
     return;
   int count = 0;
   Time first_visit;
-  const bool success = (db_.get() && db_->GetVisitCountToHost(url, &count,
-                                                              &first_visit));
-  request->ForwardResult(GetVisitCountToHostRequest::TupleType(
+  const bool success = db_.get() &&
+      db_->GetVisibleVisitCountToHost(url, &count, &first_visit);
+  request->ForwardResult(GetVisibleVisitCountToHostRequest::TupleType(
       request->handle(), success, count, first_visit));
 }
 

chrome/browser/history/history_backend.h

@@ -148,8 +148,9 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   void QueryRedirectsTo(scoped_refptr<QueryRedirectsRequest> request,
                         const GURL& url);
 
-  void GetVisitCountToHost(scoped_refptr<GetVisitCountToHostRequest> request,
-                           const GURL& url);
+  void GetVisibleVisitCountToHost(
+      scoped_refptr<GetVisibleVisitCountToHostRequest> request,
+      const GURL& url);
 
   // TODO(Nik): remove. Use QueryMostVisitedURLs instead.
   void QueryTopURLsAndRedirects(

chrome/browser/history/history_marshaling.h

@@ -31,8 +31,8 @@ typedef CancelableRequest1<HistoryService::QueryRedirectsCallback,
                            history::RedirectList>
     QueryRedirectsRequest;
 
-typedef CancelableRequest<HistoryService::GetVisitCountToHostCallback>
-    GetVisitCountToHostRequest;
+typedef CancelableRequest<HistoryService::GetVisibleVisitCountToHostCallback>
+    GetVisibleVisitCountToHostRequest;
 
 typedef CancelableRequest1<HistoryService::QueryTopURLsAndRedirectsCallback,
                            Tuple2<std::vector<GURL>,

chrome/browser/history/visit_database.cc

-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -432,9 +432,9 @@ bool VisitDatabase::GetRedirectToVisit(VisitID to_visit,
   return true;
 }
 
-bool VisitDatabase::GetVisitCountToHost(const GURL& url,
-                                        int* count,
-                                        base::Time* first_visit) {
+bool VisitDatabase::GetVisibleVisitCountToHost(const GURL& url,
+                                               int* count,
+                                               base::Time* first_visit) {
   if (!url.SchemeIs(chrome::kHttpScheme) && !url.SchemeIs(chrome::kHttpsScheme))
     return false;
 
@@ -445,24 +445,29 @@ bool VisitDatabase::GetVisitCountToHost(const GURL& url,
   // The query becomes:
   // 'url >= http://google.com/' and url < http://google.com0'.
   // 0 is used as it is one character greater than '/'.
-  GURL search_url(url);
-  const std::string host_query_min = search_url.GetOrigin().spec();
-
+  const std::string host_query_min = url.GetOrigin().spec();
   if (host_query_min.empty())
     return false;
 
-  std::string host_query_max = host_query_min;
-  host_query_max[host_query_max.size() - 1] = '0';
-
+  // We also want to restrict ourselves to main frame navigations that are not
+  // in the middle of redirect chains, hence the transition checks.
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT MIN(v.visit_time), COUNT(*) "
       "FROM visits v INNER JOIN urls u ON v.url = u.id "
-      "WHERE (u.url >= ? AND u.url < ?)"));
+      "WHERE u.url >= ? AND u.url < ? "
+      "AND (transition & ?) != 0 "
+      "AND (transition & ?) NOT IN (?, ?, ?)"));
   if (!statement)
     return false;
 
   statement.BindString(0, host_query_min);
-  statement.BindString(1, host_query_max);
+  statement.BindString(1,
+      host_query_min.substr(0, host_query_min.size() - 1) + '0');
+  statement.BindInt(2, PageTransition::CHAIN_END);
+  statement.BindInt(3, PageTransition::CORE_MASK);
+  statement.BindInt(4, PageTransition::AUTO_SUBFRAME);
+  statement.BindInt(5, PageTransition::MANUAL_SUBFRAME);
+  statement.BindInt(6, PageTransition::KEYWORD_GENERATED);
 
   if (!statement.Step()) {
     // We've never been to this page before.

chrome/browser/history/visit_database.h

-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -134,13 +134,15 @@ class VisitDatabase {
                           VisitID* from_visit,
                           GURL* from_url);
 
-  // Returns the number of visits to all urls on the scheme/host/post
-  // identified by url. This is only valid for http and https urls (all other
-  // schemes are ignored and false is returned).
-  // count is set to the number of visits, first_visit is set to the first time
-  // the host was visited. Returns true on success.
-  bool GetVisitCountToHost(const GURL& url, int* count,
-                           base::Time* first_visit);
+  // Gets the number of user-visible visits to all URLs on the same
+  // scheme/host/port as |url|, as well as the time of the earliest visit.
+  // "User-visible" is defined as in GetVisibleVisitsInRange() above, i.e.
+  // excluding redirects and subframes.
+  // This function is only valid for HTTP and HTTPS URLs; all other schemes
+  // cause the function to return false.
+  bool GetVisibleVisitCountToHost(const GURL& url,
+                                  int* count,
+                                  base::Time* first_visit);
 
   // Get the time of the first item in our database.
   bool GetStartDate(base::Time* first_visit);

chrome/browser/page_info_model.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -269,7 +269,7 @@ PageInfoModel::PageInfoModel(Profile* profile,
   HistoryService* history = profile->GetHistoryService(
       Profile::EXPLICIT_ACCESS);
   if (show_history && history) {
-    history->GetVisitCountToHost(
+    history->GetVisibleVisitCountToHost(
         url,
         &request_consumer_,
         NewCallback(this, &PageInfoModel::OnGotVisitCountToHost));