[NTP] Fix article suggestion clicks contributing to Most Visited tiles

components/history/core/browser/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 "components/history/core/browser/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 53 matching lines @@
       VisitDatabase (stores a list of visits for the URLs)
       VisitSegmentDatabase (stores groups of URLs for the most visited view).
 
     ExpireHistoryBackend (manages deleting things older than 3 months)
 */
 
 namespace history {
 
 namespace {
 
+// Referrer used for clicks on article suggestions on the NTP.
+const char kChromeSearchContentSuggestionsReferrer[] =

[Marc Treib, 2016/09/15 14:31:22]

Why "Search"?

[mastiz, 2016/09/15 14:37:56]

Made this consistent with similar definitions in url_constants.h, like
kChromeSearchLocalNtpUrl. Would you prefer if I strip it out?

[Marc Treib, 2016/09/15 14:50:55]

The local NTP is actually served at the chrome-search:// scheme. Our stuff
isn't, so yes, I'd strip it out.

[mastiz, 2016/09/15 15:06:16]

Done.

+    "https://www.googleapis.com/auth/chrome-content-suggestions";
+
 void RunUnlessCanceled(
     const base::Closure& closure,
     const base::CancelableTaskTracker::IsCanceledCallback& is_canceled) {
   if (!is_canceled.Run())
     closure.Run();
 }
 
 }  // namespace
 
 // How long we'll wait to do a commit, so that things are batched together.
@@ skipping 224 matching lines @@
     if (visit_set.find(visit_id) != visit_set.end()) {
       NOTREACHED() << "Loop in referer chain, giving up";
       break;
     }
     visit_set.insert(visit_id);
   }
   return 0;
 }
 
 SegmentID HistoryBackend::UpdateSegments(const GURL& url,
+                                         const GURL& referrer,
                                          VisitID from_visit,
                                          VisitID visit_id,
                                          ui::PageTransition transition_type,
                                          const Time ts) {
   if (!db_)
     return 0;
 
   // We only consider main frames.
   if (!ui::PageTransitionIsMainFrame(transition_type))
     return 0;
 
   SegmentID segment_id = 0;
 
   // Are we at the beginning of a new segment?
   // Note that navigating to an existing entry (with back/forward) reuses the
   // same transition type.  We are not adding it as a new segment in that case
   // because if this was the target of a redirect, we might end up with
   // 2 entries for the same final URL. Ex: User types google.net, gets
   // redirected to google.com. A segment is created for google.net. On
   // google.com users navigates through a link, then press back. That last
   // navigation is for the entry google.com transition typed. We end up adding
   // a segment for that one as well. So we end up with google.net and google.com
   // in the segment table, showing as 2 entries in the NTP.
   // Note also that we should still be updating the visit count for that segment
   // which we are not doing now. It should be addressed when
   // http://crbug.com/96860 is fixed.
   if ((ui::PageTransitionCoreTypeIs(transition_type,
                                     ui::PAGE_TRANSITION_TYPED) ||
-       ui::PageTransitionCoreTypeIs(transition_type,
-                                    ui::PAGE_TRANSITION_AUTO_BOOKMARK)) &&
+       (ui::PageTransitionCoreTypeIs(transition_type,
+                                     ui::PAGE_TRANSITION_AUTO_BOOKMARK) &&
+        referrer != GURL(kChromeSearchContentSuggestionsReferrer))) &&

[Bernhard Bauer, 2016/09/15 15:10:52]

Per https://codereview.chromium.org/2342453003/#msg22, maybe you should do this
in HistoryTabHelper instead?

[mastiz, 2016/09/15 15:31:57]

The rationale doesn't apply here: we're not trying to avoid persisting the visit
the article altogether (e.g. for omnibox autocomplete), only it's presence in
the segment DB for NTP tiles.

[Bernhard Bauer, 2016/09/15 16:13:07]

I would argue there's still a difference between doing this in the history
component layer vs. the Chrome layer, but I'll defer to sky@ anyway.

       (transition_type & ui::PAGE_TRANSITION_FORWARD_BACK) == 0) {
     // If so, create or get the segment.
     std::string segment_name = db_->ComputeSegmentName(url);
     URLID url_id = db_->GetRowForURL(url, nullptr);
     if (!url_id)
       return 0;
 
     segment_id = db_->GetSegmentNamed(segment_name);
     if (!segment_id) {
       segment_id = db_->CreateSegment(url_id, segment_name);
@@ skipping 158 matching lines @@
         ui::PAGE_TRANSITION_CHAIN_END);
 
     // No redirect case (one element means just the page itself).
     last_ids = AddPageVisit(request.url, request.time, last_ids.second, t,
                             request.visit_source);
 
     // Update the segment for this visit. KEYWORD_GENERATED visits should not
     // result in changing most visited, so we don't update segments (most
     // visited db).
     if (!is_keyword_generated) {
-      UpdateSegments(request.url, from_visit_id, last_ids.second, t,
-                     request.time);
+      UpdateSegments(request.url, request.referrer, from_visit_id,
+                     last_ids.second, t, request.time);
 
       // Update the referrer's duration.
       UpdateVisitDuration(from_visit_id, request.time);
     }
   } else {
     // Redirect case. Add the redirect chain.
 
     ui::PageTransition redirect_info = ui::PAGE_TRANSITION_CHAIN_START;
 
     RedirectList redirects = request.redirects;
@@ skipping 48 matching lines @@
         t = ui::PageTransitionFromInt(t | ui::PAGE_TRANSITION_CHAIN_END);
       }
 
       // Record all redirect visits with the same timestamp. We don't display
       // them anyway, and if we ever decide to, we can reconstruct their order
       // from the redirect chain.
       last_ids = AddPageVisit(redirects[redirect_index], request.time,
                               last_ids.second, t, request.visit_source);
       if (t & ui::PAGE_TRANSITION_CHAIN_START) {
         // Update the segment for this visit.
-        UpdateSegments(redirects[redirect_index], from_visit_id,
-                       last_ids.second, t, request.time);
+        UpdateSegments(redirects[redirect_index], request.referrer,
+                       from_visit_id, last_ids.second, t, request.time);
 
         // Update the visit_details for this visit.
         UpdateVisitDuration(from_visit_id, request.time);
       }
 
       // Subsequent transitions in the redirect list must all be server
       // redirects.
       redirect_info = ui::PAGE_TRANSITION_SERVER_REDIRECT;
     }
 
@@ skipping 2040 matching lines @@
   // transaction is currently open.
   db_->CommitTransaction();
   db_->Vacuum();
   db_->BeginTransaction();
   db_->GetStartDate(&first_recorded_time_);
 
   return true;
 }
 
 }  // namespace history