[Sync] Initial implementation of foreign sessions suggestions provider.

components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc

Index: components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc b/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b0bdda17e4bda8f432205df0ebca01d55d258bb9
--- /dev/null
+++ b/components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc
@@ -0,0 +1,335 @@
+// 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 <map>
+#include <utility>
+
+#include "base/memory/ptr_util.h"
+#include "components/ntp_snippets/category.h"
+#include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
+#include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
+#include "components/prefs/testing_pref_service.h"
+#include "components/sessions/core/serialized_navigation_entry.h"
+#include "components/sessions/core/serialized_navigation_entry_test_helper.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 "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::Time;
+using base::TimeDelta;
+using sessions::SerializedNavigationEntry;
+using sessions::SessionTab;
+using sessions::SessionWindow;
+using sync_driver::SyncedSession;
+using testing::IsEmpty;
+using testing::Property;
+using testing::ElementsAre;
+using testing::UnorderedElementsAre;
+using testing::_;
+
+namespace ntp_snippets {
+namespace {
+
+const char kUrl1[] = "http://www.fake1.com/";
+const char kUrl2[] = "http://www.fake2.com/";
+const char kUrl3[] = "http://www.fake3.com/";
+const char kUrl4[] = "http://www.fake4.com/";
+const char kUrl5[] = "http://www.fake5.com/";
+const char kUrl6[] = "http://www.fake6.com/";
+const char kUrl7[] = "http://www.fake7.com/";
+const char kUrl8[] = "http://www.fake8.com/";
+const char kUrl9[] = "http://www.fake9.com/";
+const char kUrl10[] = "http://www.fake10.com/";
+const char kUrl11[] = "http://www.fake11.com/";
+const char kTitle[] = "title is ignored";
+
+SessionWindow* GetWindow(SyncedSession* session, int window_id) {

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

GetOrCreateWindow?

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

Done.

+  if (session->windows.find(window_id) == session->windows.end()) {
+    // The session deletes the windows it points at upon destruction.
+    session->windows[window_id] = new SessionWindow();
+  }
+  return session->windows[window_id];
+}
+
+void AddTabToSession(SyncedSession* session,
+                     int window_id,
+                     const std::string& url,
+                     TimeDelta age) {
+  SerializedNavigationEntry navigation =
+      sessions::SerializedNavigationEntryTestHelper::CreateNavigation(url,
+                                                                      kTitle);
+
+  std::unique_ptr<SessionTab> tab = base::MakeUnique<SessionTab>();
+  tab->timestamp = Time::Now() - age;
+  tab->navigations.push_back(navigation);
+
+  SessionWindow* window = GetWindow(session, window_id);
+  // The window deletes the tabs it points at upon destruction.
+  window->tabs.push_back(tab.release());
+}
+
+class FakeTabsDelegate
+    : public sync_driver::OpenTabsUIDelegate,

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

do we actually need to implement this interface?

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

Done. Ended up removing OpenTabsUIDelegate from the provider interface, turned
out a lot prettier I think.

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

:-) I think so too!

+      public ForeignSessionsSuggestionsProvider::OpenTabsUIDelegateProvider {
+ public:
+  ~FakeTabsDelegate() override {}
+  void SetAllForeignSessions(std::vector<const SyncedSession*> sessions) {
+    sessions_ = sessions;
+    change_callback_.Run(this);
+  }
+
+  // OpenTabsUIDelegate implementation.
+  bool GetSyncedFaviconForPageURL(
+      const std::string& pageurl,
+      scoped_refptr<base::RefCountedMemory>* favicon_png) const override {
+    return false;
+  }
+  bool GetAllForeignSessions(
+      std::vector<const SyncedSession*>* sessions) override {
+    *sessions = sessions_;
+    return !sessions->empty();
+  }
+  bool GetForeignTab(const std::string& tag,
+                     const SessionID::id_type tab_id,
+                     const sessions::SessionTab** tab) override {
+    return false;
+  }
+  void DeleteForeignSession(const std::string& tag) override {}
+  bool GetForeignSession(
+      const std::string& tag,
+      std::vector<const sessions::SessionWindow*>* windows) override {
+    return false;
+  }
+  bool GetForeignSessionTabs(
+      const std::string& tag,
+      std::vector<const sessions::SessionTab*>* tabs) override {
+    return false;
+  }
+  bool GetLocalSession(const SyncedSession** local) override { return false; }
+
+  // OpenTabsUIDelegateProvider implementation.
+  sync_driver::OpenTabsUIDelegate* GetOpenTabsUIDelegate() override {
+    return this;
+  }
+  void SubscribeForForeignTabChange(
+      const OnChangeWithDelegate& change_callback) override {
+    change_callback_ = change_callback;
+  }
+
+ private:
+  std::vector<const SyncedSession*> sessions_;
+  OnChangeWithDelegate change_callback_;
+};
+}  // namespace
+
+class ForeignSessionsSuggestionsProviderTest : public testing::Test {
+ public:
+  ForeignSessionsSuggestionsProviderTest()
+      : pref_service_(base::MakeUnique<TestingPrefServiceSimple>()) {
+    ForeignSessionsSuggestionsProvider::RegisterProfilePrefs(
+        pref_service_->registry());
+
+    std::unique_ptr<FakeTabsDelegate> delegate =
+        base::MakeUnique<FakeTabsDelegate>();
+    fake_tabs_delegate_ = delegate.get();
+
+    // During the provider's construction the follow mock calls occur.

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

nit: s/follow/following/

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

Done.

+    EXPECT_CALL(*observer(),
+                OnNewSuggestions(_, category(), testing::IsEmpty()));

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

nit: "testing::" not required, there's a "using" above. (There's a few more
instances of this below.)

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

Done.

+    EXPECT_CALL(*observer(), OnCategoryStatusChanged(
+                                 _, category(), CategoryStatus::AVAILABLE));
+
+    provider_ = base::MakeUnique<ForeignSessionsSuggestionsProvider>(
+        &observer_, &category_factory_, std::move(delegate),
+        pref_service_.get());
+  }
+
+ protected:
+  SyncedSession* GetSession(int session_id) {

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

GotOrCreateSession?

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

Done.

+    if (sessions_map_.find(session_id) == sessions_map_.end()) {
+      std::string id_as_string = std::to_string(session_id);

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

Per https://chromium-cpp.appspot.com/, std::to_string isn't allowed yet :(

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

:( Done.

+      std::unique_ptr<SyncedSession> owned_session =
+          base::MakeUnique<SyncedSession>();
+      owned_session->session_tag = id_as_string;
+      owned_session->session_name = id_as_string;
+      sessions_map_[session_id] = std::move(owned_session);
+    }
+    return sessions_map_[session_id].get();
+  }
+
+  void AddTab(int session_id,
+              int window_id,
+              const std::string& url,
+              TimeDelta age) {
+    AddTabToSession(GetSession(session_id), window_id, url, age);
+  }
+
+  void TriggerOnChange() {
+    std::vector<const SyncedSession*> sessions;
+    for (const auto& kv : sessions_map_) {
+      sessions.push_back(kv.second.get());
+    }
+    fake_tabs_delegate_->SetAllForeignSessions(sessions);
+  }
+
+  void Dismiss(const std::string& url) {

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

s/url/id/ ?

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

So someone has to decide that to create a within_category_id for foreign tabs
you just use the url. Right now that decision is made right here. If we renamed
this field to id, then that decision is essentially being made at every location
that calls this function instead. I think the former is better. I'll add a
comment here making it more clear that this conversion is happening.

+    provider_->DismissSuggestion(provider_->MakeUniqueID(category(), url));
+  }
+
+  Category category() {
+    return category_factory_.FromKnownCategory(KnownCategories::FOREIGN_TABS);
+  }
+
+  MockContentSuggestionsProviderObserver* observer() { return &observer_; }
+
+ private:
+  FakeTabsDelegate* fake_tabs_delegate_;
+  MockContentSuggestionsProviderObserver observer_;
+  CategoryFactory category_factory_;
+  std::unique_ptr<TestingPrefServiceSimple> pref_service_;

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

I think this could just be an instance rather than a pointer?

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

Done.

+  std::unique_ptr<ForeignSessionsSuggestionsProvider> provider_;
+  std::map<int, std::unique_ptr<SyncedSession>> sessions_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(ForeignSessionsSuggestionsProviderTest);
+};
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, Empty) {
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, category(), testing::IsEmpty()));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, Single) {
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, category(),
+                  ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl1)))));
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, Old) {
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, category(), testing::IsEmpty()));
+  AddTab(0, 0, kUrl1, TimeDelta::FromHours(4));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, Ordered) {
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, category(),
+                  ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl1)),
+                              Property(&ContentSuggestion::url, GURL(kUrl2)),
+                              Property(&ContentSuggestion::url, GURL(kUrl3)),
+                              Property(&ContentSuggestion::url, GURL(kUrl4)))));
+  AddTab(0, 0, kUrl2, TimeDelta::FromMinutes(2));
+  AddTab(0, 0, kUrl4, TimeDelta::FromMinutes(4));
+  AddTab(0, 1, kUrl3, TimeDelta::FromMinutes(3));
+  AddTab(1, 0, kUrl1, TimeDelta::FromMinutes(1));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, MaxPerDevice) {
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, category(),
+                  ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl1)),
+                              Property(&ContentSuggestion::url, GURL(kUrl2)),
+                              Property(&ContentSuggestion::url, GURL(kUrl3)))));
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  AddTab(0, 0, kUrl2, TimeDelta::FromMinutes(2));
+  AddTab(0, 0, kUrl3, TimeDelta::FromMinutes(3));
+  AddTab(0, 0, kUrl4, TimeDelta::FromMinutes(4));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, MaxTotal) {
+  EXPECT_CALL(

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

nit: I think all of these tests deserve a one-line comment explaining the
expected behavior, e.g. something like "We should get a maximum of 10
suggestions in total."

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

Done.

+      *observer(),
+      OnNewSuggestions(
+          _, category(),
+          ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl1)),
+                      Property(&ContentSuggestion::url, GURL(kUrl2)),
+                      Property(&ContentSuggestion::url, GURL(kUrl3)),
+                      Property(&ContentSuggestion::url, GURL(kUrl4)),
+                      Property(&ContentSuggestion::url, GURL(kUrl5)),
+                      Property(&ContentSuggestion::url, GURL(kUrl6)),
+                      Property(&ContentSuggestion::url, GURL(kUrl7)),
+                      Property(&ContentSuggestion::url, GURL(kUrl8)),
+                      Property(&ContentSuggestion::url, GURL(kUrl9)),
+                      Property(&ContentSuggestion::url, GURL(kUrl10)))));
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  AddTab(0, 0, kUrl2, TimeDelta::FromMinutes(2));
+  AddTab(0, 0, kUrl3, TimeDelta::FromMinutes(3));
+  AddTab(1, 0, kUrl4, TimeDelta::FromMinutes(4));
+  AddTab(1, 0, kUrl5, TimeDelta::FromMinutes(5));
+  AddTab(1, 0, kUrl6, TimeDelta::FromMinutes(6));
+  AddTab(2, 0, kUrl7, TimeDelta::FromMinutes(7));
+  AddTab(2, 0, kUrl8, TimeDelta::FromMinutes(8));
+  AddTab(2, 0, kUrl9, TimeDelta::FromMinutes(9));
+  AddTab(3, 0, kUrl10, TimeDelta::FromMinutes(10));
+  AddTab(3, 0, kUrl11, TimeDelta::FromMinutes(11));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, Duplicates) {
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, category(),
+                  ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl1)))));
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  AddTab(0, 1, kUrl1, TimeDelta::FromMinutes(2));
+  AddTab(1, 1, kUrl1, TimeDelta::FromMinutes(3));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, DuplicatesChangingOtherSession) {
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, category(),
+                  ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl3)),
+                              Property(&ContentSuggestion::url, GURL(kUrl1)),
+                              Property(&ContentSuggestion::url, GURL(kUrl2)),
+                              Property(&ContentSuggestion::url, GURL(kUrl4)))));
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  AddTab(0, 0, kUrl2, TimeDelta::FromMinutes(2));
+  AddTab(0, 0, kUrl3, TimeDelta::FromMinutes(3));
+  // Normally |kUrl4| wouldn't show up, because session_id=0 already has 3
+  // younger tabs, but session_id=1 has a younger |kUrl3| which gives |kUrl4| a
+  // spot.
+  AddTab(0, 0, kUrl4, TimeDelta::FromMinutes(4));
+  AddTab(1, 0, kUrl3, TimeDelta::FromMinutes(0));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, Dismissed) {
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, category(), testing::IsEmpty()));
+  Dismiss(kUrl1);
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  TriggerOnChange();
+}
+
+TEST_F(ForeignSessionsSuggestionsProviderTest, DismissedChangingOwnSession) {
+  EXPECT_CALL(*observer(),
+              OnNewSuggestions(
+                  _, category(),
+                  ElementsAre(Property(&ContentSuggestion::url, GURL(kUrl2)),
+                              Property(&ContentSuggestion::url, GURL(kUrl3)),
+                              Property(&ContentSuggestion::url, GURL(kUrl5)))));
+  Dismiss(kUrl1);
+  Dismiss(kUrl4);
+  AddTab(0, 0, kUrl1, TimeDelta::FromMinutes(1));
+  AddTab(0, 0, kUrl2, TimeDelta::FromMinutes(2));
+  AddTab(0, 0, kUrl3, TimeDelta::FromMinutes(3));
+  AddTab(0, 0, kUrl4, TimeDelta::FromMinutes(4));
+  AddTab(0, 0, kUrl5, TimeDelta::FromMinutes(5));
+  AddTab(0, 0, kUrl6, TimeDelta::FromMinutes(6));
+  TriggerOnChange();
+}
+
+}  // namespace ntp_snippets