[Sync] Initial implementation of foreign sessions suggestions provider.

components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc

Index: components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc
diff --git a/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc b/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0ddb62829e1ed4ebf388959f57cd0d4bb9f750f8
--- /dev/null
+++ b/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc
@@ -0,0 +1,295 @@
+// 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 <tuple>
+#include <utility>
+
+#include "base/strings/string_piece.h"
+#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/ntp_snippets/pref_util.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/open_tabs_ui_delegate.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 sessions::SessionWindow;
+using sync_driver::SyncedSession;
+
+namespace ntp_snippets {
+namespace {
+
+const int kMaxForeignTabsTotal = 10;
+const int kMaxForeignTabsPerDevice = 3;
+const int kMaxForeignTabAgeInMinutes = 180;
+
+const char* kMaxForeignTabsTotalParamName = "max_foreign_tabs_total";
+const char* kMaxForeignTabsPerDeviceParamName = "max_foreign_tabs_per_device";
+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,
+    std::unique_ptr<OpenTabsUIDelegateProvider> delegate_provider,
+    PrefService* pref_service)
+    : ContentSuggestionsProvider(observer, category_factory),
+      category_status_(CategoryStatus::INITIALIZING),
+      provided_category_(
+          category_factory->FromKnownCategory(KnownCategories::FOREIGN_TABS)),
+      delegate_provider_(std::move(delegate_provider)),
+      pref_service_(pref_service) {
+  delegate_provider_->SubscribeForForeignTabChange(
+      base::Bind(&ForeignSessionsSuggestionsProvider::OnForeignTabChange,
+                 base::Unretained(this)));
+
+  // If sync is already initialzed, try suggesting now, though this is unlikely.
+  OnForeignTabChange(delegate_provider_->GetOpenTabsUIDelegate());
+}
+
+ForeignSessionsSuggestionsProvider::~ForeignSessionsSuggestionsProvider() {}
+
+// 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) {
+  // TODO(skym): Right now this continuously grows, without clearing out old and
+  // irrelevant entries. Could either use a timestamp and expire after a
+  // threshold, or compare with current foreign tabs and remove anything that
+  // isn't actively blockign a foreign_sessions tab.
+  std::set<std::string> dismissed_ids = prefs::ReadDismissedIDsFromPrefs(
+      *pref_service_, prefs::kDismissedForeignSessionsSuggestions);
+  dismissed_ids.insert(suggestion_id);
+  prefs::StoreDismissedIDsToPrefs(pref_service_,
+                                  prefs::kDismissedForeignSessionsSuggestions,
+                                  dismissed_ids);
+}
+
+void ForeignSessionsSuggestionsProvider::FetchSuggestionImage(
+    const std::string& suggestion_id,
+    const ImageFetchedCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, gfx::Image()));
+}
+
+void ForeignSessionsSuggestionsProvider::ClearHistory(
+    base::Time begin,
+    base::Time end,
+    const base::Callback<bool(const GURL& url)>& filter) {
+  std::set<std::string> dismissed_ids = prefs::ReadDismissedIDsFromPrefs(
+      *pref_service_, prefs::kDismissedForeignSessionsSuggestions);
+  for (auto iter = dismissed_ids.begin(); iter != dismissed_ids.end();) {
+    if (filter.Run(GURL(base::StringPiece(*iter)))) {
+      iter = dismissed_ids.erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+  prefs::StoreDismissedIDsToPrefs(pref_service_,
+                                  prefs::kDismissedForeignSessionsSuggestions,
+                                  dismissed_ids);
+}
+
+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::OnForeignTabChange(
+    sync_driver::OpenTabsUIDelegate* open_tabs_ui_delegate) {

[Marc Treib, 2016/09/16 12:26:48]

Is it necessary to pass in the delegate? Can't we just get it via
delegate_provider_->GetOpenTabsUIDelegate()?

[skym, 2016/09/16 18:18:48]

Done.

+  if (open_tabs_ui_delegate) {

[tschumann, 2016/09/16 13:33:20]

nit: i'd invert this condition and exit early instead. Makes it easier to focus
on the main path and saves extra indentations.

[skym, 2016/09/16 18:18:48]

Done.

+    if (category_status_ != CategoryStatus::AVAILABLE) {
+      // The further below logic will overwrite any error state. This is
+      // currently okay because no where in the current implementation does the
+      // status get set to an error state. Should this change, reconsider the
+      // overwriting logic.
+      DCHECK(category_status_ == CategoryStatus::INITIALIZING ||
+             category_status_ == CategoryStatus::NOT_PROVIDED);
+
+      // It is difficult to tell if sync simply has not initialized yet or there
+      // 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;
+    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.
+    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.
+  // TODO(skym): No sense in keeping around dismissals for urls that no longer
+  // exist on any current foreign devices. Should prune and save the pref back.
+  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;
+  std::set<std::string> dismissed_ids = prefs::ReadDismissedIDsFromPrefs(
+      *pref_service_, prefs::kDismissedForeignSessionsSuggestions);
+  for (const SyncedSession* session : foreign_sessions) {
+    for (const std::pair<const SessionID::id_type, SessionWindow*>& key_value :
+         session->windows) {
+      for (const SessionTab* tab : key_value.second->tabs) {
+        if (tab->navigations.empty())
+          continue;
+
+        const SerializedNavigationEntry& navigation = tab->navigations.back();
+        const std::string unique_id =
+            MakeUniqueID(provided_category_, navigation.virtual_url().spec());

[tschumann, 2016/09/16 13:33:20]

this line confused me at first a bit, as we don't have de-duplication logic in
here [found it afterwards].
I was tempted to suggest to do the filtering later when we also de-duplicate.
But it's actually nice to keep the two steps separated and the code cleaner
structured.

That said, what about moving this into a function like:
std::vector<SessionTuple> suggestion_candidates =
GetSuggestionCandidates(foreign_sessions, dismissed_ids, max_forein_tab_age);

I'd also suggest to turn SessionTuple into a struct, like SessionData and define
operator< within that struct. Looking at line 255-257, named members seem useful
anyways.

[skym, 2016/09/16 18:18:48]

Done. I was hesitant to initially break up this function because I felt the big
tuple kind of really tightly coupled them. But with the struct I feel much
better about this.

[tschumann, 2016/09/17 15:52:56]

Thanks!

+        // 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

[tschumann, 2016/09/16 13:33:20]

nit: rephrase to better reflect why we sort?
like // Sort by recency so that we keep the most recent entries.

[skym, 2016/09/16 18:18:48]

Done.

+  // never set to a value, so use SessionTab::timestamp() instead.
+  // TODO(skym): It might be better if we sorted by recency of session, and only
+  // then by recency of the tab. Right now a single device's tabs can be
+  // interleaved with another device's tabs.
+  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;

[tschumann, 2016/09/16 13:33:20]

nano nit: this variable is technically not collecting duplicated urls. Rename to
included_urls? (the iterator can still be called duplicate_iter as it nicely
explains the semantics in that case).

[skym, 2016/09/16 18:18:48]

Done.

+  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 duplicates
+    // 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.push_back(BuildSuggestion(session, tab, navigation));
+  }
+
+  return suggestions;
+}
+
+ContentSuggestion ForeignSessionsSuggestionsProvider::BuildSuggestion(
+    const SyncedSession& session,
+    const SessionTab& tab,
+    const SerializedNavigationEntry& navigation) {
+  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() + " - " + session.session_name));

[Marc Treib, 2016/09/16 12:26:48]

This should probably be a string resource with two placeholders, so that e.g.
the two can get properly swapped in right-to-left languages. Or maybe some
languages use a different separator char.
(If we're not sure what the final version will look like, it's also fine to just
add a TODO for now.)

[skym, 2016/09/16 18:18:48]

I'll throw in a big comment here. The favicon is on the far left, so I think it
would only make sense to swap these if we also moved around where the favicon
shows up, we want the domain adjacent to the icon. Though admittedly, I am not
familiar with localizing for right-to-left languages.

[Marc Treib, 2016/09/19 09:24:08]

Acknowledged.

+  return suggestion;
+}
+
+}  // namespace ntp_snippets