[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 use a rather ugly workaround to achieve this: a HTML page
implementing a redirect is baked in using a data: scheme URL. This
allows the history backend (as well as server-side implementations)
to distinguish the two cases described above.

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. Use the referrer to expose details about which section of the NTP
   has been clicked: however referrers are required to use http/https
   or are cleared out otherwise.

Verified requirements:
- Neither dummy redirects (with data: scheme) nor article suggestions
  show up in NTP tiles (Most Visited).
- No additional network requests.
- Dummy redirects not visible in history.
- Dummy redirects not shown in omnibox autocomplete.
- Chrome History sync propagates the required information (to
  implement analogous server-side logic).

However, the dummy redirect URLs are temporarily user-visible, which
is undesirable.

BUG=645895

[mastiz, 2016-09-14 13:09:05]

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

[commit-bot: I haz the power, 2016-09-14 13:09:17]

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-14 13:11:27]

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

[mastiz, 2016-09-14 13:11:28]

Alternative solution to https://codereview.chromium.org/2338703003/

[Marc Treib, 2016-09-14 13:25:14]

Approach LGTM - I like this much better than the transition type qualifier. Not
pretty, but a much more localized hack.

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:207:
* data: scheme (which prevents articles from showing up in NTP tiles.
nit: missing closing paren

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:217:
if (wrappedUrl == null) return mUrl;
Under what circumstances would we expect this to happen? Do we need to handle it
somehow, or log failure, or crash?

[mastiz, 2016-09-14 13:46:21]

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:207:
* data: scheme (which prevents articles from showing up in NTP tiles.
On 2016/09/14 13:25:14, Marc Treib wrote:
> nit: missing closing paren

Done.

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:217:
if (wrappedUrl == null) return mUrl;
On 2016/09/14 13:25:14, Marc Treib wrote:
> Under what circumstances would we expect this to happen? Do we need to handle
it
> somehow, or log failure, or crash?

I would expect this to never happen since we're encoding using utf-8. I avoided
crashing in case we consider to merge this for M54 (falls back to behavior prior
to this CL). I'm fine if you propose to add logging for this, but eventually we
should change it to crash IMO.

[Marc Treib, 2016-09-14 13:47:55]

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:217:
if (wrappedUrl == null) return mUrl;
On 2016/09/14 13:46:21, mastiz wrote:
> On 2016/09/14 13:25:14, Marc Treib wrote:
> > Under what circumstances would we expect this to happen? Do we need to
handle
> it
> > somehow, or log failure, or crash?
> 
> I would expect this to never happen since we're encoding using utf-8. I
avoided
> crashing in case we consider to merge this for M54 (falls back to behavior
prior
> to this CL). I'm fine if you propose to add logging for this, but eventually
we
> should change it to crash IMO.

SGTM for merging, but then please add a TODO to change this later.

[Bernhard Bauer, 2016-09-14 13:53:08]

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:217:
if (wrappedUrl == null) return mUrl;
On 2016/09/14 13:25:14, Marc Treib wrote:
> Under what circumstances would we expect this to happen? Do we need to handle
it
> somehow, or log failure, or crash?

Yeah, I wouldn't expect UTF-8 encoding to fail, so I would handle this the same
way as a DCHECK failure in C++... which is not at all 😃

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

https://codereview.chromium.org/2342453003/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:501: // autocomplete and most
visited).
What does "Most Visited" here mean? Top Sites? Because for Most Likely we
definitely do want the entry, so we can filter the corresponding URL impression
out on the server.

[Marc Treib, 2016-09-14 13:55:05]

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

https://codereview.chromium.org/2342453003/diff/1/components/history/core/bro...
components/history/core/browser/history_backend.cc:501: // autocomplete and most
visited).
On 2016/09/14 13:53:08, Bernhard Bauer wrote:
> What does "Most Visited" here mean? Top Sites? Because for Most Likely we
> definitely do want the entry, so we can filter the corresponding URL
impression
> out on the server.

