[Sync] Initial implementation of foreign sessions suggestions provider.

components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc

+// Copyright 2016 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/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h"
+
+#include <algorithm>
+#include <map>
+#include <memory>
+#include <tuple>
+#include <utility>
+
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/category_info.h"
+#include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_snippets/features.h"
+#include "components/ntp_snippets/pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#include "components/sessions/core/session_types.h"
+#include "components/sync_sessions/synced_session.h"
+#include "grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/image/image.h"
+#include "url/gurl.h"
+
+using base::TimeDelta;
+using sessions::SerializedNavigationEntry;
+using sessions::SessionTab;
+using sync_driver::SyncedSession;
+
+namespace ntp_snippets {
+namespace {
+
+const int kMaxForeignTabsTotal = 10;
+const int kMaxForeignTabsPerDevice = 3;
+const int kMaxForeignTabAgeInMinutes = 60;
+
+const char* kMaxForeignTabsTotalParamName = "max_foreign_tabs_total";
+const char* kMaxForeignTabsPerDeviceParamName = "max_tabs_per_device";

[Marc Treib, 2016/08/29 09:18:51]

max_foreign_tabs_per_device, for consistency?

[skym, 2016/09/15 23:18:17]

Done.

+const char* kMaxForeignTabAgeInMinutesParamName =
+    "max_foreign_tabs_age_in_minutes";
+
+int GetMaxForeignTabsTotal() {
+  return GetParamAsInt(ntp_snippets::kForeignSessionsSuggestionsFeature,
+                       kMaxForeignTabsTotalParamName, kMaxForeignTabsTotal);
+}
+
+int GetMaxForeignTabsPerDevice() {
+  return GetParamAsInt(ntp_snippets::kForeignSessionsSuggestionsFeature,
+                       kMaxForeignTabsPerDeviceParamName,
+                       kMaxForeignTabsPerDevice);
+}
+
+TimeDelta GetMaxForeignTabAge() {
+  return TimeDelta::FromMinutes(GetParamAsInt(
+      ntp_snippets::kForeignSessionsSuggestionsFeature,
+      kMaxForeignTabAgeInMinutesParamName, kMaxForeignTabAgeInMinutes));
+}
+
+}  // namespace
+
+ForeignSessionsSuggestionsProvider::ForeignSessionsSuggestionsProvider(
+    ContentSuggestionsProvider::Observer* observer,
+    CategoryFactory* category_factory,
+    sync_driver::SyncService* sync_service,
+    PrefService* pref_service)
+    : ContentSuggestionsProvider(observer, category_factory),
+      category_status_(CategoryStatus::INITIALIZING),
+      provided_category_(
+          category_factory->FromKnownCategory(KnownCategories::FOREIGN_TABS)),
+      sync_service_(sync_service),

[tschumann, 2016/08/29 09:39:15]

nit: I wonder if ForeignSessionsSuggestionsProvider plays too many roles
(looking at the many interface methods it implements).

What about having a SyncServiceApapter class so that we can use composition
instead of inheritance?

Something like
class SyncServiceApapter : public sync_driver::SyncServiceObserver {
 public:
   SyncServiceAdapter(sync_driver::SyncService* sync_service,
                      base::Callback<void(sync_driver::OpenTabsUIDelegate*
on_data);
}

which also hides away the registration with the service?
WDYT?

[skym, 2016/09/15 23:18:17]

Done. Tried to make it DI as well, got a little awkward, let me know if you
think there's a way I could improve it.

+      pref_service_(pref_service),
+      dismissed_ids_(prefs::ReadDismissedIDsFromPrefs(
+          prefs::kDismissedForeignSessionsSuggestions,
+          pref_service)) {
+  sync_service_->AddObserver(this);
+  // If sync is already initialzed, try suggesting now, though this is unlikely.
+  TrySuggest();
+}
+
+ForeignSessionsSuggestionsProvider::~ForeignSessionsSuggestionsProvider() {
+  sync_service_->RemoveObserver(this);
+}
+
+// static
+void ForeignSessionsSuggestionsProvider::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterListPref(prefs::kDismissedForeignSessionsSuggestions);
+}
+
+CategoryStatus ForeignSessionsSuggestionsProvider::GetCategoryStatus(
+    Category category) {
+  DCHECK_EQ(category, provided_category_);
+  return category_status_;
+}
+
+CategoryInfo ForeignSessionsSuggestionsProvider::GetCategoryInfo(
+    Category category) {
+  DCHECK_EQ(category, provided_category_);
+  return CategoryInfo(l10n_util::GetStringUTF16(
+                          IDS_NTP_FOREIGN_SESSIONS_SUGGESTIONS_SECTION_HEADER),
+                      ContentSuggestionsCardLayout::MINIMAL_CARD,
+                      /* has_more_button */ true,
+                      /* show_if_empty */ false);
+}
+
+void ForeignSessionsSuggestionsProvider::DismissSuggestion(
+    const std::string& suggestion_id) {
+  dismissed_ids_.insert(suggestion_id);

[Marc Treib, 2016/08/29 09:18:51]

Right now, this can only ever grow. We need some way to clean up
no-longer-relevant IDs, e.g. a full cleanup pass on startup. Also maybe an
expiry time?
(All that can wait for another CL, but at least a TODO would be nice.)

[battre, 2016/08/29 09:48:26]

+1 - I am a bit concerned that the preferences file becomes a huge dumping
ground if suggestions are ephemeral things and you get the opportunity to
dismiss several ones per day.
Really? Then you need to do a data scheme migration afterwards.

[Marc Treib, 2016/08/29 09:56:38]

Right now, this is behind a disabled-by-default flag, so it's okay if the
implementation is incomplete. Clearly this will need to be addressed before any
launch.

The "data migration" shouldn't be any different from the periodic cleanup which
we'll need anyway.

[skym, 2016/09/15 23:18:17]

Done.

+  prefs::StoreDismissedIDsToPrefs(prefs::kDismissedForeignSessionsSuggestions,
+                                  dismissed_ids_, pref_service_);
+}
+
+void ForeignSessionsSuggestionsProvider::FetchSuggestionImage(
+    const std::string& suggestion_id,
+    const ImageFetchedCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, gfx::Image()));
+}
+
+void ForeignSessionsSuggestionsProvider::ClearCachedSuggestions(
+    Category category) {
+  DCHECK_EQ(category, provided_category_);
+  // Ignored.
+}
+
+void ForeignSessionsSuggestionsProvider::GetDismissedSuggestionsForDebugging(
+    Category category,
+    const DismissedSuggestionsCallback& callback) {
+  DCHECK_EQ(category, provided_category_);
+  callback.Run(std::vector<ContentSuggestion>());
+}
+
+void ForeignSessionsSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
+    Category category) {
+  DCHECK_EQ(category, provided_category_);
+  pref_service_->ClearPref(prefs::kDismissedForeignSessionsSuggestions);
+}
+
+void ForeignSessionsSuggestionsProvider::OnStateChanged() {
+  // Ignored.
+}
+
+void ForeignSessionsSuggestionsProvider::OnSyncConfigurationCompleted() {
+  TrySuggest();
+}
+
+void ForeignSessionsSuggestionsProvider::OnForeignSessionUpdated() {
+  TrySuggest();
+}
+
+void ForeignSessionsSuggestionsProvider::TrySuggest() {
+  sync_driver::OpenTabsUIDelegate* open_tabs_ui_delegate =
+      sync_service_->GetOpenTabsUIDelegate();
+  if (open_tabs_ui_delegate) {
+    if (category_status_ != CategoryStatus::AVAILABLE) {
+      // It is difficult to tell if sync simply has not initialized yet or there

[Marc Treib, 2016/08/29 09:18:51]

Here, category_status_ can only be INITIALIZING or NOT_PROVIDED, right? DCHECK
that maybe, to make clear that this won't reset any error state?

[skym, 2016/09/15 23:18:17]

Done.

+      // will never be data because the user is signed out or has disabled the
+      // sessions data type. Because this provider is hidden when there are no
+      // results, always just update to AVAILABLE once we might have results.
+      category_status_ = CategoryStatus::AVAILABLE;
+      observer()->OnCategoryStatusChanged(this, provided_category_,
+                                          category_status_);
+    }
+
+    std::vector<const SyncedSession*> foreign_sessions;
+    if (open_tabs_ui_delegate->GetAllForeignSessions(&foreign_sessions)) {
+      // observer()->OnNewSuggestions must be called even when we have no
+      // suggestions to remove previous suggestions that are now filtered out.

[Marc Treib, 2016/08/29 09:18:51]

...so the "if" this is in shouldn't be there?

[skym, 2016/09/15 23:18:18]

Whooops, you are correct! Great catch.

+      observer()->OnNewSuggestions(this, provided_category_,
+                                   BuildSuggestions(foreign_sessions));
+    }
+  } else if (category_status_ == CategoryStatus::AVAILABLE) {
+    // This is to handle the case where the user disabled sync [sessions] or
+    // logs out after we've already provided actual suggestions.
+    category_status_ = CategoryStatus::NOT_PROVIDED;
+    observer()->OnCategoryStatusChanged(this, provided_category_,
+                                        category_status_);
+  }
+}
+
+std::vector<ContentSuggestion>
+ForeignSessionsSuggestionsProvider::BuildSuggestions(
+    const std::vector<const SyncedSession*>& foreign_sessions) {
+  // TODO(skym): If a tab was previously dismissed, but was since updated,
+  // should it be resurrected and removed from the dismissed list? This would
+  // likely require a change to the dismissed ids.
+  const TimeDelta max_foreign_tab_age = GetMaxForeignTabAge();
+  const int max_foreign_tabs_total = GetMaxForeignTabsTotal();
+  const int max_foreign_tabs_per_device = GetMaxForeignTabsPerDevice();
+  using SessionTuple = std::tuple<const SyncedSession*, const SessionTab*,
+                                  const SerializedNavigationEntry*>;
+  std::vector<SessionTuple> suggestion_candidates;
+  for (const SyncedSession* session : foreign_sessions) {
+    for (const std::pair<const SessionID::id_type, sessions::SessionWindow*>&
+             key_value : session->windows) {
+      for (const SessionTab* tab : key_value.second->tabs) {
+        if (tab->navigations.size() > 0) {

[Marc Treib, 2016/08/29 09:18:51]

optional nit: "if (tab->navigations.size() == 0) continue;", to remove one of
the many levels of indentation?

[battre, 2016/08/29 09:48:26]

I think we prefer empty() over size() == 0

[skym, 2016/09/15 23:18:18]

Done.

[skym, 2016/09/15 23:18:18]

Done.

+          const SerializedNavigationEntry& navigation = tab->navigations.back();
+          const std::string unique_id =
+              MakeUniqueID(provided_category_, navigation.virtual_url().spec());
+          // TODO(skym): Filter out internal pages. Tabs that contain only
+          // non-syncable content should never reach the local client, but
+          // sometimes the most recent navigation may be internal while one
+          // of the previous ones was more valid.
+          if (dismissed_ids_.find(unique_id) == dismissed_ids_.end() &&
+              (base::Time::Now() - tab->timestamp) < max_foreign_tab_age) {
+            suggestion_candidates.emplace_back(session, tab, &navigation);
+          }
+        }
+      }
+    }
+  }
+
+  // Sort by recency. Note that SerializedNavigationEntry::timestamp() is
+  // never set to a value, so use SessionTab::timestamp() instead.
+  std::sort(suggestion_candidates.begin(), suggestion_candidates.end(),
+            [](const SessionTuple& a, const SessionTuple& b) -> bool {
+              return std::get<1>(a)->timestamp > std::get<1>(b)->timestamp;
+            });
+  std::vector<ContentSuggestion> suggestions;
+  std::set<std::string> duplicate_urls;
+  std::map<std::string, int> suggestions_per_session;
+  for (const SessionTuple& tuple : suggestion_candidates) {
+    const SyncedSession& session = *std::get<0>(tuple);
+    const SessionTab& tab = *std::get<1>(tuple);
+    const SerializedNavigationEntry& navigation = *std::get<2>(tuple);
+
+    auto duplicates_iter = duplicate_urls.find(navigation.virtual_url().spec());
+    auto count_iter = suggestions_per_session.find(session.session_tag);
+    int count =
+        count_iter == suggestions_per_session.end() ? 0 : count_iter->second;
+
+    // Pick up to max (total and per device) tabs, and ensure no duplcates

[Marc Treib, 2016/08/29 09:18:51]

s/duplcates/duplicates/

[skym, 2016/09/15 23:18:18]

Done.

+    // are selected. This filtering must be done in a second pass because
+    // this can cause newer tabs occluding less recent tabs, requiring more
+    // than |max_foreign_tabs_per_device| to be considered per device.
+    if (static_cast<int>(suggestions.size()) >= max_foreign_tabs_total ||
+        duplicates_iter != duplicate_urls.end() ||
+        count >= max_foreign_tabs_per_device) {
+      continue;
+    }
+    duplicate_urls.insert(navigation.virtual_url().spec());
+    suggestions_per_session[session.session_tag] = count + 1;
+    suggestions.emplace_back(BuildSuggestion(tab, navigation));

[Marc Treib, 2016/08/29 09:18:51]

nit: push_back will do the same thing here

[skym, 2016/09/15 23:18:18]

Done.

+  }
+  return suggestions;
+}
+
+ContentSuggestion ForeignSessionsSuggestionsProvider::BuildSuggestion(
+    const SessionTab& tab,
+    const SerializedNavigationEntry& navigation) {
+  // TODO(skym): Ideally we would expose the host device's name, but there
+  // is not currently a convineint way to show this in the UI. If device

[Marc Treib, 2016/08/29 09:18:51]

s/convineint/convenient/

[skym, 2016/09/15 23:18:18]

Done.

+  // name is shown, it may make sense for tabs to be ordered by device
+  // recency and then tab/navigation recency.

[Marc Treib, 2016/08/29 09:18:51]

Some ideas (not thought through):
- Have a section per device
- Add it to the publisher_name field ("Chromebook - facebook.com")

[skym, 2016/09/15 23:18:17]

The line with the publisher already looks like:

[icon] www.google.com - 15 minutes ago

Still short enough most of the time, but once in a while a long domain name will
cause things to get cut off. I'm switching the ordering so that the icon stays
next to the domain, I think it is more usable with device, although polish-wise,
occasionally ugly.

+  ContentSuggestion suggestion(
+      MakeUniqueID(provided_category_, navigation.virtual_url().spec()),
+      navigation.virtual_url());
+  suggestion.set_title(navigation.title());
+  suggestion.set_publish_date(tab.timestamp);
+  suggestion.set_publisher_name(
+      base::UTF8ToUTF16(navigation.virtual_url().host()));
+  return suggestion;
+}
+
+}  // namespace ntp_snippets