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

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

There's a need to distinguish clicks on different elements on the NTP:
a) clicks on Most Visited tiles.
b) clicks on (newly introduced) article suggestions (aka snippets).

The first should contribute to Most Visited tiles (i.e. boost tiles
that have been clicked in the past). The second shouldn't.

We choose to achieve this by specifying a referrer for article
suggestion clicks. This exposes the referrer to third parties, which
has been discussed and considered a desirable feature.

The fix relies on such a workaround due to the current lack of
infrastructure to propagate opaque feature-specific data from upper
layers to navigation history (and sync).

The approach competes with more intrusive/controversial alternatives to
achieve the same:

1. Use page transition types (LINK vs AUTO_BOOKMARK) to distinguish
   tile clicks from article suggestion clicks: unfortunately both types
   have been used in the past (older versions of Chrome).

2. Introducing a new page transition type or qualifier: this can be
   considered a layering violation.

3. Introduce a dummy redirect by means of a data: schema page.

BUG=645895
Committed: https://crrev.com/30406bddaaa14549fdd5474ba0a411e5fb386f27
Cr-Commit-Position: refs/heads/master@{#419242}

[mastiz, 2016-09-15 14:20:46]

mastiz@chromium.org changed reviewers:
+ bauerb@chromium.org, treib@chromium.org

[mastiz, 2016-09-15 14:20:47]

Third attempt, now IMO cleaner after we relaxed the requirement to hide the
referrer from third parties.

Competes with:
https://codereview.chromium.org/2338703003/
https://codereview.chromium.org/2342453003/

[mastiz, 2016-09-15 14:21:00]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 14:21:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2016-09-15 14:31:23]

