Avoiding sending placeholder updates if window ID doesn't change

Sending the very same tab entity is wasteful: in most cases it will be
caught by other layers (which will realize the proto is identical) but
let's detect it in early stages, which also avoids unnecessary I/O to
disk.

Bug: 854493
Change-Id: Ia2ceaa4347bdf4dca1c5d0995b2466d07b41261a
Reviewed-on: https://chromium-review.googlesource.com/1107060Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568788}

components/sync_sessions/local_session_event_handler_impl.cc

@@ -177,6 +177,9 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
       if (synced_tab->IsPlaceholderTab()) {
         // For tabs without WebContents update |window_id|, as it could have
         // changed after a session restore.
+        // TODO(crbug.com/854493): Avoid associating placeholder tabs
+        // altogether, because it's not worth to update the window ID in tab
+        // entities.
         AssociateRestoredPlaceholderTab(*synced_tab, tab_id, window_id, batch);
       } else if (RELOAD_TABS == option) {
         AssociateTab(synced_tab, batch);
@@ -402,7 +405,10 @@ void LocalSessionEventHandlerImpl::AssociateRestoredPlaceholderTab(
   // Update the window id on the SessionTab itself.
   sessions::SessionTab* local_tab =
       session_tracker_->GetTab(current_session_tag_, tab_id);
-  // TODO(mastiz): Return early if the window ID hasn't changed.
+  if (local_tab->window_id == new_window_id) {
+    // Nothing to update.
+    return;
+  }
   local_tab->window_id = new_window_id;
 
   // Filter out placeholder tabs that have been associated but don't have known
@@ -412,7 +418,7 @@ void LocalSessionEventHandlerImpl::AssociateRestoredPlaceholderTab(
   }
 
   // Rewrite the specifics based on the reassociated SessionTab to preserve
-  // the new tab and window ids.
+  // the new window id.
   auto specifics = std::make_unique<sync_pb::SessionSpecifics>();
   local_tab->ToSyncData().Swap(specifics->mutable_tab());
   specifics->set_session_tag(current_session_tag_);

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -518,16 +518,17 @@ TEST_F(SessionSyncBridgeTest, ShouldReportLocalTabCreation) {
 }
 
 TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
-  const int kWindowId = 1000001;
-  const int kTabId1 = 1000002;
-  const int kTabId2 = 1000003;
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
+  const int kTabId1 = 1000003;
+  const int kTabId2 = 1000004;
   // Zero is the first assigned tab node ID.
   const int kTabNodeId1 = 0;
   const int kTabNodeId2 = 1;
 
-  AddWindow(kWindowId);
-  AddTab(kWindowId, "http://foo.com/", kTabId1);
-  AddTab(kWindowId, "http://bar.com/", kTabId2);
+  AddWindow(kWindowId1);
+  AddTab(kWindowId1, "http://foo.com/", kTabId1);
+  AddTab(kWindowId1, "http://bar.com/", kTabId2);
 
   const std::string header_storage_key =
       SessionStore::GetHeaderStorageKey(kLocalSessionTag);
@@ -541,14 +542,14 @@ TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
 
   ASSERT_THAT(GetData(header_storage_key),
               EntityDataHasSpecifics(MatchesHeader(
-                  kLocalSessionTag, {kWindowId}, {kTabId1, kTabId2})));
+                  kLocalSessionTag, {kWindowId1}, {kTabId1, kTabId2})));
   ASSERT_THAT(
       GetData(tab_storage_key1),
-      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId, kTabId1,
+      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId1, kTabId1,
                                         kTabNodeId1, {"http://foo.com/"})));
   ASSERT_THAT(
       GetData(tab_storage_key2),
-      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId, kTabId2,
+      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId1, kTabId2,
                                         kTabNodeId2, {"http://bar.com/"})));
 
   ShutdownBridge();
@@ -560,7 +561,7 @@ TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
   PlaceholderTabDelegate placeholder_tab2(
       SessionID::FromSerializedValue(kTabId2));
   ResetWindows();
