Avoid test dependency to SESSIONS SyncableService

These unit tests are the last examples of tests depending on
implementation details of SESSIONS sync, where overriding a feature
toggle was necessary.

Instead, let's embrace the new USS implementation.

Longer term, it may make sense to remove this dependency altogether and
migrate these unit tests to a test-specific OpenTabsUIDelegate. For
anything fancier, we should use proper sync integration tests. This
intermediate step makes it more obvious that there aren't behavioral
changes between the two sync implementations.

Bug: 895455
Change-Id: I1c3f26ec3037d71908cbf090644bf0580d8fd41b
Reviewed-on: https://chromium-review.googlesource.com/c/1301435
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608641}

chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc

@@ -6,13 +6,22 @@
 
 #include <stddef.h>
 
+#include <algorithm>
+#include <string>
+
 #include "base/rand_util.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/sync/base/hash_util.h"
+#include "components/sync/engine/model_type_processor.h"
+#include "components/sync/engine/non_blocking_sync_common.h"
+#include "components/sync/model/entity_data.h"
+#include "components/sync/model_impl/in_memory_metadata_change_list.h"
 #include "components/sync/protocol/session_specifics.pb.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
-#include "components/sync_sessions/sessions_sync_manager.h"
+#include "components/sync_sessions/session_store.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -75,8 +84,7 @@ struct RecentTabsBuilderTestHelper::SessionInfo {
   std::vector<WindowInfo> windows;
 };
 