Code LGTM, if we're okay with exposing this to third parties.

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if
(article.mCategory == KnownCategories.ARTICLES) {
In the longer term, we need to decide for which categories we want to set this -
at the very least, we should set it for all remote categories. But for now this
is okay.

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:75: const char
kChromeSearchContentSuggestionsReferrer[] =
Why "Search"?

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

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if
(article.mCategory == KnownCategories.ARTICLES) {
On 2016/09/15 14:31:22, Marc Treib wrote:
> In the longer term, we need to decide for which categories we want to set this
-
> at the very least, we should set it for all remote categories. But for now
this
> is okay.

Acknowledged. I can a switch statement if you want.

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:75: const char
kChromeSearchContentSuggestionsReferrer[] =
On 2016/09/15 14:31:22, Marc Treib wrote:
> Why "Search"?

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:56]

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if
(article.mCategory == KnownCategories.ARTICLES) {
On 2016/09/15 14:37:56, mastiz wrote:
> On 2016/09/15 14:31:22, Marc Treib wrote:
> > In the longer term, we need to decide for which categories we want to set
this
> -
> > at the very least, we should set it for all remote categories. But for now
> this
> > is okay.
> 
> Acknowledged. I can a switch statement if you want.

It can't be a switch, because remote categories aren't necessarily known.
Maybe just add a TODO for now?

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:75: const char
kChromeSearchContentSuggestionsReferrer[] =
On 2016/09/15 14:37:56, mastiz wrote:
> On 2016/09/15 14:31:22, Marc Treib wrote:
> > Why "Search"?
> 
> Made this consistent with similar definitions in url_constants.h, like
> kChromeSearchLocalNtpUrl. Would you prefer if I strip it out?

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]

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:324: if
(article.mCategory == KnownCategories.ARTICLES) {
On 2016/09/15 14:50:55, Marc Treib wrote:
> On 2016/09/15 14:37:56, mastiz wrote:
> > On 2016/09/15 14:31:22, Marc Treib wrote:
> > > In the longer term, we need to decide for which categories we want to set
> this
> > -
> > > at the very least, we should set it for all remote categories. But for now
> > this
> > > is okay.
> > 
> > Acknowledged. I can a switch statement if you want.
> 
> It can't be a switch, because remote categories aren't necessarily known.
> Maybe just add a TODO for now?

Done.

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:75: const char
kChromeSearchContentSuggestionsReferrer[] =
On 2016/09/15 14:50:55, Marc Treib wrote:
> On 2016/09/15 14:37:56, mastiz wrote:
> > On 2016/09/15 14:31:22, Marc Treib wrote:
> > > Why "Search"?
> > 
> > Made this consistent with similar definitions in url_constants.h, like
> > kChromeSearchLocalNtpUrl. Would you prefer if I strip it out?
> 
> The local NTP is actually served at the chrome-search:// scheme. Our stuff
> isn't, so yes, I'd strip it out.

Done.

[mastiz, 2016-09-15 15:08:28]

mastiz@chromium.org changed reviewers:
+ sky@chromium.org

[mastiz, 2016-09-15 15:08:28]

sky@chromium.org: Please review overall approach.

This seems a cleanest approach so far, now that exposing the referrer to third
parties has been considered acceptable (even desired!).

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

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new
Referrer("https://www.googleapis.com/auth/chrome-content-suggestions",
Can you make this a constant?

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:353: referrer !=
GURL(kChromeSearchContentSuggestionsReferrer))) &&
Per https://codereview.chromium.org/2342453003/#msg22, maybe you should do this
in HistoryTabHelper instead?

[Marc Treib, 2016-09-15 15:19:21]

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new
Referrer("https://www.googleapis.com/auth/chrome-content-suggestions",
Ah, actually one more nit: This is the scope for OAuth tokens; it's not the URL
for the endpoint itself. Is that on purpose? Maybe the actual endpoint
(https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/fetch) would
be slightly less of a lie? :D

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

PTAL.

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new
Referrer("https://www.googleapis.com/auth/chrome-content-suggestions",
On 2016/09/15 15:10:52, Bernhard Bauer wrote:
> Can you make this a constant?

Done.

https://codereview.chromium.org/2338133006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:326: new
Referrer("https://www.googleapis.com/auth/chrome-content-suggestions",
On 2016/09/15 15:19:21, Marc Treib wrote:
> Ah, actually one more nit: This is the scope for OAuth tokens; it's not the
URL
> for the endpoint itself. Is that on purpose? Maybe the actual endpoint
> (https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/fetch)
would
> be slightly less of a lie? :D

Acknowledged. Although you're right about the usefulness of curious users, as
discussed offline, this URL has been chosen because it's expected to be very
stable.

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:353: referrer !=
GURL(kChromeSearchContentSuggestionsReferrer))) &&
On 2016/09/15 15:10:52, Bernhard Bauer wrote:
> Per https://codereview.chromium.org/2342453003/#msg22, maybe you should do
this
> in HistoryTabHelper instead?

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]

Java LGTM.

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:353: referrer !=
GURL(kChromeSearchContentSuggestionsReferrer))) &&
On 2016/09/15 15:31:57, mastiz wrote:
> On 2016/09/15 15:10:52, Bernhard Bauer wrote:
> > Per https://codereview.chromium.org/2342453003/#msg22, maybe you should do
> this
> > in HistoryTabHelper instead?
> 
> 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.

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.

[sky, 2016-09-15 17:46:26]

https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:76:
"https://www.googleapis.com/auth/chrome-content-suggestions";
I like this approach much better, but I don't want to bake into history_backend
this url. Can you add a boolean to HistoryAddPageArgs indicating if the page
should contribute to the ntp and change HistoryTabHelper to set the value based
on the url?

[mastiz, 2016-09-15 18:12:07]

https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:76:
"https://www.googleapis.com/auth/chrome-content-suggestions";
On 2016/09/15 17:46:26, sky wrote:
> I like this approach much better, but I don't want to bake into
history_backend
> this url. Can you add a boolean to HistoryAddPageArgs indicating if the page
> should contribute to the ntp and change HistoryTabHelper to set the value
based
> on the url?

Would you be fine if we instead verify the referrer is empty? This is what the
server-side implementation does, and after all, I expect TYPED and AUTO_BOOKMARK
always contain an empty referer? (based on the fact, that referrer sanitization
requires http[s] schema)

I avoided this in the first place to reduce the risk for regressions.

[sky, 2016-09-15 20:07:06]

On 2016/09/15 18:12:07, mastiz wrote:
>
https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
> File components/history/core/browser/history_backend.cc (right):
> 
>
https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
> components/history/core/browser/history_backend.cc:76:
> "https://www.googleapis.com/auth/chrome-content-suggestions";
> On 2016/09/15 17:46:26, sky wrote:
> > I like this approach much better, but I don't want to bake into
> history_backend
> > this url. Can you add a boolean to HistoryAddPageArgs indicating if the page
> > should contribute to the ntp and change HistoryTabHelper to set the value
> based
> > on the url?
> 
> Would you be fine if we instead verify the referrer is empty? This is what the
> server-side implementation does, and after all, I expect TYPED and
AUTO_BOOKMARK
> always contain an empty referer? (based on the fact, that referrer
sanitization
> requires http[s] schema)
> 
> I avoided this in the first place to reduce the risk for regressions.

