Dynamic updating recent menu for tabs from other devices.

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

Index: chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
index b89a513b7f458ebcad4c3f6391d0a6615fdab490..ec53dceaa9e318190e4df1076826f787b9ebddb9 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
@@ -12,12 +12,14 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/sessions/chrome_tab_restore_service_client.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/profile_sync_test_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
@@ -44,6 +46,10 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::Invoke;
+using testing::Return;
+
 namespace {
 
 // This copies parts of MenuModelTest::Delegate and combines them with the
@@ -52,9 +58,8 @@ namespace {
 class TestRecentTabsSubMenuModel : public RecentTabsSubMenuModel {
  public:
   TestRecentTabsSubMenuModel(ui::AcceleratorProvider* provider,
-                             Browser* browser,
-                             sync_sessions::OpenTabsUIDelegate* delegate)
-      : RecentTabsSubMenuModel(provider, browser, delegate),
+                             Browser* browser)
+      : RecentTabsSubMenuModel(provider, browser),
         execute_count_(0),
         enable_count_(0) {}
 
@@ -115,27 +120,78 @@ class DummyRouter : public sync_sessions::LocalSessionEventRouter {
   void Stop() override {}
 };
 
+class FakeSyncServiceObserverList {
+ public:
+  FakeSyncServiceObserverList() {}
+  ~FakeSyncServiceObserverList() {}
+
+  void AddObserver(syncer::SyncServiceObserver* observer) {
+    observers_.AddObserver(observer);
+  }
+
+  void RemoveObserver(syncer::SyncServiceObserver* observer) {
+    observers_.RemoveObserver(observer);
+  }
+
+  void NotifyForeignSessionUpdated() {
+    for (auto& observer : observers_)
+      observer.OnForeignSessionUpdated(nullptr);
+  }
+
+ private:
+  base::ObserverList<syncer::SyncServiceObserver> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeSyncServiceObserverList);
+};
+
 }  // namespace
 
 class RecentTabsSubMenuModelTest
     : public BrowserWithTestWindowTest {
  public:
-  RecentTabsSubMenuModelTest()
-      : sync_service_(CreateProfileSyncServiceParamsForTest(&testing_profile_)),
-        local_device_(new syncer::LocalDeviceInfoProviderMock(
-            "RecentTabsSubMenuModelTest",
-            "Test Machine",
-            "Chromium 10k",
-            "Chrome 10k",
-            sync_pb::SyncEnums_DeviceType_TYPE_LINUX,
-            "device_id")) {
-    sync_prefs_.reset(new syncer::SyncPrefs(testing_profile_.GetPrefs()));
+  RecentTabsSubMenuModelTest() {}
+
+  void SetUp() override {
+    BrowserWithTestWindowTest::SetUp();
+
+    local_device_ = base::MakeUnique<syncer::LocalDeviceInfoProviderMock>(
+        "RecentTabsSubMenuModelTest", "Test Machine", "Chromium 10k",
+        "Chrome 10k", sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "device_id");
+    sync_prefs_ = base::MakeUnique<syncer::SyncPrefs>(profile()->GetPrefs());
+    fake_sync_service_observer_list_ =
+        base::MakeUnique<FakeSyncServiceObserverList>();
+
+    mock_sync_service_ = static_cast<browser_sync::ProfileSyncServiceMock*>(
+        ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+            profile(), BuildMockProfileSyncService));
+
+    EXPECT_CALL(*mock_sync_service_, AddObserver(_))
+        .WillRepeatedly(Invoke(fake_sync_service_observer_list_.get(),
+                               &FakeSyncServiceObserverList::AddObserver));
+    EXPECT_CALL(*mock_sync_service_, RemoveObserver(_))
+        .WillRepeatedly(Invoke(fake_sync_service_observer_list_.get(),
+                               &FakeSyncServiceObserverList::RemoveObserver));
+    EXPECT_CALL(*mock_sync_service_, IsSyncActive())
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*mock_sync_service_,
+                IsDataTypeControllerRunning(syncer::SESSIONS))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*mock_sync_service_,
+                IsDataTypeControllerRunning(syncer::PROXY_TABS))
+        .WillRepeatedly(Return(true));
+
     manager_.reset(new sync_sessions::SessionsSyncManager(
-        sync_service_.GetSyncClient()->GetSyncSessionsClient(),
+        mock_sync_service_->GetSyncClient()->GetSyncSessionsClient(),
         sync_prefs_.get(), local_device_.get(),
         std::unique_ptr<sync_sessions::LocalSessionEventRouter>(
             new DummyRouter()),
-        base::Closure(), base::Closure()));
+        base::Bind(&FakeSyncServiceObserverList::NotifyForeignSessionUpdated,
+                   base::Unretained(fake_sync_service_observer_list_.get())),
+        base::Closure()));
+
+    EXPECT_CALL(*mock_sync_service_, GetOpenTabsUIDelegateMock())
+        .WillRepeatedly(Return(manager_.get()));
+
     manager_->MergeDataAndStartSyncing(
         syncer::SESSIONS, syncer::SyncDataList(),
         std::unique_ptr<syncer::SyncChangeProcessor>(
@@ -144,6 +200,14 @@ class RecentTabsSubMenuModelTest
             new syncer::SyncErrorFactoryMock));
   }
 