Right, the formulation could be clearer. "Such entries should not show up in NTP
tiles"? (Please avoid the term "most visited", it's way too overloaded.)

[mastiz, 2016-09-14 15:23:57]

On 2016/09/14 13:55:05, Marc Treib wrote:
>
https://codereview.chromium.org/2342453003/diff/1/components/history/core/bro...
> File components/history/core/browser/history_backend.cc (right):
> 
>
https://codereview.chromium.org/2342453003/diff/1/components/history/core/bro...
> components/history/core/browser/history_backend.cc:501: // autocomplete and
most
> visited).
> On 2016/09/14 13:53:08, Bernhard Bauer wrote:
> > What does "Most Visited" here mean? Top Sites? Because for Most Likely we
> > definitely do want the entry, so we can filter the corresponding URL
> impression
> > out on the server.
> 
> Right, the formulation could be clearer. "Such entries should not show up in
NTP
> tiles"? (Please avoid the term "most visited", it's way too overloaded.)

The term "most visited" has occurrences all over the place in history backend,
including comments as well as the public API (QueryMostVisitedURLs), so I would
prefer local consistency in this case.

[mastiz, 2016-09-14 15:42:51]

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2342453003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:217:
if (wrappedUrl == null) return mUrl;
On 2016/09/14 13:53:08, Bernhard Bauer wrote:
> On 2016/09/14 13:25:14, Marc Treib wrote:
> > Under what circumstances would we expect this to happen? Do we need to
handle
> it
> > somehow, or log failure, or crash?
> 
> Yeah, I wouldn't expect UTF-8 encoding to fail, so I would handle this the
same
> way as a DCHECK failure in C++... which is not at all 😃

Added TODO for now.

[mastiz, 2016-09-14 15:44:24]

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

[mastiz, 2016-09-14 15:44:25]

sky@chromium.org: Please review overall approach.

Solution competes with https://codereview.chromium.org/2338703003/

[Bernhard Bauer, 2016-09-14 15:45:22]

On 2016/09/14 15:23:57, mastiz wrote:
> On 2016/09/14 13:55:05, Marc Treib wrote:
> >
>
https://codereview.chromium.org/2342453003/diff/1/components/history/core/bro...
> > File components/history/core/browser/history_backend.cc (right):
> > 
> >
>
https://codereview.chromium.org/2342453003/diff/1/components/history/core/bro...
> > components/history/core/browser/history_backend.cc:501: // autocomplete and
> most
> > visited).
> > On 2016/09/14 13:53:08, Bernhard Bauer wrote:
> > > What does "Most Visited" here mean? Top Sites? Because for Most Likely we
> > > definitely do want the entry, so we can filter the corresponding URL
> > impression
> > > out on the server.
> > 
> > Right, the formulation could be clearer. "Such entries should not show up in
> NTP
> > tiles"? (Please avoid the term "most visited", it's way too overloaded.)
> 
> The term "most visited" has occurrences all over the place in history backend,
> including comments as well as the public API (QueryMostVisitedURLs), so I
would
> prefer local consistency in this case.

Note that the term used here is "MostVisitedURL", which would already be
marginally less confusing. But really, this is a problem that can be solved by
throwing more words at it: "...including omnibox suggestions and
MostVisitedURLs, i.e. Top Sites".

[sky, 2016-09-14 18:01:51]

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