I don't like making assumptions about how the types are used and would prefer we
have the check in HistoryTabHelper like I outlined.

[mastiz, 2016-09-15 21:26:36]

On 2016/09/15 20:07:06, sky wrote:
> On 2016/09/15 18:12:07, mastiz wrote:
> >
>
https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
> > File components/history/core/browser/history_backend.cc (right):
> > 
> >
>
https://codereview.chromium.org/2338133006/diff/40001/components/history/core...
> > components/history/core/browser/history_backend.cc:76:
> > "https://www.googleapis.com/auth/chrome-content-suggestions";
> > On 2016/09/15 17:46:26, sky wrote:
> > > I like this approach much better, but I don't want to bake into
> > history_backend
> > > this url. Can you add a boolean to HistoryAddPageArgs indicating if the
page
> > > should contribute to the ntp and change HistoryTabHelper to set the value
> > based
> > > on the url?
> > 
> > Would you be fine if we instead verify the referrer is empty? This is what
the
> > server-side implementation does, and after all, I expect TYPED and
> AUTO_BOOKMARK
> > always contain an empty referer? (based on the fact, that referrer
> sanitization
> > requires http[s] schema)
> > 
> > I avoided this in the first place to reduce the risk for regressions.
> 
> I don't like making assumptions about how the types are used and would prefer
we
> have the check in HistoryTabHelper like I outlined.

Done as suggested, PTAL.

I didn't expose the boolean in HistoryService's AddPage() variant with multiple
arguments. Let me know if you'd like me to do that (quite some clients would
need to be updated). Besides, there's one more caller of
UpdateHistoryForNavigation which is left unmodified (I believe it's irrelevant).

I would personally prefer keeping things simpler and requiring an empty referrer
as proposed above, which IMO fits nicely the idea of Most Visited. However, as
discussed offline, I can see the simplicity of using transition types only too.

[sky, 2016-09-15 22:56:10]

Just minor stuff.

https://codereview.chromium.org/2338133006/diff/60001/chrome/browser/history/...
File chrome/browser/history/history_tab_helper.cc (right):

https://codereview.chromium.org/2338133006/diff/60001/chrome/browser/history/...
chrome/browser/history/history_tab_helper.cc:69: const bool
skip_for_ntp_most_visited =
Please document why we want this skipped.

https://codereview.chromium.org/2338133006/diff/60001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2338133006/diff/60001/components/history/core...
components/history/core/browser/history_types.h:386: bool
skip_for_ntp_most_visited);
Variables with negative names in them are harder to understand. Please use
something like include_in_ntp_most_visited. You can also use a default value for
it if you want. Lastly, make the parameter here match the name of the field.

[mastiz, 2016-09-16 08:42:10]

PTAL.

Let me highlight one different wrt to the original proposal: we now skip the
call to UpdateSegments() altogether, as opposed to filtering similarly as page
transition types are within UpdateSegments().

My understanding is that this is fine, and that's also what my tests suggest.

https://codereview.chromium.org/2338133006/diff/60001/chrome/browser/history/...
File chrome/browser/history/history_tab_helper.cc (right):

https://codereview.chromium.org/2338133006/diff/60001/chrome/browser/history/...
chrome/browser/history/history_tab_helper.cc:69: const bool
skip_for_ntp_most_visited =
On 2016/09/15 22:56:10, sky wrote:
> Please document why we want this skipped.

Done.

https://codereview.chromium.org/2338133006/diff/60001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2338133006/diff/60001/components/history/core...
components/history/core/browser/history_types.h:386: bool
skip_for_ntp_most_visited);
On 2016/09/15 22:56:10, sky wrote:
> Variables with negative names in them are harder to understand. Please use
> something like include_in_ntp_most_visited. You can also use a default value
for
> it if you want. Lastly, make the parameter here match the name of the field.

Done. I avoided the default value for consistency.

FYI, the reason why I negated the logic was to keep 'false' as default value,
which I liked in case the code evolves to use C++11 list initialization.

