[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 do this by introducing a new page transition qualifier, which marks
a visit as to-be-ignored. The qualifier is only used for NTP article
suggestions for now.

History sync is extended analogously to allow server-side generation of
Most Visited tiles adopt a similar filtering.

We've considered other competing approaches 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. Introduce a dedicated page transition type (split from
   AUTO_BOOKMARK): this is more intrusive and complex wrt to history
   sync, since older versions of Chrome would need to deserialize new
   protos as AUTO_BOOKMARK for backward compatibility.

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.

BUG=645895

[mastiz, 2016-09-13 13:02:09]

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

[commit-bot: I haz the power, 2016-09-13 13:02:22]

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-13 13:03:40]

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

[mastiz, 2016-09-13 13:03:41]

Please take a preliminary look modulo missing unit tests.

[Bernhard Bauer, 2016-09-13 13:14:40]

Looks good, just a naming suggestion:

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
File ui/base/page_transition_types.h (right):

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
ui/base/page_transition_types.h:112: // The visited URL won't contribute to Most
Visited tiles in the NTP.
Should we call this just "NTP tiles"? "Most Visited tiles" is so
similar-but-not-quite-identical to Most Likely that I would like to avoid it in
new code.

[Marc Treib, 2016-09-13 13:18:22]

I have another naming suggestion :)

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
File ui/base/page_transition_types.h (right):

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
ui/base/page_transition_types.h:113: PAGE_TRANSITION_IGNORE_FOR_NTP_MOST_VISITED
= 0x00400000,
This seems unnecessarily specific. Can we call it
PAGE_TRANSITION_CONTENT_SUGGESTIONS or something like that?

[finkm, 2016-09-13 13:33:47]

The CQ bit was unchecked by finkm@chromium.org

[finkm, 2016-09-13 13:46:23]

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

[commit-bot: I haz the power, 2016-09-13 13:46:42]

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-13 13:53:15]

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,
because of newly introduced article suggestions (aka snippets), which
shouldn't get surfaced as Most Visited tiles regardless of how many
times they were clicked.

We do this by introducing a new page transition qualifier, which marks
a visit as to-be-ignored.

History sync is extended analogously to allow server-side generation of
Most Visited tiles adopt a similar filtering.

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 do this by introducing a new page transition qualifier, which marks
a visit as to-be-ignored. The qualifier is only used for NTP article
suggestions for now.

History sync is extended analogously to allow server-side generation of
Most Visited tiles adopt a similar filtering.

We've considered other competing approaches 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. Introduce a dedicated page transition type (split from
   AUTO_BOOKMARK): this is more intrusive and complex wrt to history
   sync, since older versions of Chrome would need to deserialize new
   protos as AUTO_BOOKMARK for backward compatibility.

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.

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

[mastiz, 2016-09-13 13:53:15]

mastiz@chromium.org changed reviewers:
- finkm@chromium.org

[mastiz, 2016-09-13 14:06:24]

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

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

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-13 14:07:49]

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

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

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

PTAL. Tests added.

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
File ui/base/page_transition_types.h (right):

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
ui/base/page_transition_types.h:112: // The visited URL won't contribute to Most
Visited tiles in the NTP.
On 2016/09/13 13:14:40, Bernhard Bauer wrote:
> Should we call this just "NTP tiles"? "Most Visited tiles" is so
> similar-but-not-quite-identical to Most Likely that I would like to avoid it
in
> new code.

Done.

https://codereview.chromium.org/2338703003/diff/1/ui/base/page_transition_typ...
ui/base/page_transition_types.h:113: PAGE_TRANSITION_IGNORE_FOR_NTP_MOST_VISITED
= 0x00400000,
On 2016/09/13 13:18:20, Marc Treib wrote:
> This seems unnecessarily specific. Can we call it
> PAGE_TRANSITION_CONTENT_SUGGESTIONS or something like that?

As discussed offline: unless you feel strongly or someone elses agrees with this
renaming, I'll leave this unmodified. Rationale: avoid premature abstraction.

[mastiz, 2016-09-13 14:30:47]

FYI, I changed the logic in NewTabPage.java to set the qualifier only for the
ARTICLES section. Otherwise clicks on bookmarks would regress.

[mastiz, 2016-09-13 15:07: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 do this by introducing a new page transition qualifier, which marks
a visit as to-be-ignored. The qualifier is only used for NTP article
suggestions for now.

History sync is extended analogously to allow server-side generation of
Most Visited tiles adopt a similar filtering.

We've considered other competing approaches 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. Introduce a dedicated page transition type (split from
   AUTO_BOOKMARK): this is more intrusive and complex wrt to history
   sync, since older versions of Chrome would need to deserialize new
   protos as AUTO_BOOKMARK for backward compatibility.

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.

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 do this by introducing a new page transition qualifier, which marks
a visit as to-be-ignored. The qualifier is only used for NTP article
suggestions for now.

History sync is extended analogously to allow server-side generation of
Most Visited tiles adopt a similar filtering.

We've considered other competing approaches 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. Introduce a dedicated page transition type (split from
   AUTO_BOOKMARK): this is more intrusive and complex wrt to history
   sync, since older versions of Chrome would need to deserialize new
   protos as AUTO_BOOKMARK for backward compatibility.

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.

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

[Bernhard Bauer, 2016-09-13 15:17:38]

LGTM

https://codereview.chromium.org/2338703003/diff/40001/components/sessions/cor...
File components/sessions/core/serialized_navigation_entry_unittest.cc (right):

https://codereview.chromium.org/2338703003/diff/40001/components/sessions/cor...
components/sessions/core/serialized_navigation_entry_unittest.cc:229:
TEST(SerializedNavigationEntryTest, IgnoreForNtpTiles) {
Probably for a followup, but what would be more interesting than this would be
an instrumentation test that opens a snippet and verifies that the flag is set.

[mastiz, 2016-09-13 15:28:46]

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

[mastiz, 2016-09-13 15:28:46]

sky@chromium.org: please review the overall approach. Detailed discussion in
https://groups.google.com/a/google.com/forum/#!topic/chrome-sync-dev/iuO3dwLz-8Y

Rationale and some alternatives are summarized in the CL description. Let me
know if a VC would be helpful, since we're wishing to fix this issue for M54 in
some way.

[sky, 2016-09-13 19:12:23]

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

https://codereview.chromium.org/2338703003/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:359: (transition_type &
ui::PAGE_TRANSITION_IGNORE_FOR_NTP_TILES) == 0) {
Please add coverage of this to history related unittests.

[mastiz1, 2016-09-13 20:52:15]

mastiz@google.com changed reviewers:
+ mastiz@google.com

[mastiz1, 2016-09-13 20:52:16]

PTAL

...although an alternative promising idea has been brought up by bauerb@ and
requires investigation: the adoption of data: URLs with a redirect to the target
site. Will follow up on this alternative tomorrow.

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

https://codereview.chromium.org/2338703003/diff/60001/components/history/core...
components/history/core/browser/history_backend.cc:359: (transition_type &
ui::PAGE_TRANSITION_IGNORE_FOR_NTP_TILES) == 0) {
On 2016/09/13 19:12:23, sky wrote:
> Please add coverage of this to history related unittests.

Done.