https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:502: if (!has_redirects &&
Wouldn't this mean the page won't show up in history at all?

[mastiz, 2016-09-14 18:10:19]

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

https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:502: if (!has_redirects &&
On 2016/09/14 18:01:51, sky wrote:
> Wouldn't this mean the page won't show up in history at all?

The article itself (redirectee) gets listed in history, but not the dummy
'data:' redirect, according to my testing on Android. I don't however fully
understand why this case doesn't get reported as a single navigation chain (i.e.
single call to AddPage()).

[sky, 2016-09-14 18:17:49]

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

https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:502: if (!has_redirects &&
On 2016/09/14 18:10:18, mastiz wrote:
> On 2016/09/14 18:01:51, sky wrote:
> > Wouldn't this mean the page won't show up in history at all?
> 
> The article itself (redirectee) gets listed in history, but not the dummy
> 'data:' redirect, according to my testing on Android. I don't however fully
> understand why this case doesn't get reported as a single navigation chain
(i.e.
> single call to AddPage()).

I believe this change would make it so data urls are never added though, right?
That isn't the case today, which would mean for users that have data urls in top
sites they would no longer end up getting them (they would slowly expire).

[mastiz, 2016-09-14 18:22:03]

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

https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:502: if (!has_redirects &&
On 2016/09/14 18:17:49, sky wrote:
> On 2016/09/14 18:10:18, mastiz wrote:
> > On 2016/09/14 18:01:51, sky wrote:
> > > Wouldn't this mean the page won't show up in history at all?
> > 
> > The article itself (redirectee) gets listed in history, but not the dummy
> > 'data:' redirect, according to my testing on Android. I don't however fully
> > understand why this case doesn't get reported as a single navigation chain
> (i.e.
> > single call to AddPage()).
> 
> I believe this change would make it so data urls are never added though,
right?
> That isn't the case today, which would mean for users that have data urls in
top
> sites they would no longer end up getting them (they would slowly expire).

More specifically, data URLs with AUTO_BOOKMARK (note for example that clicks on
actual bookmarks are marked with LINK). Your point is still valid, but it should
be rare, and the server-side implementation of Chrome Suggestions discards such
scheme. WDYT?

[mastiz, 2016-09-14 18:40:48]

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

https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:502: if (!has_redirects &&
On 2016/09/14 18:22:02, mastiz wrote:
> On 2016/09/14 18:17:49, sky wrote:
> > On 2016/09/14 18:10:18, mastiz wrote:
> > > On 2016/09/14 18:01:51, sky wrote:
> > > > Wouldn't this mean the page won't show up in history at all?
> > > 
> > > The article itself (redirectee) gets listed in history, but not the dummy
> > > 'data:' redirect, according to my testing on Android. I don't however
fully
> > > understand why this case doesn't get reported as a single navigation chain
> > (i.e.
> > > single call to AddPage()).
> > 
> > I believe this change would make it so data urls are never added though,
> right?
> > That isn't the case today, which would mean for users that have data urls in
> top
> > sites they would no longer end up getting them (they would slowly expire).
> 
> More specifically, data URLs with AUTO_BOOKMARK (note for example that clicks
on
> actual bookmarks are marked with LINK). Your point is still valid, but it
should
> be rare, and the server-side implementation of Chrome Suggestions discards
such
> scheme. WDYT?

Also, would it be any better if we left FROM_ADDRESS_BAR cases unaffected?

[sky, 2016-09-14 21:19:06]

On Wed, Sep 14, 2016 at 11:22 AM,  <mastiz@chromium.org> wrote:
>
>
https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
> File components/history/core/browser/history_backend.cc (right):
>
>
https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
> components/history/core/browser/history_backend.cc:502: if
> (!has_redirects &&
> On 2016/09/14 18:17:49, sky wrote:
>> On 2016/09/14 18:10:18, mastiz wrote:
>> > On 2016/09/14 18:01:51, sky wrote:
>> > > Wouldn't this mean the page won't show up in history at all?
>> >
>> > The article itself (redirectee) gets listed in history, but not the
> dummy
>> > 'data:' redirect, according to my testing on Android. I don't
> however fully
>> > understand why this case doesn't get reported as a single navigation
> chain
>> (i.e.
>> > single call to AddPage()).
>>
>> I believe this change would make it so data urls are never added
> though, right?
>> That isn't the case today, which would mean for users that have data
> urls in top
>> sites they would no longer end up getting them (they would slowly
> expire).
>
> More specifically, data URLs with AUTO_BOOKMARK (note for example that
> clicks on actual bookmarks are marked with LINK). Your point is still
> valid, but it should be rare, and the server-side implementation of
> Chrome Suggestions discards such scheme. WDYT?

The server side implementation may ignore that, but not the local
implementation, right?

  -Scott
>
> https://codereview.chromium.org/2342453003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2016-09-14 22:38:52]

On 2016/09/14 21:19:06, sky wrote:
> On Wed, Sep 14, 2016 at 11:22 AM,  <mailto:mastiz@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
> > File components/history/core/browser/history_backend.cc (right):
> >
> >
>
https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
> > components/history/core/browser/history_backend.cc:502: if
> > (!has_redirects &&
> > On 2016/09/14 18:17:49, sky wrote:
> >> On 2016/09/14 18:10:18, mastiz wrote:
> >> > On 2016/09/14 18:01:51, sky wrote:
> >> > > Wouldn't this mean the page won't show up in history at all?
> >> >
> >> > The article itself (redirectee) gets listed in history, but not the
> > dummy
> >> > 'data:' redirect, according to my testing on Android. I don't
> > however fully
> >> > understand why this case doesn't get reported as a single navigation
> > chain
> >> (i.e.
> >> > single call to AddPage()).
> >>
> >> I believe this change would make it so data urls are never added
> > though, right?
> >> That isn't the case today, which would mean for users that have data
> > urls in top
> >> sites they would no longer end up getting them (they would slowly
> > expire).
> >
> > More specifically, data URLs with AUTO_BOOKMARK (note for example that
> > clicks on actual bookmarks are marked with LINK). Your point is still
> > valid, but it should be rare, and the server-side implementation of
> > Chrome Suggestions discards such scheme. WDYT?
> 
> The server side implementation may ignore that, but not the local
> implementation, right?

Correct, the local implementation of say Most Visited (aka Top Sites) would
(prior to to patch) surface a 'data:' URL.

> 
>   -Scott
> >
> > https://codereview.chromium.org/2342453003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-09-15 00:14:38]

On 2016/09/14 22:38:52, mastiz wrote:
> On 2016/09/14 21:19:06, sky wrote:
> > On Wed, Sep 14, 2016 at 11:22 AM,  <mailto:mastiz@chromium.org> wrote:
> > >
> > >
> >
>
https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
> > > File components/history/core/browser/history_backend.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/2342453003/diff/40001/components/history/core...
> > > components/history/core/browser/history_backend.cc:502: if
> > > (!has_redirects &&
> > > On 2016/09/14 18:17:49, sky wrote:
> > >> On 2016/09/14 18:10:18, mastiz wrote:
> > >> > On 2016/09/14 18:01:51, sky wrote:
> > >> > > Wouldn't this mean the page won't show up in history at all?
> > >> >
> > >> > The article itself (redirectee) gets listed in history, but not the
> > > dummy
> > >> > 'data:' redirect, according to my testing on Android. I don't
> > > however fully
> > >> > understand why this case doesn't get reported as a single navigation
> > > chain
> > >> (i.e.
> > >> > single call to AddPage()).
> > >>
> > >> I believe this change would make it so data urls are never added
> > > though, right?
> > >> That isn't the case today, which would mean for users that have data
> > > urls in top
> > >> sites they would no longer end up getting them (they would slowly
> > > expire).
> > >
> > > More specifically, data URLs with AUTO_BOOKMARK (note for example that
> > > clicks on actual bookmarks are marked with LINK). Your point is still
> > > valid, but it should be rare, and the server-side implementation of
> > > Chrome Suggestions discards such scheme. WDYT?
> > 
> > The server side implementation may ignore that, but not the local
> > implementation, right?
> 
> Correct, the local implementation of say Most Visited (aka Top Sites) would
> (prior to to patch) surface a 'data:' URL.
> 
> > 
> >   -Scott
> > >
> > > https://codereview.chromium.org/2342453003/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >

In thinking about the options we shouldn't have to update history at all for
this. I think the right place for not updating history is HistoryTabHelper.
HistoryTabHelper is the one that calls through to update history, is in chrome
code, and is the best place to ignore navigations we don't want persisted.