I don't think we need to componentize site engagement to do this. Instead, could we define an EngagementService abstract class in MostVisitedSites, and have the SiteEngagementService subclass that? When constructing the MostVisitedSites object it can be passed the concrete SiteEngagementService object as the implementation of the abstract class.

We already have an abstract class SiteEngagementScoreProvider that would be ideal for this purpose: it could be moved into MostVisitedSites.

Sounds good high-level, although SiteEngagementScoreProvider doesn't seem very appropriate. What MostVisitedSites needs is a function that will return a list of sites (ideally sorted), similar (I believe) to SiteEngagementService::GetAllDetails().

Another alternative is that the TopSites interface is reused, by implementing an adapter for SiteEngagementService, which would avoid changes (and additional complexity) in MostVisitedSites.

One thing to keep in mind is blacklisting, i.e. users manually removing tiles from the NTP. TopSites handles (and persists) that internally, so we'd need something equivalent for SiteEngagementService. The adapter mentioned above would be a good place to implement this.

After a cursory look, it seems that TopSites::GetMostVisitedURLs() is the key method call returning a sorted list of top sites? I think we could either:

1. plumb the flag right into TopSitesImpl, and have it conditionally re-order the MostVisitedURLList based on engagement when GetMostVisitedURLs() is called if the flag is on (it's just a case of calling std::sort on that vector with a functor querying the list of all origins + engagement scores).

2. subclass TopSitesImpl (so we don't have to reimplement all of the blacklisting and persistence logic), and override GetMostVisitedURLs() to do the reordering based on engagement.

What do you think? Both of these are pretty similar, and I think 1 is simplest. Let me know if I've mistaken the methods here. :)

1. seems simpler to me, though TopSitesImpl is quite convoluted with its several caches and databases. Not sure how easy it'd actually be in the end.
Also, if we want to support desktop with this, we need to make sure that the thumbnail capturing logic works with the site engagement list. By integrating into TopSites, we might just get that for free.

c#4: yes, being able to avoid having to replicate all of the thumbnail logic was a big motivator. Put another way, c#3 basically proposes that since TopSites produces a final list of sorted URLs at some point, just use site engagement at that point to reorder the list before sending it to MostVisitedSites. That should be good enough to test out in Canary/Dev and see how an engagement ordering would compare to the existing one.

So the proposal is to still use the existing 8 pages from TopSites, just reordered according to the engagement score? I don't think that'll work very well. In that one example from the email thread, many of the top engagement sites weren't in the top 8 TopSites.

Hmm, at what point does TopSites truncate the URLs to 8? Does it only ever store 8 or does it have the bigger list at some point? I made the assumption that a full list was passed to MostVisitedSites which then did the truncation.

TopSites maintains the top 10 only (recently changed from 20 for performance reasons, see https://chromium-review.googlesource.com/656399), and it only requests that many from the history backend, see TopSitesImpl::num_results_to_request_from_history().

c#8: if that's the case, then perhaps the right integration point is one of:

1. request the top 10 URLs from the site engagement service rather than from history (and override the history observer methods to be no-ops). This would probably require some new observer methods to get top sites to update when the top engagement origins change

2. override the history service's most visited URLs method to query site engagement (and the observer methods as well so that top sites naturally updates).

c#10 remains the most viable path here after some more thought. I'm visiting TOK for the next week, but I'll try and carve out some time to put CLs together when I return to SYD. :)

Proof of concept CL at https://chromium-review.googlesource.com/c/chromium/src/+/691497. I haven't added tests yet, but I'll ping treib@ and mastiz@ for an initial look at how I've implemented it before I add those.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8896b1cda304a7d7f8ba08dec69714243a1a7320

commit 8896b1cda304a7d7f8ba08dec69714243a1a7320
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Oct 17 23:24:36 2017

Implement site engagement sorting of TopSites behind a flag.

This CL introduces a feature flag which, when enabled, sources URLs for
the NTP TopSites tiles from the site engagement service instead of using
the history-based "frecency" approach. Using engagement allows sites
that are infrequently navigated to but heavily used to be prioritised in
TopSites.

Currently, engagement sorting of TopSites is limited to sourcing origins
only, as the site engagement service only records engagement per origin.
Future work will overcome this limitation by integrating engagement as a
component in the frecency scoring in VisitSegmentDatabase, rather than
as a completely separate component.

BUG=764218

Change-Id: I13d2072e3d1dd94684fafcbd5e5e991af9e43d00
Reviewed-on: https://chromium-review.googlesource.com/691497
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509587}
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/BUILD.gn
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/about_flags.cc
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/engagement/top_sites/OWNERS
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/engagement/top_sites/site_engagement_top_sites_provider.cc
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/engagement/top_sites/site_engagement_top_sites_provider.h
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/engagement/top_sites/site_engagement_top_sites_provider_unittest.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/history/top_sites_factory.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/browser/thumbnails/thumbnail_service_unittest.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/common/chrome_features.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/common/chrome_features.h
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/chrome/test/BUILD.gn
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/BUILD.gn
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/default_top_sites_provider.cc
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/default_top_sites_provider.h
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/expire_history_backend_unittest.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/history_types.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/history_types.h
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/top_sites_impl.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/top_sites_impl.h
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/top_sites_impl_unittest.cc
[add] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/components/history/core/browser/top_sites_provider.h
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/ios/chrome/browser/history/top_sites_factory.cc
[modify] https://crrev.com/8896b1cda304a7d7f8ba08dec69714243a1a7320/tools/metrics/histograms/enums.xml

treib/mastiz: is there any more interest from the NTP team to move forward with this now that birthday stuff is starting to settle?

Neither of us is working on the NTP anymore. Adding the new NTP folks instead :)

Hi Dominick! I'm the new TL for the NTP. I'm not familiar with the SiteEngagementService - I'll set up some time with you to discuss further. Thanks!

+bklmn@ 

(I'm still interested in this!)