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..c4ecbf4add9ea1b44daa827edbbc25b1222f4335 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) {}
 
@@ -120,22 +125,48 @@ class DummyRouter : public sync_sessions::LocalSessionEventRouter {
 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>(

[Peter Kasting, 2017/03/23 04:48:34]

This seems very heavyweight.  It would be nice to avoid Gmock entirely, perhaps
by simply calling the "foreign session updated" method of the menu model
directly?  See also comments in the sync code.

+        "RecentTabsSubMenuModelTest", "Test Machine", "Chromium 10k",
+        "Chrome 10k", sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "device_id");
+
+    sync_prefs_.reset(new syncer::SyncPrefs(profile()->GetPrefs()));
+
+    mock_sync_service_ = static_cast<browser_sync::ProfileSyncServiceMock*>(
+        ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+            profile(), BuildMockProfileSyncService));
+
+    EXPECT_CALL(*mock_sync_service_, AddObserver(_))
+        .WillRepeatedly(Invoke(this, &RecentTabsSubMenuModelTest::AddObserver));
+    EXPECT_CALL(*mock_sync_service_, RemoveObserver(_))
+        .WillRepeatedly(
+            Invoke(this, &RecentTabsSubMenuModelTest::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(
+            &browser_sync::ProfileSyncService::NotifyForeignSessionUpdated,
+            base::Unretained(mock_sync_service_)),
+        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 +175,21 @@ class RecentTabsSubMenuModelTest
             new syncer::SyncErrorFactoryMock));
   }
 
+  void TearDown() override {
+    manager_.reset();
+    sync_prefs_.reset();
+    local_device_.reset();
+    BrowserWithTestWindowTest::TearDown();
+  }
+
+  void AddObserver(syncer::SyncServiceObserver* observer) {
+    mock_sync_service_->SyncServiceBase::AddObserver(observer);
+  }
+
+  void RemoveObserver(syncer::SyncServiceObserver* observer) {
+    mock_sync_service_->SyncServiceBase::RemoveObserver(observer);
+  }
+
   void WaitForLoadFromLastSession() {
     content::RunAllBlockingPoolTasksUntilIdle();
   }
@@ -165,16 +211,15 @@ 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_;
+  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 +261,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 +355,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 +480,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);
@@ -491,6 +536,105 @@ TEST_F(RecentTabsSubMenuModelTest, OtherDevices) {
   EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(10, &url, &title));
   EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(11, &url, &title));
   EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(12, &url, &title));
+

[Peter Kasting, 2017/03/23 04:48:35]

Nit: Prefer to pull this out into its own test, even if it means duplicating
setup code (but consider refactoring that setup code to avoid duplication), and
give it (and this existing test) descriptive names so it's clear what aspects of
the other devices submenu they're testing.

+// Remove this when dynamic model is enabled in RecentTabsSubMenuModel.

[Peter Kasting, 2017/03/23 04:48:34]

Nit: This comment sounds like "remove this block", but I think you mean "remove
the #if".  Try "Enable this for Mac" instead?

+#if !defined(OS_MACOSX)
+  // Make changes dynamically
+  int previous_enable_count = model.enable_count();
+  int previous_execute_count = model.execute_count();
+
+  base::Time update_timestamp =
+      base::Time::Now() - base::TimeDelta::FromMinutes(5);
+
+  // Add tab to the only window in the first session and add tab to the seсond
+  // window in the second session.
+  recent_tabs_builder.AddTabWithInfo(0, 0, update_timestamp, base::string16());
+  recent_tabs_builder.AddTabWithInfo(1, 1, 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-8         <3 tabs of the only window of session 0>
+  // 9           <separator>
+  // 10          <section header for 2nd session>
+  // 11          <new added tab of window 1 of session 2>
+  // 12          <the only tab of window 0 of session 1>
+  // 13-14       <2 tabs of window 1 of session 2>
+
+  num_items = model.GetItemCount();

[Peter Kasting, 2017/03/23 04:48:34]

Do we really need to check everything below in order to verify the change you've
made in this CL?  It would be good to have a minimal test that just says, hey,
we added a tab somewhere, and we picked up on that in the menu, without
retesting everything else.

+  EXPECT_EQ(15, 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));
+  model.ActivatedAt(7);
+  EXPECT_TRUE(model.IsEnabledAt(7));
+  model.ActivatedAt(8);
+  EXPECT_TRUE(model.IsEnabledAt(8));
+
+  model.ActivatedAt(11);
+  EXPECT_TRUE(model.IsEnabledAt(11));
+  model.ActivatedAt(12);
+  EXPECT_TRUE(model.IsEnabledAt(12));
+  model.ActivatedAt(13);
+  EXPECT_TRUE(model.IsEnabledAt(13));
+  model.ActivatedAt(14);
+  EXPECT_TRUE(model.IsEnabledAt(14));
+
+  EXPECT_EQ(previous_enable_count + 9, model.enable_count());
+  EXPECT_EQ(previous_execute_count + 12, 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_TRUE(model.GetLabelFontListAt(4) != nullptr);

[Peter Kasting, 2017/03/23 04:48:35]

Nit: Can probably be EXPECT_NE(nullptr, ...); (2 places)

+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(5));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(6));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(7));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(8));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(9));
+  EXPECT_TRUE(model.GetLabelFontListAt(10) != nullptr);
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(11));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(12));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(13));
+  EXPECT_EQ(nullptr, model.GetLabelFontListAt(14));
+
+  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));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(7, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(8, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(9, &url, &title));
+  EXPECT_FALSE(model.GetURLAndTitleForItemAtIndex(10, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(11, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(12, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(13, &url, &title));
+  EXPECT_TRUE(model.GetURLAndTitleForItemAtIndex(14, &url, &title));
+#endif  // !defined(OS_MACOSX)
 }
 
 // Per http://crbug.com/603744, MaxSessionsAndRecenty fails intermittently on
@@ -528,7 +672,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 +707,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 +734,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 +752,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));