[sync] Scope BrowserListRouterHelper to browsers with a matching profile

chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc

Index: chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc
diff --git a/chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc b/chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..43bcf1221f65ecc9ba7bd69aed8a3deeaf1fa14d
--- /dev/null
+++ b/chrome/browser/sync/sessions/browser_list_router_helper_unittest.cc
@@ -0,0 +1,71 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/sessions/browser_list_router_helper.h"
+
+#include "base/stl_util.h"
+#include "chrome/browser/sync/sessions/sync_sessions_web_contents_router.h"
+#include "chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+
+namespace sync_sessions {
+
+class BrowserListRouterHelperTest : public BrowserWithTestWindowTest {
+ protected:
+  ~BrowserListRouterHelperTest() override {}
+
+  std::set<Browser*> attached_browsers(SyncSessionsWebContentsRouter* router) {

[skym, 2017/05/15 20:58:01]

The naming for this method is odd. GetAttachedBrowsers ? Also, it should take a
const& router, right?

[Patrick Noland, 2017/05/15 21:56:51]

n/a since I removed this.

+    return router->browser_list_helper_->attached_browsers_;
+  }
+};
+
+TEST_F(BrowserListRouterHelperTest, ObservationScopedToSingleProfile) {
+  std::unique_ptr<TestingProfile> profile_1 =
+      base::WrapUnique(BrowserWithTestWindowTest::CreateProfile());
+  std::unique_ptr<TestingProfile> profile_2 =
+      base::WrapUnique(BrowserWithTestWindowTest::CreateProfile());
+
+  std::unique_ptr<Browser> browser_2 = base::WrapUnique(CreateBrowser(
+      profile_2.get(), browser()->type(), false, CreateBrowserWindow()));
+
+  SyncSessionsWebContentsRouter* router_1 =
+      SyncSessionsWebContentsRouterFactory::GetInstance()->GetForProfile(
+          profile_1.get());
+  SyncSessionsWebContentsRouter* router_2 =
+      SyncSessionsWebContentsRouterFactory::GetInstance()->GetForProfile(
+          profile_2.get());
+
+  // Check that the initial browsers are only attached to their respective
+  // browser list helpers.
+  std::set<Browser*> attached_browsers_1 = attached_browsers(router_1);
+  std::set<Browser*> attached_browsers_2 = attached_browsers(router_2);
+  std::set<Browser*> intersection =
+      base::STLSetIntersection<std::set<Browser*>>(attached_browsers_1,
+                                                   attached_browsers_2);
+  EXPECT_EQ(0u, intersection.size());
+
+  // Add a browser for each profile.
+  std::unique_ptr<Browser> new_browser_in_first_profile =
+      base::WrapUnique(CreateBrowser(profile_1.get(), browser()->type(), false,
+                                     CreateBrowserWindow()));
+  std::unique_ptr<Browser> new_browser_in_second_profile =
+      base::WrapUnique(CreateBrowser(profile_2.get(), browser()->type(), false,
+                                     CreateBrowserWindow()));
+  // Check that the added browsers are only attached to their respective browser

[skym, 2017/05/15 20:58:01]

Would it be useful to check that browser counts as expected as well?

[Patrick Noland, 2017/05/15 21:56:51]

n/a since I removed this

+  // list helpers.
+  attached_browsers_1 = attached_browsers(router_1);
+  attached_browsers_2 = attached_browsers(router_2);
+  intersection = base::STLSetIntersection<std::set<Browser*>>(
+      attached_browsers_1, attached_browsers_2);
+  EXPECT_EQ(0u, intersection.size());
+
+  // Cleanup needed for manually created browsers so they don't complain about

[skym, 2017/05/15 20:58:01]

Who is "they"? Why does this test case care?

Is is possible to test that BrowserListRouterHelper::OnBrowserRemoved does the
right thing? If so, you could just drop this comment, right?

[Patrick Noland, 2017/05/15 21:56:51]

I will fix this comment, but the destructor for Browser will DCHECK if it has
unclosed tabs. The browser created by the base class is automatically cleaned
up, but manually created ones aren't. Even if I remove the browsers from the
helper, they still need to destruct at some point, so I need to close their
tabs. 

+  // having open tabs when destructing.
+  browser_2->tab_strip_model()->CloseAllTabs();
+  new_browser_in_first_profile->tab_strip_model()->CloseAllTabs();
+  new_browser_in_second_profile->tab_strip_model()->CloseAllTabs();
+}

[skym, 2017/05/15 20:58:01]

Would also be cool to test ~BrowserListRouterHelper() but feel free to not do
this.

[Patrick Noland, 2017/05/15 21:56:51]

Acknowledged.

+
+}  // namespace sync_sessions