Besides, to double check, as mentioned in my previous reply, note that I didn't
extend the signature of AddPage() for the overload with all inlined args (would
require updating many clients).

[sky, 2016-09-16 17:18:52]

LGTM with the following addressed. Thanks for your patience and finding a good
solution.

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:595: // Update the segment
for this visit.
This comment belongs inside the if, but the comment directly documents what a
single function call does, so just remove it.

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
components/history/core/browser/history_types.h:399: bool
consider_for_ntp_most_visited;
Document what this means (as it isn't readily obvious).

[mastiz, 2016-09-16 17:58:21]

All comments addressed. Thanks for your reviews and guidance, as well as the
back and forth with various competing approaches!

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:595: // Update the segment
for this visit.
On 2016/09/16 17:18:51, sky wrote:
> This comment belongs inside the if, but the comment directly documents what a
> single function call does, so just remove it.

Done.

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
File components/history/core/browser/history_types.h (right):

https://codereview.chromium.org/2338133006/diff/80001/components/history/core...
components/history/core/browser/history_types.h:399: bool
consider_for_ntp_most_visited;
On 2016/09/16 17:18:51, sky wrote:
> Document what this means (as it isn't readily obvious).

Done.

[mastiz, 2016-09-16 17:59:58]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-09-16 17:59:59]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2338133006/#ps100001
(title: "Addressed comments: removed obvious comment and added docs for flag.")

[commit-bot: I haz the power, 2016-09-16 18:00:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 18:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 18:16:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2016-09-16 18:27:26]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 18:27:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2016-09-16 19:08:26]

The CQ bit was unchecked by mastiz@chromium.org

[mastiz, 2016-09-16 19:08:39]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-09-16 19:08:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
treib@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2338133006/#ps120001
(title: "Updated trivial occurrence in unit test.")

[commit-bot: I haz the power, 2016-09-16 19:09:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 19:16:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-16 19:18:31]

Description was changed from

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

There's a need to distinguish clicks on different elements on the NTP:
a) clicks on Most Visited tiles.
b) clicks on (newly introduced) article suggestions (aka snippets).

The first should contribute to Most Visited tiles (i.e. boost tiles
that have been clicked in the past). The second shouldn't.

We choose to achieve this by specifying a referrer for article
suggestion clicks. This exposes the referrer to third parties, which
has been discussed and considered a desirable feature.

The fix relies on such a workaround due to the current lack of
infrastructure to propagate opaque feature-specific data from upper
layers to navigation history (and sync).

The approach competes with more intrusive/controversial alternatives to
achieve the same:

1. Use page transition types (LINK vs AUTO_BOOKMARK) to distinguish
   tile clicks from article suggestion clicks: unfortunately both types
   have been used in the past (older versions of Chrome).

2. Introducing a new page transition type or qualifier: this can be
   considered a layering violation.

3. Introduce a dummy redirect by means of a data: schema page.

BUG=645895
==========

to

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

There's a need to distinguish clicks on different elements on the NTP:
a) clicks on Most Visited tiles.
b) clicks on (newly introduced) article suggestions (aka snippets).

The first should contribute to Most Visited tiles (i.e. boost tiles
that have been clicked in the past). The second shouldn't.

We choose to achieve this by specifying a referrer for article
suggestion clicks. This exposes the referrer to third parties, which
has been discussed and considered a desirable feature.

The fix relies on such a workaround due to the current lack of
infrastructure to propagate opaque feature-specific data from upper
layers to navigation history (and sync).

The approach competes with more intrusive/controversial alternatives to
achieve the same:

1. Use page transition types (LINK vs AUTO_BOOKMARK) to distinguish
   tile clicks from article suggestion clicks: unfortunately both types
   have been used in the past (older versions of Chrome).

2. Introducing a new page transition type or qualifier: this can be
   considered a layering violation.

3. Introduce a dummy redirect by means of a data: schema page.

BUG=645895

Committed: https://crrev.com/30406bddaaa14549fdd5474ba0a411e5fb386f27
Cr-Commit-Position: refs/heads/master@{#419242}
==========

[commit-bot: I haz the power, 2016-09-16 19:18:32]

Patchset 7 (id:??) landed as
https://crrev.com/30406bddaaa14549fdd5474ba0a411e5fb386f27
Cr-Commit-Position: refs/heads/master@{#419242}