-  TestSyncedWindowDelegate* window = AddWindow(kWindowId);
+  TestSyncedWindowDelegate* window = AddWindow(kWindowId2);
   window->OverrideTabAt(0, &placeholder_tab1);
   window->OverrideTabAt(1, &placeholder_tab2);
 
@@ -569,16 +570,16 @@ TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
   EXPECT_CALL(mock_processor(),
               Put(header_storage_key,
                   EntityDataHasSpecifics(MatchesHeader(
-                      kLocalSessionTag, {kWindowId}, {kTabId1, kTabId2})),
+                      kLocalSessionTag, {kWindowId2}, {kTabId1, kTabId2})),
                   _));
   EXPECT_CALL(mock_processor(), Put(tab_storage_key1,
                                     EntityDataHasSpecifics(MatchesTab(
-                                        kLocalSessionTag, kWindowId, kTabId1,
+                                        kLocalSessionTag, kWindowId2, kTabId1,
                                         kTabNodeId1, {"http://foo.com/"})),
                                     _));
   EXPECT_CALL(mock_processor(), Put(tab_storage_key2,
                                     EntityDataHasSpecifics(MatchesTab(
-                                        kLocalSessionTag, kWindowId, kTabId2,
+                                        kLocalSessionTag, kWindowId2, kTabId2,
                                         kTabNodeId2, {"http://bar.com/"})),
                                     _));
 
@@ -588,14 +589,14 @@ TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
 
   EXPECT_THAT(GetData(header_storage_key),
               EntityDataHasSpecifics(MatchesHeader(
-                  kLocalSessionTag, {kWindowId}, {kTabId1, kTabId2})));
+                  kLocalSessionTag, {kWindowId2}, {kTabId1, kTabId2})));
   EXPECT_THAT(
       GetData(tab_storage_key1),
-      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId, kTabId1,
+      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId2, kTabId1,
                                         kTabNodeId1, {"http://foo.com/"})));
   EXPECT_THAT(
       GetData(tab_storage_key2),
-      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId, kTabId2,
+      EntityDataHasSpecifics(MatchesTab(kLocalSessionTag, kWindowId2, kTabId2,
                                         kTabNodeId2, {"http://bar.com/"})));
 
   EXPECT_THAT(
@@ -603,27 +604,28 @@ TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
       UnorderedElementsAre(
           Pair(header_storage_key,
                EntityDataHasSpecifics(MatchesHeader(
-                   kLocalSessionTag, {kWindowId}, {kTabId1, kTabId2}))),
+                   kLocalSessionTag, {kWindowId2}, {kTabId1, kTabId2}))),
           Pair(tab_storage_key1, EntityDataHasSpecifics(MatchesTab(
-                                     kLocalSessionTag, kWindowId, kTabId1,
+                                     kLocalSessionTag, kWindowId2, kTabId1,
                                      kTabNodeId1, {"http://foo.com/"}))),
           Pair(tab_storage_key2, EntityDataHasSpecifics(MatchesTab(
-                                     kLocalSessionTag, kWindowId, kTabId2,
+                                     kLocalSessionTag, kWindowId2, kTabId2,
                                      kTabNodeId2, {"http://bar.com/"})))));
 }
 
 TEST_F(SessionSyncBridgeTest,
        ShouldIgnoreUnsyncablePlaceholderTabDuringRestore) {
-  const int kWindowId = 1000001;
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
   const int kTabId1 = 1000002;
   const int kTabId2 = 1000003;
   // Zero is the first assigned tab node ID.
   const int kTabNodeId1 = 0;
 
-  AddWindow(kWindowId);
-  AddTab(kWindowId, "http://foo.com/", kTabId1);
+  AddWindow(kWindowId1);
+  AddTab(kWindowId1, "http://foo.com/", kTabId1);
   // Tab 2 is unsyncable because of the URL scheme.
-  AddTab(kWindowId, "about:blank", kTabId2);
+  AddTab(kWindowId1, "about:blank", kTabId2);
 
   const std::string header_storage_key =
       SessionStore::GetHeaderStorageKey(kLocalSessionTag);
@@ -638,9 +640,9 @@ TEST_F(SessionSyncBridgeTest,
       UnorderedElementsAre(
           Pair(header_storage_key,
                EntityDataHasSpecifics(
-                   MatchesHeader(kLocalSessionTag, {kWindowId}, {kTabId1}))),
+                   MatchesHeader(kLocalSessionTag, {kWindowId1}, {kTabId1}))),
           Pair(tab_storage_key1, EntityDataHasSpecifics(MatchesTab(
-                                     kLocalSessionTag, kWindowId, kTabId1,
+                                     kLocalSessionTag, kWindowId1, kTabId1,
                                      kTabNodeId1, {"http://foo.com/"})))));
 
   ShutdownBridge();