+  void TearDown() override {
+    manager_.reset();
+    fake_sync_service_observer_list_.reset();
+    sync_prefs_.reset();
+    local_device_.reset();
+    BrowserWithTestWindowTest::TearDown();
+  }
+
   void WaitForLoadFromLastSession() {
     content::RunAllBlockingPoolTasksUntilIdle();
   }
@@ -165,16 +229,16 @@ class RecentTabsSubMenuModelTest
   }
 
  private:
-  TestingProfile testing_profile_;
-  browser_sync::ProfileSyncServiceMock sync_service_;
+  std::unique_ptr<syncer::LocalDeviceInfoProviderMock> local_device_;
   std::unique_ptr<syncer::SyncPrefs> sync_prefs_;
+  std::unique_ptr<FakeSyncServiceObserverList> fake_sync_service_observer_list_;

[Peter Kasting, 2017/03/24 00:06:27]

Nit: How come this needs to be stored in a unique_ptr instead of just by value? 
Does it actually have to be deleted at a certain spot in TearDown()?  Is this to
ensure that we won't have any observers left on it between tests?

+  browser_sync::ProfileSyncServiceMock* mock_sync_service_ = nullptr;
   std::unique_ptr<sync_sessions::SessionsSyncManager> manager_;
-  std::unique_ptr<syncer::LocalDeviceInfoProviderMock> local_device_;
 };
 
 // Test disabled "Recently closed" header with no foreign tabs.
 TEST_F(RecentTabsSubMenuModelTest, NoTabs) {
-  TestRecentTabsSubMenuModel model(NULL, browser(), NULL);
+  TestRecentTabsSubMenuModel model(nullptr, browser());
 
   // Expected menu:
   // Menu index  Menu items
@@ -216,7 +280,7 @@ TEST_F(RecentTabsSubMenuModelTest, RecentlyClosedTabsFromCurrentSession) {
   AddTab(browser(), GURL("http://foo/2"));
   browser()->tab_strip_model()->CloseAllTabs();
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), NULL);
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   // Expected menu:
   // Menu index  Menu items
   // --------------------------------------
@@ -310,7 +374,7 @@ TEST_F(RecentTabsSubMenuModelTest,
   // Let the shutdown of previous TabRestoreService run.
   content::RunAllBlockingPoolTasksUntilIdle();
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), NULL);
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   TestRecentTabsMenuModelDelegate delegate(&model);
   EXPECT_FALSE(delegate.got_changes());
 
@@ -435,7 +499,7 @@ TEST_F(RecentTabsSubMenuModelTest, OtherDevices) {
   // 10          <the only tab of window 0 of session 1>
   // 11-12       <2 tabs of window 1 of session 2>
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), GetOpenTabsDelegate());
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   int num_items = model.GetItemCount();
   EXPECT_EQ(13, num_items);
   model.ActivatedAt(0);
@@ -493,6 +557,131 @@ TEST_F(RecentTabsSubMenuModelTest, OtherDevices) {
   EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(12, &url, &title));
 }
 
