Make some tab restore tests observe service loading directly.

This replaces the slower, more generic RunAllTasksUntilIdle() and
saves code to boot.

I thought this might make the tests less flaky, but it turned out to
be unrelated to the flakiness I was investigating.  Still seems worth
doing.

Bug: none
Change-Id: I66814c7596bac79f1e5b05356a86fd87984983cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071586
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744365}

chrome/browser/BUILD.gn

@@ -5826,6 +5826,8 @@ static_library("test_support") {
       "sessions/session_service_test_helper.h",
       "sessions/tab_loader_tester.cc",
       "sessions/tab_loader_tester.h",
+      "sessions/tab_restore_service_load_waiter.cc",
+      "sessions/tab_restore_service_load_waiter.h",
       "ui/tabs/tab_activity_simulator.cc",
       "ui/tabs/tab_activity_simulator.h",
     ]

chrome/browser/sessions/tab_restore_browsertest.cc

@@ -136,7 +136,8 @@ class TabRestoreTest : public InProcessBrowserTest {
         content::NOTIFICATION_LOAD_STOP,
         content::NotificationService::AllSources());
     {
-      TabRestoreServiceLoadWaiter waiter(browser);
+      TabRestoreServiceLoadWaiter waiter(
+          TabRestoreServiceFactory::GetForProfile(browser->profile()));
       chrome::RestoreTab(browser);
       waiter.Wait();
     }
@@ -858,13 +859,13 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, PRE_GetRestoreTabType) {
   // automatically upon launch.
   // Wait for robustness because InProcessBrowserTest::PreRunTestOnMainThread
   // does not flush the task scheduler.
-  TabRestoreServiceLoadWaiter waiter(browser());
-  waiter.Wait();
-
-  // When we start, we should get nothing.
   sessions::TabRestoreService* service =
       TabRestoreServiceFactory::GetForProfile(browser()->profile());
   ASSERT_TRUE(service);
+  TabRestoreServiceLoadWaiter waiter(service);
+  waiter.Wait();
+
+  // When we start, we should get nothing.
   EXPECT_TRUE(service->entries().empty());
 
   // Add a tab and close it
@@ -887,13 +888,13 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, GetRestoreTabType) {
   // automatically upon launch.
   // Wait for robustness because InProcessBrowserTest::PreRunTestOnMainThread
   // does not flush the task scheduler.
-  TabRestoreServiceLoadWaiter waiter(browser());
-  waiter.Wait();
-
-  // When we start this time we should get a Tab.
   sessions::TabRestoreService* service =
       TabRestoreServiceFactory::GetForProfile(browser()->profile());
   ASSERT_TRUE(service);
+  TabRestoreServiceLoadWaiter waiter(service);
+  waiter.Wait();
+
+  // When we start this time we should get a Tab.
   ASSERT_GE(service->entries().size(), 1u);
   EXPECT_EQ(sessions::TabRestoreService::TAB, service->entries().front()->type);
 }

chrome/browser/sessions/tab_restore_service_load_waiter.cc

@@ -4,32 +4,20 @@
 
 #include "chrome/browser/sessions/tab_restore_service_load_waiter.h"
 