-RecentTabsBuilderTestHelper::RecentTabsBuilderTestHelper()
-    : max_tab_node_id_(0) {
+RecentTabsBuilderTestHelper::RecentTabsBuilderTestHelper() {
   start_time_ = base::Time::Now();
 }
 
@@ -176,32 +184,32 @@ base::string16 RecentTabsBuilderTestHelper::GetTabTitle(int session_index,
   return title;
 }
 
-void RecentTabsBuilderTestHelper::ExportToSessionsSyncManager(
-    sync_sessions::SessionsSyncManager* manager) {
-  syncer::SyncChangeList changes;
+void RecentTabsBuilderTestHelper::ExportToSessionSync(
+    syncer::ModelTypeProcessor* processor,
+    sync_sessions::OpenTabsUIDelegate* verification_delegate) {
+  syncer::UpdateResponseDataList updates;
+
   for (int s = 0; s < GetSessionCount(); ++s) {
-    sync_pb::EntitySpecifics session_entity;
-    sync_pb::SessionSpecifics* meta = session_entity.mutable_session();
-    BuildSessionSpecifics(s, meta);
+    sync_pb::SessionSpecifics header_specifics = BuildHeaderSpecifics(s);
     for (int w = 0; w < GetWindowCount(s); ++w) {
-      BuildWindowSpecifics(s, w, meta);
+      AddWindowToHeaderSpecifics(s, w, &header_specifics);
       for (int t = 0; t < GetTabCount(s, w); ++t) {
-        sync_pb::EntitySpecifics entity;
-        sync_pb::SessionSpecifics* tab_base = entity.mutable_session();
-        BuildTabSpecifics(s, w, t, tab_base);
-        changes.push_back(syncer::SyncChange(
-            FROM_HERE, syncer::SyncChange::ACTION_ADD,
-            syncer::SyncData::CreateRemoteData(tab_base->tab_node_id(), entity,
-                                               GetTabTimestamp(s, w, t))));
+        updates.push_back(BuildUpdateResponseData(BuildTabSpecifics(s, w, t),
+                                                  GetTabTimestamp(s, w, t)));
       }
     }
-    changes.push_back(
-        syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD,
-                           syncer::SyncData::CreateRemoteData(
-                               1, session_entity, GetSessionTimestamp(s))));
+
+    updates.push_back(
+        BuildUpdateResponseData(header_specifics, GetSessionTimestamp(s)));
   }
-  manager->ProcessSyncChanges(FROM_HERE, changes);
-  VerifyExport(manager->GetOpenTabsUIDelegate());
+
+  sync_pb::ModelTypeState model_type_state;
+  model_type_state.set_initial_sync_done(true);
+  processor->OnUpdateReceived(model_type_state, updates);
+  // ClientTagBasedModelTypeProcessor uses ModelTypeProcessorProxy during
+  // activation, which involves task posting for receiving updates.
+  base::RunLoop().RunUntilIdle();
+  VerifyExport(verification_delegate);
 }
 
 void RecentTabsBuilderTestHelper::VerifyExport(
@@ -241,22 +249,22 @@ RecentTabsBuilderTestHelper::GetTabTitlesSortedByRecency() {
   return titles;
 }
 
-void RecentTabsBuilderTestHelper::BuildSessionSpecifics(
-    int session_index,
-    sync_pb::SessionSpecifics* meta) {
+sync_pb::SessionSpecifics RecentTabsBuilderTestHelper::BuildHeaderSpecifics(
+    int session_index) {
+  sync_pb::SessionSpecifics specifics;
   SessionID session_id = GetSessionID(session_index);
-  meta->set_session_tag(ToSessionTag(session_id));
-  sync_pb::SessionHeader* header = meta->mutable_header();
+  specifics.set_session_tag(ToSessionTag(session_id));
+  sync_pb::SessionHeader* header = specifics.mutable_header();
   header->set_device_type(sync_pb::SyncEnums_DeviceType_TYPE_CROS);
   header->set_client_name(ToSessionName(session_id));
+  return specifics;
 }
 
-void RecentTabsBuilderTestHelper::BuildWindowSpecifics(
+void RecentTabsBuilderTestHelper::AddWindowToHeaderSpecifics(
     int session_index,
     int window_index,
-    sync_pb::SessionSpecifics* meta) {
-  sync_pb::SessionHeader* header = meta->mutable_header();
-  sync_pb::SessionWindow* window = header->add_window();
+    sync_pb::SessionSpecifics* specifics) {
+  sync_pb::SessionWindow* window = specifics->mutable_header()->add_window();
   SessionID window_id = GetWindowID(session_index, window_index);
   window->set_window_id(window_id.id());
   window->set_selected_tab_index(0);
@@ -265,18 +273,19 @@ void RecentTabsBuilderTestHelper::BuildWindowSpecifics(
     window->add_tab(GetTabID(session_index, window_index, i).id());
 }
 
-void RecentTabsBuilderTestHelper::BuildTabSpecifics(
+sync_pb::SessionSpecifics RecentTabsBuilderTestHelper::BuildTabSpecifics(
     int session_index,
     int window_index,
-    int tab_index,
-    sync_pb::SessionSpecifics* tab_base) {
+    int tab_index) {
+  sync_pb::SessionSpecifics specifics;
+
   SessionID session_id = GetSessionID(session_index);
   SessionID window_id = GetWindowID(session_index, window_index);
   SessionID tab_id = GetTabID(session_index, window_index, tab_index);
 
-  tab_base->set_session_tag(ToSessionTag(session_id));
-  tab_base->set_tab_node_id(++max_tab_node_id_);
-  sync_pb::SessionTab* tab = tab_base->mutable_tab();
+  specifics.set_session_tag(ToSessionTag(session_id));
+  specifics.set_tab_node_id(++max_tab_node_id_);
+  sync_pb::SessionTab* tab = specifics.mutable_tab();
   tab->set_window_id(window_id.id());
   tab->set_tab_id(tab_id.id());
   tab->set_tab_visual_index(1);
@@ -289,4 +298,23 @@ void RecentTabsBuilderTestHelper::BuildTabSpecifics(
   navigation->set_title(base::UTF16ToUTF8(GetTabTitle(
       session_index, window_index, tab_index)));
   navigation->set_page_transition(sync_pb::SyncEnums_PageTransition_TYPED);
+
+  return specifics;
+}
+
+syncer::UpdateResponseData RecentTabsBuilderTestHelper::BuildUpdateResponseData(
+    const sync_pb::SessionSpecifics& specifics,
+    base::Time timestamp) {
+  syncer::EntityData entity;
+  *entity.specifics.mutable_session() = specifics;
+  entity.creation_time = timestamp;
+  entity.modification_time = timestamp;
+  entity.client_tag_hash = syncer::GenerateSyncableHash(
+      syncer::SESSIONS, sync_sessions::SessionStore::GetClientTag(specifics));
+  entity.id = entity.client_tag_hash;
+
+  syncer::UpdateResponseData update;
+  update.entity = entity.PassToPtr();
+  update.response_version = ++next_response_version_;
+  return update;
 }

chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.h

@@ -18,7 +18,11 @@ class SessionSpecifics;
 
 namespace sync_sessions {
 class OpenTabsUIDelegate;
-class SessionsSyncManager;
+}
+
+namespace syncer {
+class ModelTypeProcessor;
+struct UpdateResponseData;
 }
 
 // Utility class to help add recent tabs for testing.
@@ -50,20 +54,23 @@ class RecentTabsBuilderTestHelper {
                        int window_index,
                        int tab_index);
 
-  void ExportToSessionsSyncManager(sync_sessions::SessionsSyncManager* manager);
+  void ExportToSessionSync(
+      syncer::ModelTypeProcessor* processor,
+      sync_sessions::OpenTabsUIDelegate* verification_delegate);
 
   std::vector<base::string16> GetTabTitlesSortedByRecency();
 
  private:
-  void BuildSessionSpecifics(int session_index,
-                             sync_pb::SessionSpecifics* meta);
-  void BuildWindowSpecifics(int session_index,
-                            int window_index,
-                            sync_pb::SessionSpecifics* meta);
-  void BuildTabSpecifics(int session_index,
-                         int window_index,
-                         int tab_index,
-                         sync_pb::SessionSpecifics* tab_base);
+  sync_pb::SessionSpecifics BuildHeaderSpecifics(int session_index);
+  void AddWindowToHeaderSpecifics(int session_index,
+                                  int window_index,
+                                  sync_pb::SessionSpecifics* specifics);
+  sync_pb::SessionSpecifics BuildTabSpecifics(int session_index,
+                                              int window_index,
+                                              int tab_index);
+  syncer::UpdateResponseData BuildUpdateResponseData(
+      const sync_pb::SessionSpecifics& specifics,
+      base::Time timestamp);
   void VerifyExport(sync_sessions::OpenTabsUIDelegate* delegate);
 
   struct TabInfo;
@@ -73,7 +80,8 @@ class RecentTabsBuilderTestHelper {
   std::vector<SessionInfo> sessions_;
   base::Time start_time_;
 
-  int max_tab_node_id_;
+  int max_tab_node_id_ = 0;
+  int next_response_version_ = 1;
 
   DISALLOW_COPY_AND_ASSIGN(RecentTabsBuilderTestHelper);
 };

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -14,7 +15,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/scoped_feature_list.h"
+#include "base/test/bind_test_util.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/sessions/chrome_tab_restore_service_client.h"
@@ -36,21 +37,15 @@
 #include "components/sessions/core/session_types.h"
 #include "components/sync/device_info/local_device_info_provider_mock.h"
 #include "components/sync/driver/data_type_controller.h"
-#include "components/sync/driver/sync_driver_switches.h"
-#include "components/sync/model/fake_sync_change_processor.h"
-#include "components/sync/model/sync_error_factory_mock.h"
+#include "components/sync/engine/data_type_activation_response.h"
+#include "components/sync/model/data_type_activation_request.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "components/sync_sessions/sessions_sync_manager.h"
 #include "components/sync_sessions/synced_session.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_utils.h"
-#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
@@ -118,9 +113,7 @@ class TestRecentTabsMenuModelDelegate : public ui::MenuModelDelegate {
 class RecentTabsSubMenuModelTest
     : public BrowserWithTestWindowTest {
  public:
-  RecentTabsSubMenuModelTest() {
-    override_features_.InitAndDisableFeature(switches::kSyncUSSSessions);
-  }
+  RecentTabsSubMenuModelTest() {}
 
   void WaitForLoadFromLastSession() { content::RunAllTasksUntilIdle(); }
 
@@ -128,14 +121,28 @@ class RecentTabsSubMenuModelTest
     BrowserWithTestWindowTest::SetUp();
     session_sync_service_ = SessionSyncServiceFactory::GetForProfile(profile());
 
+    syncer::DataTypeActivationRequest activation_request;
+    activation_request.cache_guid = "test_cache_guid";
+    activation_request.error_handler = base::DoNothing();
+
     ProfileSyncServiceFactory::GetForProfile(profile())
         ->GetLocalDeviceInfoProviderForTest()
-        ->Initialize("RecentTabsSubMenuModelTest", "Test Machine", "device_id");
-
-    session_sync_service_->GetSyncableService()->MergeDataAndStartSyncing(
-        syncer::SESSIONS, syncer::SyncDataList(),
-        std::make_unique<syncer::FakeSyncChangeProcessor>(),
-        std::make_unique<syncer::SyncErrorFactoryMock>());
+        ->Initialize(activation_request.cache_guid, "Test Machine",
+                     "device_id");
+
+    std::unique_ptr<syncer::DataTypeActivationResponse> activation_response;
+    base::RunLoop loop;
+    session_sync_service_->GetControllerDelegate()->OnSyncStarting(
+        activation_request,
+        base::BindLambdaForTesting(
+            [&](std::unique_ptr<syncer::DataTypeActivationResponse> response) {
+              activation_response = std::move(response);
+              loop.Quit();
+            }));
+    loop.Run();
+    ASSERT_NE(nullptr, activation_response);
+    ASSERT_NE(nullptr, activation_response->type_processor);
+    sync_processor_ = std::move(activation_response->type_processor);
 
     EnableSync();
   }
@@ -159,17 +166,14 @@ class RecentTabsSubMenuModelTest
   }
 
   void RegisterRecentTabs(RecentTabsBuilderTestHelper* helper) {
-    // The static_cast below is safe due to the feature override for
-    // switches::kSyncUSSSessions.
-    helper->ExportToSessionsSyncManager(
-        static_cast<sync_sessions::SessionsSyncManager*>(
-            SessionSyncServiceFactory::GetForProfile(profile())
-                ->GetSyncableService()));
+    helper->ExportToSessionSync(
+        sync_processor_.get(),
+        session_sync_service_->GetUnderlyingOpenTabsUIDelegateForTest());
   }
 
  private:
-  base::test::ScopedFeatureList override_features_;
   sync_sessions::SessionSyncService* session_sync_service_;
+  std::unique_ptr<syncer::ModelTypeProcessor> sync_processor_;
 
   DISALLOW_COPY_AND_ASSIGN(RecentTabsSubMenuModelTest);
 };

components/sync_sessions/session_sync_service.cc

@@ -93,6 +93,11 @@ void SessionSyncService::SetSyncSessionsGUID(const std::string& guid) {
   sessions_client_->GetSessionSyncPrefs()->SetSyncSessionsGUID(guid);
 }
 
+OpenTabsUIDelegate*
+SessionSyncService::GetUnderlyingOpenTabsUIDelegateForTest() {
+  return sessions_sync_manager_->GetOpenTabsUIDelegate();
+}
+
 void SessionSyncService::NotifyForeignSessionUpdated() {
   foreign_sessions_changed_callback_list_.Notify();
 }

components/sync_sessions/session_sync_service.h

@@ -69,6 +69,10 @@ class SessionSyncService : public KeyedService {
   // Used on Android only, to override the machine tag.
   void SetSyncSessionsGUID(const std::string& guid);
 
+  // Returns OpenTabsUIDelegate regardless of sync being enabled or disabled,
+  // useful for tests.
+  OpenTabsUIDelegate* GetUnderlyingOpenTabsUIDelegateForTest();
+
  private:
   void NotifyForeignSessionUpdated();