+// Mac doesn't support the dynamic menu.
+#if defined(OS_MACOSX)
+#define MAYBE_OtherDevicesDynamicUpdate DISABLED_OtherDevicesDynamicUpdate
+#else
+#define MAYBE_OtherDevicesDynamicUpdate OtherDevicesDynamicUpdate
+#endif
+TEST_F(RecentTabsSubMenuModelTest, OtherDevicesDynamicUpdate) {
+  base::Time update_timestamp =
+      base::Time::Now() - base::TimeDelta::FromMinutes(10);
+
+  RecentTabsBuilderTestHelper recent_tabs_builder;
+
+  // Create one session with one window and one tab.
+  recent_tabs_builder.AddSession();
+  recent_tabs_builder.AddWindow(0);
+  recent_tabs_builder.AddTabWithInfo(0, 0, update_timestamp, base::string16());
+
+  RegisterRecentTabs(&recent_tabs_builder);
+
+  // Verify that data is populated correctly in RecentTabsSubMenuModel.
+  // Expected menu:
+  // - first inserted tab is most recent and hence is top
+  // Menu index  Menu items
+  // -----------------------------------------------------
+  // 0           History
+  // 1           <separator>
+  // 2           Recently closed header (disabled)
+  // 3           <separator>
+  // 4           <section header for 1st session>
+  // 5           <tab of the only window of session 0>
+
+  TestRecentTabsSubMenuModel model(nullptr, browser());
+  int num_items = model.GetItemCount();

[Peter Kasting, 2017/03/24 00:06:27]

Nit: Just inline into the next statement (2 places)

+  EXPECT_EQ(6, num_items);
+  model.ActivatedAt(0);

[Peter Kasting, 2017/03/24 00:06:27]

Again, is there a way to slim down this test to only check the things we need to
verify this fix?  I'm wondering how many ActivatedAt(), IsEnabledAt(),
GetLabelFontListAt(), and GetURLAndTitleForItemAtIndex() calls we can rip out
and still verify this.

Basically, I'd expect we could do something like:

* Get the item count
* Push a sync change
* Check that there's one additional item
* (Maybe) Check that the second-to-last item has the title of this new tab

And just not bother checking anything else, since all the rest should be covered
by other tests.

+  EXPECT_TRUE(model.IsEnabledAt(0));
+  model.ActivatedAt(1);
+  EXPECT_TRUE(model.IsEnabledAt(1));
+  model.ActivatedAt(2);
+  EXPECT_FALSE(model.IsEnabledAt(2));
+  model.ActivatedAt(3);
+  EXPECT_TRUE(model.IsEnabledAt(3));
+
+  model.ActivatedAt(5);
+  EXPECT_TRUE(model.IsEnabledAt(5));
+
+  EXPECT_EQ(2, model.enable_count());
+  EXPECT_EQ(5, model.execute_count());
+
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(0));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(1));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(2));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(3));
+  EXPECT_NE(nullptr, model.GetLabelFontListAt(4));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(5));
+
+  std::string url;
+  base::string16 title;
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(0, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(1, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(2, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(3, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(4, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(5, &url, &title));
+
+  // Make changes dynamically.
+  int previous_enable_count = model.enable_count();
+  int previous_execute_count = model.execute_count();
+
+  update_timestamp = base::Time::Now() - base::TimeDelta::FromMinutes(5);
+
+  // Add tab to the only window.
+  recent_tabs_builder.AddTabWithInfo(0, 0, update_timestamp, base::string16());
+
+  RegisterRecentTabs(&recent_tabs_builder);
+
+  // Verify that data is populated correctly in RecentTabsSubMenuModel.
+  // Expected menu:
+  // - first inserted tab is most recent and hence is top
+  // Menu index  Menu items
+  // -----------------------------------------------------
+  // 0           History
+  // 1           <separator>
+  // 2           Recently closed header (disabled)
+  // 3           <separator>
+  // 4           <section header for 1st session>
+  // 5           <new added tab of the only window of session 0>
+  // 6           <tab of the only window of session 0>
+
+  num_items = model.GetItemCount();
+  EXPECT_EQ(7, num_items);
+  model.ActivatedAt(0);
+  EXPECT_TRUE(model.IsEnabledAt(0));
+  model.ActivatedAt(1);
+  EXPECT_TRUE(model.IsEnabledAt(1));
+  model.ActivatedAt(2);
+  EXPECT_FALSE(model.IsEnabledAt(2));
+  model.ActivatedAt(3);
+  EXPECT_TRUE(model.IsEnabledAt(3));
+
+  model.ActivatedAt(5);
+  EXPECT_TRUE(model.IsEnabledAt(5));
+  model.ActivatedAt(6);
+  EXPECT_TRUE(model.IsEnabledAt(6));
+
+  EXPECT_EQ(previous_enable_count + 3, model.enable_count());
+  EXPECT_EQ(previous_execute_count + 6, model.execute_count());
+
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(0));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(1));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(2));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(3));
+  EXPECT_NE(nullptr, model.GetLabelFontListAt(4));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(5));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(6));
+
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(0, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(1, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(2, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(3, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(4, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(5, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(6, &url, &title));
+}
+
 // Per http://crbug.com/603744, MaxSessionsAndRecenty fails intermittently on
 // windows, linux and mac.
 #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
@@ -528,7 +717,7 @@ TEST_F(RecentTabsSubMenuModelTest, MAYBE_MaxSessionsAndRecency) {
   // 10          <section header for 3rd session>
   // 11          <the only tab of the only window of session 1>
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), GetOpenTabsDelegate());
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   int num_items = model.GetItemCount();
   EXPECT_EQ(12, num_items);
 
@@ -563,7 +752,7 @@ TEST_F(RecentTabsSubMenuModelTest, MaxTabsPerSessionAndRecency) {
   // 4           <section header for session>
   // 5-8         <4 most-recent tabs of session>
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), GetOpenTabsDelegate());
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   int num_items = model.GetItemCount();
   EXPECT_EQ(9, num_items);
 
@@ -590,7 +779,7 @@ TEST_F(RecentTabsSubMenuModelTest, MaxWidth) {
   // 4           <section header for 1st session>
   // 5           <the only tab of the only window of session 1>
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), GetOpenTabsDelegate());
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   EXPECT_EQ(6, model.GetItemCount());
   EXPECT_EQ(-1, model.GetMaxWidthForItemAtIndex(2));
   EXPECT_NE(-1, model.GetMaxWidthForItemAtIndex(3));
@@ -608,7 +797,7 @@ TEST_F(RecentTabsSubMenuModelTest, MaxWidthNoDevices) {
   // 3           <separator>
   // 4           No tabs from other Devices
 
-  TestRecentTabsSubMenuModel model(NULL, browser(), NULL);
+  TestRecentTabsSubMenuModel model(nullptr, browser());
   EXPECT_EQ(5, model.GetItemCount());
   EXPECT_EQ(-1, model.GetMaxWidthForItemAtIndex(2));
   EXPECT_NE(-1, model.GetMaxWidthForItemAtIndex(3));