-#include "chrome/browser/sessions/tab_restore_service_factory.h"
-#include "chrome/browser/ui/browser.h"
-#include "components/sessions/core/tab_restore_service.h"
-
-// Class used to run a message loop waiting for the TabRestoreService to finish
-// loading. Does nothing if the TabRestoreService was already loaded.
-TabRestoreServiceLoadWaiter::TabRestoreServiceLoadWaiter(Browser* browser)
-    : tab_restore_service_(
-          TabRestoreServiceFactory::GetForProfile(browser->profile())),
-      do_wait_(!tab_restore_service_->IsLoaded()) {
-  if (do_wait_)
-    tab_restore_service_->AddObserver(this);
+TabRestoreServiceLoadWaiter::TabRestoreServiceLoadWaiter(
+    sessions::TabRestoreService* service)
+    : service_(service) {
+  observer_.Add(service_);
 }
 
-TabRestoreServiceLoadWaiter::~TabRestoreServiceLoadWaiter() {
-  if (do_wait_)
-    tab_restore_service_->RemoveObserver(this);
-}
+TabRestoreServiceLoadWaiter::~TabRestoreServiceLoadWaiter() = default;
 
 void TabRestoreServiceLoadWaiter::Wait() {
-  if (do_wait_)
+  if (!service_->IsLoaded())
     run_loop_.Run();
 }
 
 void TabRestoreServiceLoadWaiter::TabRestoreServiceLoaded(
     sessions::TabRestoreService* service) {
-  DCHECK(do_wait_);
   run_loop_.Quit();
 }

chrome/browser/sessions/tab_restore_service_load_waiter.h

@@ -6,31 +6,30 @@
 #define CHROME_BROWSER_SESSIONS_TAB_RESTORE_SERVICE_LOAD_WAITER_H_
 
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "components/sessions/core/tab_restore_service.h"
 #include "components/sessions/core/tab_restore_service_observer.h"
 
-class Browser;
-
 // Class used to run a message loop waiting for the TabRestoreService to finish
 // loading. Does nothing if the TabRestoreService was already loaded.
 class TabRestoreServiceLoadWaiter : public sessions::TabRestoreServiceObserver {
  public:
-  explicit TabRestoreServiceLoadWaiter(Browser* browser);
-
+  explicit TabRestoreServiceLoadWaiter(sessions::TabRestoreService* service);
   ~TabRestoreServiceLoadWaiter() override;
 
   void Wait();
 
  private:
-  // Overridden from TabRestoreServiceObserver:
+  // TabRestoreServiceObserver:
   void TabRestoreServiceDestroyed(
       sessions::TabRestoreService* service) override {}
   void TabRestoreServiceLoaded(sessions::TabRestoreService* service) override;
 
-  sessions::TabRestoreService* const tab_restore_service_;
-  const bool do_wait_;
+  sessions::TabRestoreService* const service_;
   base::RunLoop run_loop_;
-
-  DISALLOW_COPY_AND_ASSIGN(TabRestoreServiceLoadWaiter);
+  ScopedObserver<sessions::TabRestoreService,
+                 sessions::TabRestoreServiceObserver>
+      observer_{this};
 };
 
 #endif  // CHROME_BROWSER_SESSIONS_TAB_RESTORE_SERVICE_LOAD_WAITER_H_

chrome/browser/sessions/tab_restore_service_unittest.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sessions/session_service_utils.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/sessions/tab_restore_service_load_waiter.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/chrome_render_view_test.h"
@@ -179,8 +180,9 @@ class TabRestoreServiceImplTest : public ChromeRenderViewHostTestHarness {
 
   void SynchronousLoadTabsFromLastSession() {
     // Ensures that the load is complete before continuing.
+    TabRestoreServiceLoadWaiter waiter(service_.get());
     service_->LoadTabsFromLastSession();
-    content::RunAllTasksUntilIdle();
+    waiter.Wait();
   }
 
   sessions::LiveTab* live_tab() { return live_tab_.get(); }
@@ -196,32 +198,6 @@ class TabRestoreServiceImplTest : public ChromeRenderViewHostTestHarness {
   SessionID tab_id_;
 };
 
-namespace {
-
-class TestTabRestoreServiceObserver
-    : public sessions::TabRestoreServiceObserver {
- public:
-  TestTabRestoreServiceObserver() : got_loaded_(false) {}
-
-  void clear_got_loaded() { got_loaded_ = false; }
-  bool got_loaded() const { return got_loaded_; }
-
-  // TabRestoreServiceObserver:
-  void TabRestoreServiceDestroyed(
-      sessions::TabRestoreService* service) override {}
-  void TabRestoreServiceLoaded(sessions::TabRestoreService* service) override {
-    got_loaded_ = true;
-  }
-
- private:
-  // Was TabRestoreServiceLoaded() invoked?
-  bool got_loaded_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestTabRestoreServiceObserver);
-};
-
-}  // namespace
-
 TEST_F(TabRestoreServiceImplTest, Basic) {
   AddThreeNavigations();
 
@@ -285,6 +261,7 @@ TEST_F(TabRestoreServiceImplTest, Restore) {
 
   // Have the service record the tab.
   service_->CreateHistoricalTab(live_tab(), -1);
+  EXPECT_EQ(1U, service_->entries().size());
 
   // Recreate the service and have it load the tabs.
   RecreateService();
@@ -513,11 +490,7 @@ TEST_F(TabRestoreServiceImplTest, LoadPreviousSession) {
 
   EXPECT_FALSE(service_->IsLoaded());
 
-  TestTabRestoreServiceObserver observer;
-  service_->AddObserver(&observer);
   SynchronousLoadTabsFromLastSession();
-  EXPECT_TRUE(observer.got_loaded());
-  service_->RemoveObserver(&observer);
 
   // Make sure we get back one entry with one tab whose url is url1.
   ASSERT_EQ(1U, service_->entries().size());
@@ -984,11 +957,7 @@ TEST_F(TabRestoreServiceImplTest, GoToLoadedWhenHaveMaxEntries) {
   }
 
   EXPECT_FALSE(service_->IsLoaded());
-  TestTabRestoreServiceObserver observer;
-  service_->AddObserver(&observer);
   EXPECT_EQ(max_entries, service_->entries().size());
   SynchronousLoadTabsFromLastSession();
-  EXPECT_TRUE(observer.got_loaded());
   EXPECT_TRUE(service_->IsLoaded());
-  service_->RemoveObserver(&observer);
 }

chrome/browser/ui/browser_command_controller_browsertest.cc

@@ -173,7 +173,8 @@ IN_PROC_BROWSER_TEST_F(BrowserCommandControllerBrowserTest,
   // automatically upon launch.
   // Wait for robustness because InProcessBrowserTest::PreRunTestOnMainThread
   // does not flush the task scheduler.
-  TabRestoreServiceLoadWaiter waiter(browser());
+  TabRestoreServiceLoadWaiter waiter(
+      TabRestoreServiceFactory::GetForProfile(browser()->profile()));
   waiter.Wait();
 
   // After initialization, the command should become disabled because there's
@@ -203,7 +204,8 @@ IN_PROC_BROWSER_TEST_F(BrowserCommandControllerBrowserTest,
   // automatically upon launch.
   // Wait for robustness because InProcessBrowserTest::PreRunTestOnMainThread
   // does not flush the task scheduler.
-  TabRestoreServiceLoadWaiter waiter(browser());
+  TabRestoreServiceLoadWaiter waiter(
+      TabRestoreServiceFactory::GetForProfile(browser()->profile()));
   waiter.Wait();
 
   // After initialization, the command should remain enabled because there's

chrome/test/BUILD.gn

@@ -1179,8 +1179,6 @@ if (!is_android) {
       "../browser/sessions/session_restore_observer_browsertest.cc",
       "../browser/sessions/tab_restore_browsertest.cc",
       "../browser/sessions/tab_restore_service_browsertest.cc",
-      "../browser/sessions/tab_restore_service_load_waiter.cc",
-      "../browser/sessions/tab_restore_service_load_waiter.h",
       "../browser/signin/e2e_tests/live_sign_in_test.cc",
       "../browser/signin/e2e_tests/live_test.cc",
       "../browser/signin/e2e_tests/live_test.h",