@@ -652,7 +654,7 @@ TEST_F(SessionSyncBridgeTest,
   PlaceholderTabDelegate placeholder_tab2(
       SessionID::FromSerializedValue(kTabId2));
   ResetWindows();
-  TestSyncedWindowDelegate* window = AddWindow(kWindowId);
+  TestSyncedWindowDelegate* window = AddWindow(kWindowId2);
   window->OverrideTabAt(0, &placeholder_tab1);
   window->OverrideTabAt(1, &placeholder_tab2);
 
@@ -665,20 +667,21 @@ TEST_F(SessionSyncBridgeTest,
       UnorderedElementsAre(
           Pair(header_storage_key,
                EntityDataHasSpecifics(
-                   MatchesHeader(kLocalSessionTag, {kWindowId}, {kTabId1}))),
+                   MatchesHeader(kLocalSessionTag, {kWindowId2}, {kTabId1}))),
           Pair(tab_storage_key1, EntityDataHasSpecifics(MatchesTab(
-                                     kLocalSessionTag, kWindowId, kTabId1,
+                                     kLocalSessionTag, kWindowId2, kTabId1,
                                      kTabNodeId1, {"http://foo.com/"})))));
 }
 
 // Ensure that tabbed windows from a previous session are preserved if no
 // windows are present on startup.
 TEST_F(SessionSyncBridgeTest, ShouldRestoreTabbedDataIfNoWindowsDuringStartup) {
-  const int kWindowId = 1000001;
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
   const int kTabNodeId = 0;
 
-  AddWindow(kWindowId);
-  TestSyncedTabDelegate* tab = AddTab(kWindowId, "http://foo.com/");
+  AddWindow(kWindowId1);
+  TestSyncedTabDelegate* tab = AddTab(kWindowId1, "http://foo.com/");
 
   const std::string header_storage_key =
       SessionStore::GetHeaderStorageKey(kLocalSessionTag);
@@ -725,7 +728,7 @@ TEST_F(SessionSyncBridgeTest, ShouldRestoreTabbedDataIfNoWindowsDuringStartup) {
                       kLocalSessionTag, /*window_id=*/_, /*tab_id=*/_,
                       kTabNodeId, {"http://foo.com/", "http://bar.com/"})),
                   _));
-  AddWindow(kWindowId)->OverrideTabAt(0, tab);
+  AddWindow(kWindowId2)->OverrideTabAt(0, tab);
   tab->Navigate("http://bar.com/");
 }
 

components/sync_sessions/sessions_sync_manager_unittest.cc

@@ -1799,9 +1799,12 @@ TEST_F(SessionsSyncManagerTest, SwappedOutOnRestore) {
   in.push_back(CreateRemoteData(t1_entity));
   out.clear();
   manager()->StopSyncing(syncer::SESSIONS);
+  ResetWindows();
 
   PlaceholderTabDelegate t1_override(
       SessionID::FromSerializedValue(t1_entity.session().tab().tab_id()));
+  window = AddWindow();
+  window->OverrideTabAt(0, tab1);
   window->OverrideTabAt(1, &t1_override);
   InitWithSyncDataTakeOutput(in, &out);